-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
LGTM if CI is green. Do we skip the cc tests in that case? |
No, just the inspector cctests. |
That's what I meant to ask 😄 Thanks for the clarification |
LGTM pending CI |
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>
@thealphanerd Apologies, I forgot to add you as a reviewer. |
Yes, thanks, that works with fixing it when v8_inspector is disabled. But what about the case when it's enabled, and Would wrapping all those deps inside of |
Virtual shrug. Of course it could be made to work but since |
@bnoordhuis nbd. I've started using https://github.com/evanlucas/core-get-reviewers to simplify things for myself, might be worth checking out. 👍 |
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>
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/