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

prefix x86 asm symbols with _ on Windows like on Apple #61

Merged
merged 2 commits into from
Aug 7, 2023
Merged

prefix x86 asm symbols with _ on Windows like on Apple #61

merged 2 commits into from
Aug 7, 2023

Conversation

jeremyd2019
Copy link
Contributor

@jeremyd2019 jeremyd2019 commented Jul 24, 2023

Fixes #60

I added i686 to the sha1.yml also, it passes on stable, but fails on 1.43.0 probably due to it not being compatible with the version of mingw-w64 and/or gcc provided by msys2. This also fails on x86_64. I don't know where it was finding x86_64-w64-mingw32-gcc before, probably something random installed on the runner. Now it is definitely using the msys2 version. @mati865

@jeremyd2019
Copy link
Contributor Author

If this is desired, I could do the other x86 asm files too.

@tarcieri
Copy link
Member

@jeremyd2019 you can test it by cloning https://github.com/RustCrypto/hashes and adding a directive to Cargo.toml like:

[patch.crates-io.sha1-asm]
path = "../asm-hashes/sha1-asm"

@mati865
Copy link

mati865 commented Jul 24, 2023

fails on 1.43.0 probably due to it not being compatible with the version of mingw-w64 and/or gcc provided by msys2

That's very likely. 1.48 was the release when I've implemented the refined version of external toolchain detection mechanism. Before that it was hit or miss.

@jeremyd2019
Copy link
Contributor Author

Thanks. I am not all that familiar with rust...

jeremyd@XXXX MINGW32 /d/tmp/hashes/sha1
$ cargo test -F asm
   Compiling version_check v0.9.4
   Compiling typenum v1.16.0
   Compiling cc v1.0.79
   Compiling proc-macro-hack v0.5.20+deprecated
   Compiling generic-array v0.14.7
   Compiling sha1-asm v0.5.1 (D:\tmp\asm-hashes\sha1)
   Compiling blobby v0.3.1
   Compiling block-buffer v0.10.4
   Compiling crypto-common v0.1.6
   Compiling digest v0.10.7
   Compiling hex-literal-impl v0.2.3
   Compiling cpufeatures v0.2.9
   Compiling cfg-if v1.0.0
   Compiling sha1 v0.10.5 (D:\tmp\hashes\sha1)
   Compiling hex-literal v0.2.2
    Finished test [optimized + debuginfo] target(s) in 2m 10s
     Running unittests src\lib.rs (D:\tmp\hashes\target\debug\deps\sha1-84c50a79e1c3c373.exe)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\mod.rs (D:\tmp\hashes\target\debug\deps\mod-8bc81ebefe5b1830.exe)

running 2 tests
test sha1_main ... ok
test sha1_rand ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.13s

   Doc-tests sha1

running 1 test
test src\lib.rs - (line 14) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.80s

@jeremyd2019 jeremyd2019 changed the title sha1: prefix asm symbol with _ on Windows like on Apple prefix asm symbol with _ on Windows like on Apple Jul 24, 2023
@jeremyd2019 jeremyd2019 changed the title prefix asm symbol with _ on Windows like on Apple prefix asm symbols with _ on Windows like on Apple Jul 24, 2023
@jeremyd2019
Copy link
Contributor Author

If this is desired, I could do the other x86 asm files too.

I went ahead and did this, and all hashes tests pass on i686-pc-windows-gnu with -F asm now.

@jeremyd2019 jeremyd2019 changed the title prefix asm symbols with _ on Windows like on Apple prefix x86 asm symbols with _ on Windows like on Apple Jul 24, 2023
@newpavlov
Copy link
Member

Looks good! Can you fix the CI? Simply bumping MSRV should be fine. It's unfortunate and strictly speaking goes against our MSRV policy, but I guess the crates did not work on Windows properly either way and MacOS was broken as well.

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Jul 26, 2023

I was going to suggest that I could remove the CI changes from this PR if that would help reviewability. I'm not much of a rust user/developer so I wouldn't know how to go about the proper way of doing this. I could just bump the version in the CI matrix (I'm guessing to 1.48.0 based on @mati865's comment). Also, if this is something you want, I could attempt to update the other CI workflows (I only did sha1 so far) too.

I don't know what version to use to make macos work, either.

@jeremyd2019
Copy link
Contributor Author

I don't know what version to use to make macos work, either.

Trial and error found the lowest rust version that succeeds on macos is 1.54.0.

Actually use MSYS2's mingw toolchains instead of whatever happens to be
on the runner.

Update msrv matrix entries to versions that work with current
toolchains/OS versions: 1.48.0 for windows-gnu, and 1.54.0 for macos.

Add building i686-pc-windows-gnu.
as was already being done for Apple
@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Jul 26, 2023

I guess the crates did not work on Windows properly either way and MacOS was broken as well.

The CI did pass with 1.43.0 on x86_64 when using whatever mingw-w64 toolchain it found on the runner (presumably that toolchain was suitably old to work with that older rust version). I switched to using msys2's toolchain because there was no i686 toolchain found on the runner otherwise, and because setup-msys2 was being used I think it was intended that msys2 would be used.

I imagine it was something similar on MacOS, where the toolchain being used on the runner was too new to work with older rust versions, but if an older os/toolchain version were used the crate itself would probably be fine.

@newpavlov newpavlov merged commit 3515f94 into RustCrypto:master Aug 7, 2023
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sha1-asm fails to link on i686-pc-windows-gnu
4 participants