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.

Sign up for GitHub

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

Jump to bottom

build: --shared-openssl still builds openssl #7478

Closed
ChALkeR opened this issue Jun 29, 2016 · 4 comments · Fixed by #7569
Closed

build: --shared-openssl still builds openssl #7478

ChALkeR opened this issue Jun 29, 2016 · 4 comments · Fixed by #7569
Assignees
@ofrobots
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. question Issues that look for answers.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Jun 29, 2016 •
edited
Loading

Since 84ad31f (#6792), see changes in node.gyp.

Note that even --without-inspector does not revert this behaviour.

Is that the desired behaviour?

/cc @ofrobots @jasnell @bnoordhuis
@ChALkeR ChALkeR added question Issues that look for answers. build Issues and PRs related to build files or the CI. labels Jun 29, 2016
@ChALkeR ChALkeR mentioned this issue Jun 29, 2016
Closed
@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Jun 29, 2016
@ofrobots ofrobots self-assigned this Jun 29, 2016
ofrobots added a commit to ofrobots/node that referenced this issue Jun 29, 2016
@ofrobots
build: make cctest work with --shared-openssl
841b434 
A new cctest added for v8_inspector was requiring the bundled openssl
unconditionally.

Fixes: nodejs#7478
Ref: nodejs#6792
@ofrobots
Copy link
Contributor

@ChALkeR Can you try out this fix: 841b434. In my build environment, the system provided openssl is not compatible with node, so I am trying find a build environment to check this in.

@bnoordhuis
Copy link
Member

It builds the bundled openssl because it's linked into cctest. Seems expected and reasonable to me.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 29, 2016 •
edited
Loading

@ofrobots Thanks! Will try soon. I'm building on Arch Linux, it has openssl 1.0.2.h.
Btw, what will happen with --shared-libuv and/or --shared-http-parser? Those

@bnoordhuis It's strange to me that:

  1. --shared-openssl builds bundled openssl.
  2. That change was introduced in «v8-inspector support».
  3. --without-inspector does not revert it.

I mean that each one of those is unexpected, not just the combination of all three.

Btw, 84ad31f does not even build with --without-inspector (with or without --shared-openssl), but that looks fixed in master. Upd: it was fixed in 3e7c5bc (#7078).

@bnoordhuis bnoordhuis mentioned this issue Jun 29, 2016
Merged
@bnoordhuis
Copy link
Member

#7486

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jun 30, 2016
@bnoordhuis
build: drop unconditional openssl dep from cctest
7dcc4af 
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>
Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
@bnoordhuis @Fishrock123
build: drop unconditional openssl dep from cctest
1bd6a62 
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>
This was referenced Jul 6, 2016
Merged
Closed
ChALkeR added a commit to ChALkeR/io.js that referenced this issue Jul 9, 2016
@ChALkeR
build: respect --shared-* flags for inspector deps
990c968 
Don't build openssl/http_parser/libuv for v8_inspector if corresponding
--shared-* flags were passed to the ./configure script.

Fixes: nodejs#7478
Fixes: nodejs#7583
PR-URL: nodejs#7569
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
evanlucas pushed a commit that referenced this issue Jul 13, 2016
@ChALkeR @evanlucas
build: respect --shared-* flags for inspector deps
933ff62 
Don't build openssl/http_parser/libuv for v8_inspector if corresponding
--shared-* flags were passed to the ./configure script.

Fixes: #7478
Fixes: #7583
PR-URL: #7569
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ofrobots ofrobots

Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. question Issues that look for answers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

build: respect --shared-* flags for inspector deps ChALkeR/io.js
4 participants
@mscdex @ofrobots @bnoordhuis @ChALkeR