add nasm support for msvc #87

KaneGreen · 2021-03-16T12:34:32Z

issue #59

For Windows users, if the OPENSSL_RUST_USE_NASM environment variable is set, the assembly language routines will be enabled.

Tested on Windows 10 64bit (20H2), VS Community 2019 (16.9.1), rustc 1.50.0 (stable-x86_64-pc-windows-msvc), NASM version 2.15.05.

alexcrichton · 2021-03-16T14:13:41Z

Thanks! Can this be tested on CI? (both with and without nasm ideally)

Additionally I think the logic is inverted? It looks like if the env var is unset then this panics? I think it's fine to have the env var to forcibly disable or forcibly enable, but otherwise detecting nasm at build time seems like the best choice.

KaneGreen · 2021-03-16T15:20:58Z

I tested this on my local machine, not a CI.

This logic is a bit confusing. According to my understanding and test results, it works like this:
function is_nasm_ready() return value:

	env is set	env not set or =0
nasm is installed	true ✔	false ❌
nasm not installed	panic! 💥	false ❌

In other words, by default the env var is not set, so nasm is disabled. When the env var is set to a non-zero value, it detects nasm (and panic when nasm is not installed because this env var conflicts with the absence of nasm)

alexcrichton · 2021-03-16T20:59:04Z

Oh oops I never know how to read map_or, so I got mixed up.

I still think, though, that maybe an env var isn't worth it here? Could nasm.exe just be detected and we enable it whether it's installed or not?

KaneGreen · 2021-03-17T03:08:57Z

New version:

the env var changed to OPENSSL_RUST_NO_NASM, which disable the asm compiling.
the default behavior is auto-detecting.

function is_nasm_ready() return value:

	env var is set	env var not set or =0
nasm is installed	false ❌	true ✔
nasm not installed	false ❌	false ❌

In other words, by default the env var is not set, nasm.exe will be auto-detected. But user can use the OPENSSL_RUST_NO_NASM env var to disable it anyway.

I think the env var is necessary. It is a switch to control whether to use asm compiling, for it is not easy to temporarily exclude a binary from the PATH.
What will happen if we just detect nasm.exe without noticing users? Considering there are two people using build environments with the same configuration except that one installed nasm.exe but the other does not, they use the same command to build the same code -- but they get two different binaries (I am not referring to the difference in very details, but to the significant difference caused by the difference in the code/objects).
So, I think the env var is worth.

alexcrichton · 2021-03-17T19:14:20Z

Looks good to me! Can you ensure that this is tested on CI?

KaneGreen · 2021-03-18T03:32:43Z

CI Test added. Now, the windows test on GitHub Actions will take about 25 min.
On my fork, it passed. And on my local windows PC, the build process works well.
Now, CI here is also passed.

alexcrichton

Thanks! In addition to what's below could the env var being set to 1 assert that nasm.exe is used? That way if CI is misconfigured we'll catch it early.

alexcrichton · 2021-03-18T14:05:42Z

.github/workflows/main.yml

+        cargo test --manifest-path testcrate/Cargo.toml --target ${{ matrix.target }} --release -vv
+        cargo run --release --target ${{ matrix.target }} --manifest-path testcrate/Cargo.toml --features package -vv
+      if: startsWith(matrix.os, 'windows') && startsWith(matrix.target, 'x86_64')
+      name: Run tests with nasm (Windows x86_64)


Instead of being subsequent steps could these be two new matrix entries to allow them to run in parallel as well?

KaneGreen · 2021-03-18T16:52:43Z

Well, there so many tests about windows. 32bit/64bit, crt static yes/no, nasm none/installed/env-var -- 2*2*3 == 12 tests. Anyway,these tests passed.

alexcrichton · 2021-03-18T23:43:21Z

Er well I don't think that the full matrix really needs to be tested. I think it's fine to just add a single x86_64 builder which enables nasm.

KaneGreen · 2021-03-19T01:37:32Z

In this commit, I reduced the number of tests. Maybe only one left for nasm.exe may be too few.
In this version, we have 4 new tests for nasm.exe (comparing to e634b86)

	system	crt_static	nasm_exe	env_no_nasm	remark
1	i686		installed		to test wheter it builds on win32
2	x86_64		installed		to test wheter it builds on win64
3	x86_64		installed	set	to test wheter the env var works
4	x86_64	yes	installed		to test wheter it builds with crt_static

KaneGreen · 2021-03-19T11:16:17Z

This force push makes all previous commits as one commit.

alexcrichton · 2021-03-19T16:21:09Z

Thanks for that but I'm not super interested in adding tons of CI configuration just for this use case, it's a lot of goop to maintain. I think that all that needs to be added is a singular new builder which has a singular extra step of downloading nasm. I then think that the env var, when set to 1 (or nonzero basically), would panic the build script of nasm.exe isn't detected. This new CI builder would enable the env var which guarantees that nasm is used and that it builds/works.

KaneGreen · 2021-03-20T00:57:15Z

I'll reduce the number of CI tests.

However, I've changed the env var and now it won't panic. See #87 (comment) for details.

About the env var, the two methods:

Which would you like?

This will automatically detect whether nasm.exe is installed and try to enable the assembly language routines. These can also be disabled by set the `OPENSSL_RUST_NO_NASM` environment variable to a non-zero value.

KaneGreen · 2021-03-20T02:08:53Z

.github/workflows/main.yml

+      if: startsWith(matrix.os, 'windows')
+      run: |
+        if [ '${{ matrix.nasm_exe }}' = 'installed' ] ; then
+        export PATH=$PWD/nasm_win64:$PATH


Why do I add nasm to PATH in this step instead of in Download nasm.exe?
Because using echo PATH=$PWD/xxxxxx:$PATH >> $GITHUB_ENV in previous step will cause an error when configuring.

For adding to PATH you'll want to use a different command -- https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#adding-a-system-path

alexcrichton · 2021-03-22T14:29:30Z

.github/workflows/main.yml

-      name: Run tests (not Windows)
-    - run: |
+    - name: Download nasm.exe (Windows)
+      if: startsWith(matrix.os, 'windows') && matrix.nasm_exe == 'installed'


I think you can remove the windows check here sce nasm_exe is only set for windows builders.

alexcrichton · 2021-03-22T14:30:44Z

.github/workflows/main.yml

+      run: |
+        # if the nasm.exe is disabled, there should be 'no-asm' in the configure command
+        echo 'CI: In output files, there should be no-asm configured:'
+        grep 'no-asm' target/${{ matrix.target }}/*/build/*/output


I would prefer to avoid doing this since it's an unstable implementation detail of Cargo. As I've mentioned before I think the build script should panic if nasm is explicitly requested but it's not found.

I see this output file path in the cargo book. So, I thought it is a stable feature. Now, I'll remove this.
However, on how to switch nasm.exe on and off, do you mean that we should use the OPENSSL_RUST_USE_NASM way?

KaneGreen · 2021-03-23T09:39:23Z

In this version, I set the environment variable back to OPENSSL_RUST_USE_NASM. But its behavior is changed:

function is_nasm_ready() return value:

env var value	`yes`	`auto` or not set	`no`
nasm is installed	true ✔	(auto-detect) true ✔	false ❌
nasm not installed	panic! 💥	(auto-detect) false ❌	false ❌

This information is also updated in README.md.

About the CI tests:
Now, there is only one test abuot nasm: it only tests that the nasm works with this build process.
I didn't test the env var for the following reason.

I think the code for the OPENSSL_RUST_USE_NASM env var is simple enough.
I've tested these is_nasm_ready() and check_env_var() functions several times in my local Windows PC.
Testing the OPENSSL_RUST_USE_NASM env var needs more jobs in CI. And CI scripts will become more complicated.

About the panic when nasm not installed and the env var is set to yes:
This panic is not caused by my code. It is caused by the ./Configure not exit with code 0, which satisfies this requirement:

As I've mentioned before I think the build script should panic if nasm is explicitly requested but it's not found.
(source)

alexcrichton · 2021-03-23T15:37:21Z

src/lib.rs

+                .to_str()
+                .expect("The environment variable has invalid Unicode")
+                .to_lowercase();
+            match &tmp[..] {


I don't think there's any need to go all out here, let's just check for either 0 or 1

Well, there are there situations we need consider: force-enable, force-disable and auto-detecting. In this case, I think 0 or 1 is not suitable. I think 'yes/y', 'no/n', 'auto/a' will be better. (And leaving it unset means auto) (I'll remove those 'true'/'false', whatever)
What do you think?

I personally don't really want to maintain an env var which is so meticulously parsed, I think a simple 0/1 is all we need. This isn't really going to be heavily used enough to warrant so much to handle it.

alexcrichton · 2021-03-23T15:37:35Z

.github/workflows/main.yml

+        WINNASMVERSION='2.15.05'
+        curl -O https://www.nasm.us/pub/nasm/releasebuilds/${WINNASMVERSION}/win64/nasm-${WINNASMVERSION}-win64.zip
+        unzip nasm-${WINNASMVERSION}-win64.zip
+        echo "$PWD/nasm-${WINNASMVERSION}" >> $GITHUB_PATH


Could this step also set OPENSSL_RUST_USE_NASM?

Leaving it unset means auto-detecting. Adding this makes it works better in CI -- if some one don't install nasm correctly, the panic will let us know. I'll do this in later commit.

alexcrichton · 2021-03-23T15:37:58Z

src/lib.rs

+            })
+    }
+
+    #[cfg(not(windows))]


I think the #[cfg] here can be avoided since nothing is windows-specific in that it only compiles on Windows.

I don't think these bothers. I think this notices us that we don't consider running nasm.exe on a non-windows os. (Even though nasm has non-windows version. However, according to OpenSSL's docs, it doesn't need nasm to build on non-windows platforms.)

KaneGreen · 2021-03-25T02:39:23Z

In this version, the acceptable values of the environment variable OPENSSL_RUST_USE_NASM is changed:

function is_nasm_ready() return value:

env var value	`1`	not set	`0`
nasm is installed	true ✔	(auto-detect) true ✔	false ❌
nasm not installed	panic! 💥	(auto-detect) false ❌	false ❌

This information is also updated in README.md.

KaneGreen · 2021-03-25T15:14:37Z

Thank you for considering merging this PR. And thank you for your suggestions during this process.

* only build necessary target (alexcrichton#43) * Bump to 111.6.1+1.1.1d * Update checkout actions reference * Fixed apparent typo in the readme (alexcrichton#46) * Work around upstream cargo issues * Add handling for target riscv64gc-unknown-linux-gnu (alexcrichton#48) * Disable asmjs for now * Allow to specify custom path for Perl (alexcrichton#49) * Allow to specify custom path for Perl Env var OPENSSL_SRC_PERL can be set to specify a path to the perl binary to use to call the openssl perl scripts. Resolves alexcrichton#45 * Fallback to PERL env var if OPENSSL_SRC_PERL is not set * Add support for non-x86 archs on FreeBSD (alexcrichton#50) Caused by: process didn't exit successfully: `.../rust-bootstrap/work-aarch64/rustc-1.40.0-src/build/x86_64-unknown-freebsd/stage1-tools/release/build/openssl-sys-4b2d35028bf73953/build-script-main` (exit code: 101) --- stdout cargo:rustc-cfg=const_fn --- stderr thread 'main' panicked at 'don't know how to configure OpenSSL for aarch64-unknown-freebsd', .../rust-bootstrap/work-aarch64/rustc-1.40.0-src/vendor/openssl-src/src/lib.rs:178:18 * Update CI installation of Rust on macos~ * support for illumos systems (alexcrichton#52) Resolves alexcrichton#51 * More comment out of asmjs * Use gmake by default on all DragonFlyBSD, FreeBSD or Solaris targets (alexcrichton#53) * Bump to 1.1.1e (alexcrichton#54) * Add support for illumos triple (alexcrichton#56) * Bump to 1.1.1f (alexcrichton#58) * Release 111.8.1+1.1.1f * Bump to 1.1.1g * Do not overwrite AR and RANLIB env vars if set (alexcrichton#62) * Add engine support for linux-gnu (alexcrichton#63) * add engine support for linux-gnu Signed-off-by: Xintao <hunterlxt@live.com> * address comment Signed-off-by: Xintao <hunterlxt@live.com> * add blacklist os Signed-off-by: Xintao <hunterlxt@live.com> * add blacklist os Signed-off-by: Xintao <hunterlxt@live.com> * To check CI build info Signed-off-by: Xintao <hunterlxt@live.com> * add comments Signed-off-by: Xintao <hunterlxt@live.com> * Bump to 111.10.0+1.1.1g * Fix build on macOS with latest cc crate (alexcrichton#67) cc v1.0.58 broke the macOS build by including the "-arch" flag in the default set of compiler flags. Strip it out, like we do for iOS targets. Closes alexcrichton#66. * Bump to 111.10.1+1.1.1g * add optional features for less used algorithms (alexcrichton#68) This commit disables by default a few of the weaker cryptographical algorithms into a "weak-crypto" feature as well as some of the less used algorithms into their own specific features. These algorithms are not directly exposed through the rust-openssl crate. The compilation of these can be re-enabled by selecting the desired features. This should slightly reduce build time and library size. Signed-off-by: Petre Eftime <petre.eftime@gmail.com> * Bump to 111.10.2+1.1.1g * Bump to 1.1.1h (alexcrichton#73) * Add FreeBSD powerpc64le support (alexcrichton#75) FreeBSD PowerPC64LE is a new target. This is needed to cross-build cargo. * Add upstream patch to allow building on aarch64-apple-darwin (alexcrichton#74) * Use fs module convenience methods for MUSL patch * Add upstream patch to allow building on aarch64-apple-darwin Closes alexcrichton#72 * i586 support (alexcrichton#76) * Bump to 111.12.0+1.1.1h * Add aarch64-apple-darwin to CI (alexcrichton#77) * OpenBSD support (alexcrichton#78) * Update OpenSSL to 1.1.1i * Update CI * Remove now no-longer-necessary patches * Support targets `armv7-unknown-linux-gnueabi/musleabi` (alexcrichton#80) Support targets `armv7-unknown-linux-gnueabi` & `armv7-unknown-linux-musleabi` * Make it compile for wasm32-wasi (alexcrichton#81) * Catch more failures on Windows in CI (alexcrichton#84) * Catch more failures on Windows in CI * Link missing library for OpenSSL on MSVC Looks like some functions may pull in user32 functions, so that library needs to be linked. * Try to fix msvc +crt-static builds * Try to see all failures * More output * Fix setting crt-static * More CI tweaks * Add the bin dir to cargo's search path on MSVC That's where it contains the actual dlls needed at runtime * Moved "no-shared" so that also windows statically link to the libraries (alexcrichton#83) * Moved "no-shared" so that windows statically link to the libraries * try reapplying changes * removed shared as it's always false. Co-authored-by: molleafauss <lmollea@yahoo.it> * Bump to OpenSSL 1.1.1j (alexcrichton#85) * add nasm support for msvc (alexcrichton#87) * add nasm support for windows-msvc This will automatically detect whether nasm.exe is installed and try to enable the assembly language routines. These can also be disabled by set the `OPENSSL_RUST_NO_NASM` environment variable to a non-zero value. * don't use '>> $GITHUB_ENV' to overwrite PATH * remove the windows check in CI * give user more control on the env var * add env var in CI, less acceptable values for env var * fix path format for using bash on windows * 'OPENSSL_RUST_USE_NASM' env var only accept 0 or 1 * Bump to 1.1.1k * Add FreeBSD powerpc support (alexcrichton#90) This is needed to support cargo cross-build for 32-bit powerpc on FreeBSD. * Upgrade to GitHub-native Dependabot (alexcrichton#92) Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> * Support targets `armv5te-unknown-linux-gnueabi/musleabi` (alexcrichton#94) * Support targets `popwerpc64(le)-unknown-linux-musl` (alexcrichton#95) * Support targets `mips64(el)-unknown-linux-muslabi64` (alexcrichton#96) * Support target `s390x-unknown-linux-musl` (alexcrichton#97) * Bump to openssl 1.1.1l (alexcrichton#100) * Bump to 1.1.1m * test on 1.1.1 branch * Fix aarch64-apple-darwin CI (alexcrichton#116) (cherry picked from commit 466ffd2) * Bump to 1.1.1n (alexcrichton#123) * Update "old" windows image on CI (alexcrichton#126) * Bump to 1.1.1o (alexcrichton#136) * Bump to 1.1.1o * Disable arm-linux-androideabi test * Backport wycheproof exclude to 1.1.1 branch (alexcrichton#139) * Backport wycheproof exclude to 1.1.1 branch * Backport exclude CI checks * Backport exclude CI checks * Configure `--openssldir` to its default location (alexcrichton#141) (alexcrichton#142) As pointed out in alexcrichton#140 otherwise certificates and configure is by default looked up in the directory of the build machine itself which is often a writable path. Instead switch the configuration option back to the default recommended in openssl's `INSTALL.md` Closes alexcrichton#140 * Bump to 111.20.0+1.1.1o (alexcrichton#143) * Bump to 1.1.1p (alexcrichton#145) * Bump to 1.1.1q (alexcrichton#147) Co-authored-by: Jay <BusyJay@users.noreply.github.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com> Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com> Co-authored-by: msizanoen1 <55322658+msizanoen1@users.noreply.github.com> Co-authored-by: Franck Royer <royer.franck@gmail.com> Co-authored-by: Tobias Kortkamp <t6@users.noreply.github.com> Co-authored-by: Joshua M. Clulow <josh@sysmgr.org> Co-authored-by: MikaelUrankar <49529234+MikaelUrankar@users.noreply.github.com> Co-authored-by: Steven Fackler <sfackler@gmail.com> Co-authored-by: Patrick Mooney <pmooney@pfmooney.com> Co-authored-by: Steven Fackler <sfackler@palantir.com> Co-authored-by: James McMurray <jamesmcm03@gmail.com> Co-authored-by: Xintao <hunterlxt@live.com> Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com> Co-authored-by: petreeftime <petre.eftime@gmail.com> Co-authored-by: Brandon Bergren <git@bdragon.rtk0.net> Co-authored-by: Jake Goulding <shepmaster@mac.com> Co-authored-by: Will <will@mon.im> Co-authored-by: Jake Goulding <jake.goulding@gmail.com> Co-authored-by: kiron1 <kiron1@gmail.com> Co-authored-by: IceCodeNew <32576256+IceCodeNew@users.noreply.github.com> Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> Co-authored-by: Alex Malgaroli <alex.malgaroli@gmail.com> Co-authored-by: molleafauss <lmollea@yahoo.it> Co-authored-by: Kane <KaneGreen@users.noreply.github.com> Co-authored-by: Brandon Bergren <bdragon@FreeBSD.org> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: messense <messense@icloud.com> Co-authored-by: Alexis Mousset <contact@amousset.me> Co-authored-by: Eric Huss <eric@huss.org> Co-authored-by: Alexis Mousset <alexis.mousset@rudder.io>

alexcrichton reviewed Mar 18, 2021

View reviewed changes

KaneGreen added 2 commits March 20, 2021 09:51

add nasm support for windows-msvc

f2a79e0

This will automatically detect whether nasm.exe is installed and try to enable the assembly language routines. These can also be disabled by set the `OPENSSL_RUST_NO_NASM` environment variable to a non-zero value.

don't use '>> $GITHUB_ENV' to overwrite PATH

4b27e79

KaneGreen commented Mar 20, 2021

View reviewed changes

alexcrichton reviewed Mar 22, 2021

View reviewed changes

KaneGreen added 2 commits March 23, 2021 10:42

remove the windows check in CI

457aa1d

give user more control on the env var

7cee6f3

alexcrichton reviewed Mar 23, 2021

View reviewed changes

KaneGreen and others added 3 commits March 24, 2021 10:47

add env var in CI, less acceptable values for env var

cc41e51

fix path format for using bash on windows

76c7750

'OPENSSL_RUST_USE_NASM' env var only accept 0 or 1

127ad0c

alexcrichton merged commit 0ac015f into alexcrichton:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add nasm support for msvc #87

add nasm support for msvc #87

KaneGreen commented Mar 16, 2021

alexcrichton commented Mar 16, 2021

KaneGreen commented Mar 16, 2021 •

edited

Loading

alexcrichton commented Mar 16, 2021

KaneGreen commented Mar 17, 2021 •

edited

Loading

alexcrichton commented Mar 17, 2021

KaneGreen commented Mar 18, 2021 •

edited

Loading

alexcrichton left a comment

alexcrichton Mar 18, 2021

KaneGreen commented Mar 18, 2021 •

edited

Loading

alexcrichton commented Mar 18, 2021

KaneGreen commented Mar 19, 2021

KaneGreen commented Mar 19, 2021

alexcrichton commented Mar 19, 2021

KaneGreen commented Mar 20, 2021 •

edited

Loading

KaneGreen Mar 20, 2021

alexcrichton Mar 22, 2021

alexcrichton Mar 22, 2021

alexcrichton Mar 22, 2021

KaneGreen Mar 22, 2021

KaneGreen commented Mar 23, 2021

alexcrichton Mar 23, 2021

KaneGreen Mar 23, 2021 •

edited

Loading

alexcrichton Mar 24, 2021

alexcrichton Mar 23, 2021

KaneGreen Mar 23, 2021 •

edited

Loading

alexcrichton Mar 23, 2021

KaneGreen Mar 23, 2021

KaneGreen commented Mar 25, 2021

KaneGreen commented Mar 25, 2021

add nasm support for msvc #87

add nasm support for msvc #87

Conversation

KaneGreen commented Mar 16, 2021

alexcrichton commented Mar 16, 2021

KaneGreen commented Mar 16, 2021 • edited Loading

alexcrichton commented Mar 16, 2021

KaneGreen commented Mar 17, 2021 • edited Loading

alexcrichton commented Mar 17, 2021

KaneGreen commented Mar 18, 2021 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaneGreen commented Mar 18, 2021 • edited Loading

alexcrichton commented Mar 18, 2021

KaneGreen commented Mar 19, 2021

KaneGreen commented Mar 19, 2021

alexcrichton commented Mar 19, 2021

KaneGreen commented Mar 20, 2021 • 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

Choose a reason for hiding this comment

KaneGreen commented Mar 23, 2021

Choose a reason for hiding this comment

KaneGreen Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaneGreen Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaneGreen commented Mar 25, 2021

KaneGreen commented Mar 25, 2021

KaneGreen commented Mar 16, 2021 •

edited

Loading

KaneGreen commented Mar 17, 2021 •

edited

Loading

KaneGreen commented Mar 18, 2021 •

edited

Loading

KaneGreen commented Mar 18, 2021 •

edited

Loading

KaneGreen commented Mar 20, 2021 •

edited

Loading

KaneGreen Mar 23, 2021 •

edited

Loading

KaneGreen Mar 23, 2021 •

edited

Loading