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: default building of master is broken on Windows without NASM #19918

Closed
vsemozhetbyt opened this issue Apr 10, 2018 · 6 comments
Closed
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 10, 2018

https://github.com/nodejs/node/blob/master/BUILDING.md#windows-1 states:

Optional (for OpenSSL assembler modules): the NetWide Assembler, if not installed in the default location it needs to be manually added to PATH.

But vcbuild test just aborts without NASM:

j:\temp\node-master>vcbuild test
Looking for Python 2.x
Looking for NASM
Could not find NASM, it will not be used.

j:\temp\node-master>

See also: https://github.com/nodejs/node/blob/master/BUILDING.md#openssl-asm-support

@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/build @nodejs/platform-windows

@vsemozhetbyt vsemozhetbyt added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Apr 10, 2018
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 10, 2018

Possible ref: eec659c

@rvagg
Copy link
Member

rvagg commented Apr 11, 2018

This is a question/issue for @shigeki and @joaocgreis and comes in as a result of the openssl 1.1.0 upgrade. Perhaps it's simply an matter of removing "Optional" from the docs?

@shigeki
Copy link
Contributor

shigeki commented Apr 11, 2018

I missed to check vcbuild.bat without nasm. It can be fallbacked to openssl_no_asm:1 in configure in case that nasm is not installed. I'm going to fix this.

@bnoordhuis
Copy link
Member

What about making nasm mandatory and adding a no-asm switch to vcbuild.bat (and updating the error message to tell the user about the switch)?

A no-asm build causes pretty steep performance regressions so it should arguably be opt-in rather than quietly falling back on that.

@shigeki
Copy link
Contributor

shigeki commented Apr 11, 2018

I have no preference to fallback automatically with no-asm build or to opt-in.
I made PR for the latter in #19943.

The same issue exist in the assembler version check on UNIX and Mac. I will submit a new issue for it.

bzoz added a commit to JaneaSystems/node that referenced this issue Apr 11, 2018
Adds NASM installation to the Boxstarter script.

Refs: nodejs#19918
jasnell pushed a commit that referenced this issue Apr 14, 2018
Adds NASM installation to the Boxstarter script.

Refs: #19918

PR-URL: #19950
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue Apr 16, 2018
Instead of automatically falling back to openssl_no_asm with warning
if no nasm is found during build on Windows, this stops vcbuild.bat
and requires users to specify openssl_no_asm option explicitly.

Fixes: #19918
PR-URL: #19943
Refs: #19930
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
jasnell pushed a commit that referenced this issue Apr 16, 2018
Adds NASM installation to the Boxstarter script.

Refs: #19918

PR-URL: #19950
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue May 1, 2018
Adds NASM installation to the Boxstarter script.

Refs: nodejs#19918

PR-URL: nodejs#19950
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 17, 2018
Adds NASM installation to the Boxstarter script.

Refs: #19918

PR-URL: #19950
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[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. windows Issues and PRs related to the Windows platform.
Projects
None yet
4 participants