build: require --openssl-no-asm if no suitable assemble on x86_64 and ia32 #20226

shigeki · 2018-04-23T15:01:44Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
[] tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This is a copy of #20217 (review) and add an additional commit to limit assembler check only on x86_64 and ia32 and BUILDING.md updates.

I submit a new PR with fast-track for only 5 hours left to land semver-major on Node-v10.0.0 until PST noon as described in #20217 (comment).

Fixes: #19944
Refs: #20217

@rvagg @mhdawson @danbev @jasnell @bnoordhuis @nodejs/build @nodejs/tsc

The current openssl checks assembler version only x86_64 or ia32 target arch for use of AES-NI, AVX and AVX2. This requires --openssl-no-asm option during configure when an older assembler version is found only on x86_64 or ia32.

shigeki · 2018-04-23T15:04:42Z

I made CI in https://ci.nodejs.org/job/node-test-commit/17948/ and run this again in https://ci.nodejs.org/job/node-test-pull-request/14442/.

vsemozhetbyt · 2018-04-23T15:26:58Z

BUILDING.md


 * gas (GNU assembler) version 2.23 or higher
 * xcode version 5.0 or higher
 * llvm version 3.3 or higher
 * nasm version 2.10 or higher in Windows

-Otherwise, `--openssl-no-asm` is added with warning in configure.
+Otherwise, configure is failed. Install one or build with --openssl-no-asm.


configure -> `configure`?
--openssl-no-asm -> `--openssl-no-asm`?

@vsemozhetbyt Thanks and very sorry for I often missed backquotes. Fixed in 0388dbd.

It is OK, cryptography domain is complicated enough to not also track all the doc nits)

minor nit: 'Otherwise configure will fail with an error. This can be avoided by either providing a new enough assembler as per the list above or by using the --openssl-no-asm flag.'

jasnell · 2018-04-23T15:28:51Z

@nodejs/tsc ... need eyes on this today. It needs to land by noon pacific if it's going to make it in to 10.0.0

@rvagg @shigeki ... can you summarize a bit why this needs to be fast tracked? Why is is critical this gets into 10.0.0?

shigeki · 2018-04-23T15:50:36Z

can you summarize a bit why this needs to be fast tracked? Why is is critical this gets into 10.0.0?

This is my summary.

OpenSSL requires new assembler version to support hardware support for encryption/decryption (AES-NI, AVX and AVX2) on x86_64 and ia32 as https://github.com/nodejs/node/blob/master/BUILDING.md#openssl-asm-support.
The current configure just shows warning and fallback to built with --openssl-no-asm if it does not meet requirements above.
This change is made according to the comment of @rvagg in build: require --openssl-no-asm if no suitable assembler #20217 (comment)

The main behaviour change here is to fail ./configure if you don't have an assembler new enough, which forces you to supply --openssl-no-asm rather than doing it implicitly. The cost of no-asm is that you get binaries that are potentially a lot slower, it's not something you should walk blindly in to. Discussion around this is primarily in #19944

I'm not sure how much user are affected this. I think we can save them with adding some fallbacks after 10.0.0 release if users require it but this semver-major change can be landed only at this time of Node-v10.0.0.

vsemozhetbyt · 2018-04-23T15:54:33Z

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

ofrobots

It is indeed debatable if this is semver major. Fine to get into 10.0.0 IMO.

ofrobots · 2018-04-23T17:03:22Z

BUILDING.md


 * gas (GNU assembler) version 2.23 or higher
 * xcode version 5.0 or higher
 * llvm version 3.3 or higher
 * nasm version 2.10 or higher in Windows

-Otherwise, `--openssl-no-asm` is added with warning in configure.
+Otherwise, configure is failed. Install one or build with --openssl-no-asm.


minor nit: 'Otherwise configure will fail with an error. This can be avoided by either providing a new enough assembler as per the list above or by using the --openssl-no-asm flag.'

shigeki · 2018-04-23T17:18:48Z

@ofrobots I follow your comment to the doc for I'm not native English speaker. Fixed in e885c42. Thanks.

MylesBorins

LGTM. This only touches the compilation tool chain and ensures that people are not going to be broken when building.

I think this should be Semver-Patch imho. The breaking change was in the OpenSSL upgrade, this is a bugfix

MylesBorins · 2018-04-23T17:27:13Z

BUILDING.md


 * gas (GNU assembler) version 2.23 or higher
 * xcode version 5.0 or higher
 * llvm version 3.3 or higher
 * nasm version 2.10 or higher in Windows

-Otherwise, `--openssl-no-asm` is added with warning in configure.
+Otherwise `configure` will fail with an error. This can be avoided by
+either providing a new enough assembler as per the list above or by


nit: s/new enough assembler/a newer

Fixed in 36becf8. Thanks.

MylesBorins · 2018-04-23T17:28:04Z

configure

-    sys.exit()
+       Please make sure you have a C compiler installed on your system and/or
+       consider adjusting the CC environment variable if you installed
+       it in a non-standard prefix.''')


did we want to remove the sys.exit() here

MylesBorins · 2018-04-23T17:28:25Z

configure

-    sys.exit()
+       Please make sure you have a C compiler installed on your system and/or
+       consider adjusting the CC environment variable if you installed
+       it in a non-standard prefix.''')


same as above re: sys.exit

MylesBorins · 2018-04-23T17:28:37Z

configure

-    sys.exit()
+       Please make sure you have a C compiler installed on your system and/or
+       consider adjusting the CC environment variable if you installed
+       it in a non-standard prefix.''')


same here re: sys.exit()

@MylesBorins This is in def error(msg) and sys.exit(1) is included in it.

thanks for explaining 😄

shigeki · 2018-04-23T17:48:30Z

I removed semver-major tag to follow the comments.

jasnell · 2018-04-23T17:51:19Z

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

shigeki · 2018-04-23T17:52:56Z

Oops, I did it https://ci.nodejs.org/job/node-test-pull-request/14453/ and just cancelled it.

jasnell · 2018-04-23T18:20:02Z

There was some red on linux... appears to be a flaky test, trying again https://ci.nodejs.org/job/node-test-commit-linux/18222/

jasnell · 2018-04-23T18:39:58Z

Ok, things look good between the two CI runs. There are still a couple running on one but they passed on the others and the red that is remaining is known. Will be landing this and will be doing another CI run on 10.0.0 just to make certain.

shigeki · 2018-04-23T18:44:02Z

Yes. The failures on alpine linux are due to its timezone problem and I think it is nothing to do with this PR.

PR-URL: #20226 Fixes: #19944 Refs: #20217 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

The current openssl checks assembler version only x86_64 or ia32 target arch for use of AES-NI, AVX and AVX2. This requires --openssl-no-asm option during configure when an older assembler version is found only on x86_64 or ia32. PR-URL: #20226 Fixes: #19944 Refs: #20217 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

PR-URL: #20226 Fixes: #19944 Refs: #20217 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

The current openssl checks assembler version only x86_64 or ia32 target arch for use of AES-NI, AVX and AVX2. This requires --openssl-no-asm option during configure when an older assembler version is found only on x86_64 or ia32. PR-URL: #20226 Fixes: #19944 Refs: #20217 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

jasnell · 2018-04-23T18:46:04Z

Landed in 65db8c7, de968999, and 3bcd857

shigeki · 2018-04-23T18:48:56Z

Thanks. Happy release of Node-v10.0.0!

rvagg · 2018-04-23T21:57:57Z

thanks @shigeki! sometimes I wonder if you even sleep.

rvagg and others added 3 commits April 23, 2018 23:25

build: extract error() function in configure

7e77b4d

build: require --openssl-no-asm if old assembler

9ed2a04

build: limit assembler version check on x86

35b3fca

The current openssl checks assembler version only x86_64 or ia32 target arch for use of AES-NI, AVX and AVX2. This requires --openssl-no-asm option during configure when an older assembler version is found only on x86_64 or ia32.

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Apr 23, 2018

shigeki mentioned this pull request Apr 23, 2018

build: require --openssl-no-asm if no suitable assembler #20217

Closed

mscdex added this to the 10.0.0 milestone Apr 23, 2018

vsemozhetbyt reviewed Apr 23, 2018

View reviewed changes

fix according to the comment from vsemozhetbyt

0388dbd

jasnell approved these changes Apr 23, 2018

View reviewed changes

ofrobots approved these changes Apr 23, 2018

View reviewed changes

fix doc according to the comment of ofrobots

e885c42

MylesBorins approved these changes Apr 23, 2018

View reviewed changes

fix the doc according to the comment of MylesBorins

36becf8

shigeki removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 23, 2018

richardlau approved these changes Apr 23, 2018

View reviewed changes

MylesBorins added the dont-land-on-v4.x label Apr 23, 2018

MylesBorins added dont-land-on-v6.x labels Apr 23, 2018

jasnell closed this Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: require --openssl-no-asm if no suitable assemble on x86_64 and ia32 #20226

build: require --openssl-no-asm if no suitable assemble on x86_64 and ia32 #20226

shigeki commented Apr 23, 2018

shigeki commented Apr 23, 2018

vsemozhetbyt Apr 23, 2018 •

edited

Loading

shigeki Apr 23, 2018

vsemozhetbyt Apr 23, 2018

ofrobots Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

vsemozhetbyt commented Apr 23, 2018

ofrobots left a comment

ofrobots Apr 23, 2018

shigeki commented Apr 23, 2018

MylesBorins left a comment

MylesBorins Apr 23, 2018

shigeki Apr 23, 2018

MylesBorins Apr 23, 2018

MylesBorins Apr 23, 2018

MylesBorins Apr 23, 2018

shigeki Apr 23, 2018

MylesBorins Apr 23, 2018

shigeki commented Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

jasnell commented Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

rvagg commented Apr 23, 2018

build: require --openssl-no-asm if no suitable assemble on x86_64 and ia32 #20226

build: require --openssl-no-asm if no suitable assemble on x86_64 and ia32 #20226

Conversation

shigeki commented Apr 23, 2018

Checklist

shigeki commented Apr 23, 2018

vsemozhetbyt Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

vsemozhetbyt commented Apr 23, 2018

ofrobots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Apr 23, 2018

MylesBorins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

jasnell commented Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

jasnell commented Apr 23, 2018

shigeki commented Apr 23, 2018

rvagg commented Apr 23, 2018

vsemozhetbyt Apr 23, 2018 •

edited

Loading