Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a job for building --without-ssl #1574

Closed
danbev opened this issue Nov 15, 2018 · 26 comments
Closed

Create a job for building --without-ssl #1574

danbev opened this issue Nov 15, 2018 · 26 comments
Assignees
Labels
ci-change PSA of configuration changes ci-job-request ci-public

Comments

@danbev
Copy link
Contributor

danbev commented Nov 15, 2018

I would be great to have a job that builds and tests node configuring it using the --without-ssl flag. Currently it is easy to forget to test this option and PRs are merged without us noticing.

I noticed #643 exists and perhaps that should be reopened instead. Feel free to close this if that is this the case.

@refack
Copy link
Contributor

refack commented Nov 15, 2018

Thanks for the issues.
I'll follow up.

@refack
Copy link
Contributor

refack commented Nov 15, 2018

So it seems @rvagg already wrote the script for this, it just wasn't turned on (probably because the test suit failed)

@richardlau
Copy link
Member

@refack #643 (comment)

@refack
Copy link
Contributor

refack commented Nov 15, 2018

Job is now ready, and will pass once nodejs/node#24376 land: https://ci.nodejs.org/job/refack-node-test-commit-linux-containered/15/

@danbev
Copy link
Contributor Author

danbev commented Nov 16, 2018

Job is now ready, and will pass once nodejs/node#24376 land: https://ci.nodejs.org/job/refack-node-test-commit-linux-containered/15/

Awesome, thanks!

@rvagg
Copy link
Member

rvagg commented Nov 18, 2018

I honestly don't know why that was never enabled (just requires adding a label to the job iirc), perhaps it was due to too many failures without openssl.

@refack
Copy link
Contributor

refack commented Nov 18, 2018

I can imagine it was because the test suit was failing, and in several places, not just tests, for example calls to npm don't work. But now nodejs/node#24376 should be the last bloker.

@refack
Copy link
Contributor

refack commented Nov 21, 2018

With nodejs/node#24376 landed 3 days ago, PRs should be either fresh, or "rebasable", so IMHO we're ready to enable.
Sanity on master https://ci.nodejs.org/job/refack-node-test-commit-linux-containered/16/

@refack
Copy link
Contributor

refack commented Nov 21, 2018

Oooof, new test fails - test.parallel/test-cli-node-print-help

@danbev
Copy link
Contributor Author

danbev commented Nov 21, 2018

Oooof, new test fails - [test.parallel/test-cli-node-print-help](https://ci.nodejs.org/job/refack-

I just ran into this. I can take a look at if and create a PR for it if you like?

@refack
Copy link
Contributor

refack commented Nov 21, 2018

I just ran into this. I can take a look at if and create a PR for it if you like?

I thought about doing it myself, but be my guest :)

@rvagg
Copy link
Member

rvagg commented Nov 22, 2018

VersionSelectorScript.groovy:

  [ /sharedlibs_withoutssl/,          anyType,     lt(10)  ],

So that's allowing it to run on 10+. Should we be switching that to lt(12) since that change isn't backported?

@richardlau
Copy link
Member

Having this job would allow us to catch things like nodejs/node#26182 (most likely due to a recent eslint update).

@refack
Copy link
Contributor

refack commented Feb 18, 2019

Turned it on https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_withoutssl_x64/10746/ but blocked by nodejs/node#26182

If someone could ping me when it's solved, I'll turn it back on.

@danbev
Copy link
Contributor Author

danbev commented Feb 22, 2019

@refack We have merged nodejs/node#26182 now. Would you be able to turn the job back on? Thanks!

@richardlau
Copy link
Member

Might be worth holding off until the security releases are done just to be on the safe side: #1699

@richardlau
Copy link
Member

Security releases are done. Ping @refack I think we can turn this on now.

@refack
Copy link
Contributor

refack commented Feb 28, 2019

Turned on. Let's see https://ci.nodejs.org/job/node-test-commit-linux-containered/10929/

@refack
Copy link
Contributor

refack commented Feb 28, 2019

https://ci.nodejs.org/job/node-test-commit-linux-containered/10929/nodes=ubuntu1604_sharedlibs_withoutssl_x64/ is ✔️ .
Closing.

@refack refack closed this as completed Feb 28, 2019
@danbev
Copy link
Contributor Author

danbev commented Mar 1, 2019

Awesome, thanks @refack, @rvagg, and @richardlau!

@rvagg rvagg reopened this Mar 1, 2019
@rvagg
Copy link
Member

rvagg commented Mar 1, 2019

failing on v10.x with test-cli-node-print-help still https://ci.nodejs.org/view/All/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_withoutssl_x64/10933/

fix needs to be backported or we need to update jenkins/scripts/VersionSelectorScript.groovy to do lt(11) rather than lt(10)

@danbev
Copy link
Contributor Author

danbev commented Mar 1, 2019

I can take a look at backporting but it won't be until Monday at the earliest I'm afraid.

@rvagg
Copy link
Member

rvagg commented Mar 1, 2019

@danbev ok, so let's leave the VersionSelectorScript with lt(10) for now anticipating getting it fixed early next week. But @BethGriggs will need to be aware of the failing test (test-cli-node-print-help on node-test-commit-linux-containered / withoutssl) that can be ignored while preparing for 10.15.3 in the meantime.

@danbev
Copy link
Contributor Author

danbev commented Mar 4, 2019

@rvagg It looks like the fix for this is already on v10.x-staging. I'm probably missing something obvious here (I'm a little sleep deprived and home with a sick kid today).

@richardlau
Copy link
Member

@rvagg It looks like the fix for this is already on v10.x-staging. I'm probably missing something obvious here (I'm a little sleep deprived and home with a sick kid today).

The build Rod referenced was against v10.15.2. The fix was included in the v10.15.3 release that went out yesterday.

@richardlau
Copy link
Member

There is, however, a failure on v11.x that we're not seeing on neither master nor v10.x: nodejs/node#26480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-change PSA of configuration changes ci-job-request ci-public
Projects
None yet
Development

No branches or pull requests

4 participants