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

build: drop unconditional openssl dep from cctest #7486

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jun 29, 2016

Don't link in openssl when building ./configure --without-inspector,
it's only used by the inspector cctests. Ditto libuv and http_parser.

Fixes: #7478

CI: https://ci.nodejs.org/job/node-test-pull-request/3131/

@bnoordhuis bnoordhuis added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. labels Jun 29, 2016
@MylesBorins
Copy link
Contributor

LGTM if CI is green. Do we skip the cc tests in that case?

@bnoordhuis
Copy link
Member Author

No, just the inspector cctests.

@MylesBorins
Copy link
Contributor

That's what I meant to ask 😄

Thanks for the clarification

@cjihrig
Copy link
Contributor

cjihrig commented Jun 29, 2016

LGTM pending CI

@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Jun 29, 2016
@jasnell
Copy link
Member

jasnell commented Jun 29, 2016

LGTM

Don't link in openssl when building `./configure --without-inspector`,
it's only used by the inspector cctests.  Ditto libuv and http_parser.

Fixes unnecessarily building openssl when `--shared-openssl` is also
passed to configure.

Fixes: nodejs#7478
PR-URL: nodejs#7486
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis closed this Jun 30, 2016
@bnoordhuis bnoordhuis deleted the fix7478 branch June 30, 2016 11:33
@bnoordhuis bnoordhuis merged commit 7dcc4af into nodejs:master Jun 30, 2016
@bnoordhuis
Copy link
Member Author

@thealphanerd Apologies, I forgot to add you as a reviewer.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 30, 2016

Yes, thanks, that works with fixing it when v8_inspector is disabled.

But what about the case when it's enabled, and --shared-openssl is also enabled? It still builds bundled openssl for some reason. Same for other deps.

Would wrapping all those deps inside ofv8_inspector condition in additional idividual conditions (like 841b434 does for openssl) break anything?

@bnoordhuis
Copy link
Member Author

Virtual shrug. Of course it could be made to work but since --shared-openssl is primarily intended for embedders, I don't really care much because I doubt they would care much.

@MylesBorins
Copy link
Contributor

@bnoordhuis nbd. I've started using https://github.com/evanlucas/core-get-reviewers to simplify things for myself, might be worth checking out. 👍

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Don't link in openssl when building `./configure --without-inspector`,
it's only used by the inspector cctests.  Ditto libuv and http_parser.

Fixes unnecessarily building openssl when `--shared-openssl` is also
passed to configure.

Fixes: #7478
PR-URL: #7486
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@MylesBorins MylesBorins added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. inspector Issues and PRs related to the V8 inspector protocol openssl Issues and PRs related to the OpenSSL dependency. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants