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: add checks for openssl configure options #12175

Closed
wants to merge 4 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 3, 2017

Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl
$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm
$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 3, 2017
@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Apr 3, 2017
@vsemozhetbyt
Copy link
Contributor

configure Outdated
return
configure_library('openssl', o)

def withoutSSLError(option):
print('Error: --without-ssl is incompatible with %s' % option)
exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Style: without_ssl_error and no semicolon.

You could define it in the if options.without_ssl: block if that's the only place you use it (or simply use or to chain the conditions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've fixed these issues now.

@danbev
Copy link
Contributor Author

danbev commented Apr 3, 2017

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a nit.

configure Outdated
return
configure_library('openssl', o)



Copy link
Member

Choose a reason for hiding this comment

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

Whitespace change. :-)

danbev added a commit to danbev/node that referenced this pull request Apr 5, 2017
Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips

PR-URL: nodejs#12175
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Apr 5, 2017

Landed in 1f74b9f

@danbev danbev closed this Apr 5, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips

PR-URL: nodejs#12175
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips

PR-URL: #12175
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@MylesBorins
Copy link
Contributor

landed on v6.x assuming the options are relevant. LMK if it should be backported

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips

PR-URL: #12175
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
@danbev danbev deleted the openssl-shared-without-check branch June 28, 2017 05:43
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Currently it is possible to configure using --without-ssl and
--shared-openssl/--openssl-no-asm/openssl-fips without an error
occuring.

The commit add check for these combinations:

$ ./configure --without-ssl --shared-openssl
Error: --without-ssl is incompatible with --shared-openssl

$ ./configure --without-ssl --openssl-no-asm
Error: --without-ssl is incompatible with --openssl-no-asm

$ ./configure --without-ssl --openssl-fips=dummy
Error: --without-ssl is incompatible with --openssl-fips

PR-URL: nodejs/node#12175
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
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. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants