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: add node_use_openssl check to install.py #11766

Closed
wants to merge 2 commits into from
Closed

build: add node_use_openssl check to install.py #11766

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 9, 2017 •
edited
Loading

When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 9, 2017
@danbev
Copy link
Contributor Author

danbev commented Mar 9, 2017

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

@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 9, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 9, 2017

I'm confused about this. Why should we not allow compiling without OpenSSL?

@danbev
Copy link
Contributor Author

danbev commented Mar 9, 2017

I'm confused about this. Why should we not allow compiling without OpenSSL?

Sorry for the confusion. This is not really about comping without OpenSSL but after doing so running make install will copy the OpenSSL headers into the target installation directory which I was not expecting. I know I added two checks to this commit but wanted to get some feedback and perhaps just having a check in tools/install would be enough. Or close this as this might not really effect real use cases (I only noticed because I was testing) but thought I would be worth bringing up at least.

@mscdex
Copy link
Contributor

mscdex commented Mar 9, 2017

Don't the current changes to the configure script prevent users from compiling entirely without OpenSSL though?

@danbev
Copy link
Contributor Author

danbev commented Mar 9, 2017

Don't the current changes to the configure script prevent users from compiling entirely without OpenSSL though?

Yes, the change to configure does force that. How about I remove that check and leave the one in tools/install.py?

@mscdex
Copy link
Contributor

mscdex commented Mar 9, 2017

That sounds good to me. AFAIK that extra check in tools/install.py should be all that's needed.

@danbev
Copy link
Contributor Author

danbev commented Mar 9, 2017

That sounds good to me. AFAIK that extra check in tools/install.py should be all that's needed.

Great, thanks for that and sorry about the confusion!

@danbev
build: add node_use_openssl check to install.py
5de9bc6 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.
@danbev danbev force-pushed the check-without-ssl-and-shared-openssl branch from 1739500 to 5de9bc6 Compare March 10, 2017 06:54
@danbev
Copy link
Contributor Author

danbev commented Mar 10, 2017

Rebased and clarified commit title and message.

@danbev danbev changed the title build: add --without-ssl --shared-openssl check build: add node_use_openssl check to install.py Mar 10, 2017
mscdex
mscdex reviewed Mar 10, 2017
View reviewed changes
configure Outdated
@@ -975,7 +975,6 @@ def configure_openssl(o):
else:
o['variables']['openssl_fips'] = ''


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated whitespace change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that was sloppy of me. Fix it now. Thanks

@danbev
Copy link
Contributor Author

danbev commented Mar 13, 2017

@mscdex Thanks for the review! Would you like to approve this and I can add you to the list of reviewers?

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2017

LGTM

danbev added a commit to danbev/node that referenced this pull request Mar 13, 2017
@danbev
build: add node_use_openssl check to install.py
471aa19 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

PR-URL: nodejs#11766
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Mar 13, 2017

Landed in 471aa19

@danbev danbev closed this Mar 13, 2017
@danbev danbev deleted the check-without-ssl-and-shared-openssl branch March 13, 2017 18:54
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 14, 2017
@danbev @italoacasas
build: add node_use_openssl check to install.py
542a373 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

PR-URL: nodejs#11766
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 14, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@danbev @jungx098
build: add node_use_openssl check to install.py
2e9e972 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

PR-URL: nodejs#11766
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@danbev @MylesBorins
build: add node_use_openssl check to install.py
563c2ce 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

PR-URL: #11766
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@danbev @MylesBorins
build: add node_use_openssl check to install.py
2c2a664 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

PR-URL: #11766
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@danbev @andrew749
build: add node_use_openssl check to install.py
c21c63c 
When configuring --without-ssl and then running make install
openssl headers will be copied from deps/openssl to the target
installation directory.

This commit adds a check for is node_use_openssl is set in which
case the headers are not copied.

PR-URL: nodejs/node#11766
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@cjihrig cjihrig cjihrig approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danbev @mscdex @bnoordhuis @cjihrig @MylesBorins @nodejs-github-bot