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 openssl-system-ca-path to windows #17066

Closed
wants to merge 4 commits into from
Closed

build: add openssl-system-ca-path to windows #17066

wants to merge 4 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 16, 2017 •
edited by refack
Loading

This commit adds the openssl-system-ca-path configuration variable to
the windows build system.

C:\node>vcbuild.bat /?
vcbuild.bat [debug/release] [msi]
...
[openssl-system-ca-path path-to-system-certs] [clean]
Examples:
  vcbuild.bat                              : builds release build
  ...
  vcbuild.bat openssl-system-ca-path path  : builds using the specified
  path to system CA (PEM format) in addition to the OpenSSL supplied
  CA store or compiled-in Mozilla CA copy
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

cc @nodejs/build @nodejs/platform-windows

[refack edited checklist formatting (since GitHub parses it)]

@danbev
build: add openssl-system-ca-path to windows
2b8a6dd 
This commit adds the openssl-system-ca-path configuration variable to
the windows build system.

C:\node>vcbuild.bat /?
vcbuild.bat [debug/release] [msi]
...
[openssl-system-ca-path path-to-system-certs] [clean]
Examples:
  vcbuild.bat                              : builds release build
  ...
  vcbuild.bat openssl-system-ca-path path  : builds using the specified
  path to system CA (PEM format) in addition to the OpenSSL supplied
  CA store or compiled-in Mozilla CA copy
@danbev
squash: remove line continuation for if statement
f8a58d4
@danbev
squash: remove ^ when setting configure_flags
da5bd95
@danbev
squash: update the options description
aa4cb55
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Nov 16, 2017
@danbev
Copy link
Contributor Author

danbev commented Nov 16, 2017

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

bnoordhuis
bnoordhuis reviewed Nov 16, 2017
View reviewed changes
vcbuild.bat Outdated
@@ -115,6 +116,8 @@ if /i "%1"=="no-NODE-OPTIONS" set no_NODE_OPTIONS=1&goto arg-ok
if /i "%1"=="debug-http2" set debug_http2=1&goto arg-ok
if /i "%1"=="debug-nghttp2" set debug_nghttp2=1&goto arg-ok
if /i "%1"=="link-module" set "link_module= --link-module=%2%link_module%"&goto arg-ok-2
if /i "%1"=="openssl-system-ca-path" ^
set "openssl_system_ca_path= --openssl-system-ca-path=%2%"&goto arg-ok-2
Copy link
Member

Choose a reason for hiding this comment

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

Indent here and below?

vcbuild.bat Outdated
echo vcbuild.bat lint : runs the C++ and JavaScript linter
echo vcbuild.bat openssl-system-ca-path path : builds using the specified path ^
to system CA (PEM format) in addition to the OpenSSL supplied CA store or ^
compiled-in Mozilla CA copy
Copy link
Member

Choose a reason for hiding this comment

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

It's just the Mozilla CA store. OpenSSL doesn't provide certs, it only validates them. =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, I'll update the description. Thanks!

jasnell
jasnell approved these changes Nov 22, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Ping @bnoordhuis ... PTAL

@refack
Copy link
Contributor

refack commented Nov 22, 2017

Again Can I ask for motivation. Is this a common case? If not I would rather we leave it out of vcbuild since it seems like the alternative is trivial:

python configure --openssl-system-ca-path=PATH
vcbuild noprojgen

@refack
Copy link
Contributor

refack commented Nov 22, 2017

Alternatively we can implement a generic arg parser, that every CLI arg that starts with -- is appended to configure_flags

@danbev
Copy link
Contributor Author

danbev commented Nov 22, 2017

Again Can I ask for motivation. Is this a common case?

Sorry, I must have missed your first request for motivation this. The motivation is because I thought it should be added to match the options available on Unix-like systems. If doing python configure is something users on windows are comfortable with and use, then we can certainly close this. I'm not a windows users and was not aware of that way of building.

@gibfahn
Copy link
Member

gibfahn commented Nov 23, 2017

Sorry, I must have missed your first request for motivation this. The motivation is because I thought it should be added to match the options available on Unix-like systems.

Is there a make option to set openssl-system-ca-path? In Unix we just run ./configure or python configure with the relevant options (or set CONFIG_FLAGS), can we not just do that in vcbuild as well?

@refack why do we have to duplicate all the configure options in vcbuild again? Can't windows users just run configure first? If it's because vcbuild wants to call configure again, how about we have python parse the CONFIG_FLAGS env var, instead of having Makefile pass it as a command line arg? That should work on Windows too I assume.

@refack
Copy link
Contributor

refack commented Nov 25, 2017

why do we have to duplicate all the configure options in vcbuild again?

I don't think we should. As I see it vcbuild args are shortcuts for the more common build setups. I don't feel we need to wrap all possible configuration options.
Besides being able to run configure separately from vcbuild noprojgen, I was just reminded that we support an environment variable %config_flags% which is passed to configure verbatim.
But we might need to document both of these options.

refack
refack suggested changes Nov 25, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

I'm not convinced we need to mirror all of configure's options.

@gibfahn
Copy link
Member

gibfahn commented Nov 26, 2017

I was just reminded that we support an environment variable %config_flags% which is passed to configure verbatim.

So you can do set "CONFIG_FLAGS=--openssl-system-ca-path=C:\path\to\ca %CONFIG_FLAGS%"; vcbuild or configure --openssl-system-ca-path=C:\path\to\ca; vcbuild to get the equivalent to this PR? Seems like a better way to go (especially as that's how the Makefile works).

But we might need to document both of these options.

Definitely.

@danbev
Copy link
Contributor Author

danbev commented Nov 26, 2017

Closing this in preferens of the options give above. Thanks everyone for the feedback/comments!

@danbev danbev closed this Nov 26, 2017
@refack refack mentioned this pull request Nov 26, 2017
Closed
@danbev danbev deleted the windows-system-ca-path-option branch November 27, 2017 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack requested changes

@jasnell jasnell jasnell 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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@danbev @refack @gibfahn @bnoordhuis @jasnell @nodejs-github-bot