feature `sha1/asm` is enabled on windows #917

NobodyXu · 2023-06-29T14:42:27Z

Duplicates

I have searched the existing issues

Current behavior 😯

Compiling on windows failed failed with errors from sha1-asm, which is known in RustCrypto/asm-hashes#17 and should be migrated by gix-features by disabling sha1/asm on windows:

warning: cl : Command line warning D9024 : unrecognized source file type 'src/x64.S', object file assumed
warning: cl : Command line warning D9027 : source file 'src/x64.S' ignored

error: failed to run custom build command for `sha1-asm v0.5.1`

Caused by:
  process didn't exit successfully: `D:\a\cargo-binstall\cargo-binstall\target\release\build\sha1-asm-898e65df03af5add\build-script-build` (exit code: 1)
  --- stdout
  TARGET = Some("x86_64-pc-windows-msvc")
  OPT_LEVEL = Some("z")
  HOST = Some("x86_64-pc-windows-msvc")
  cargo:rerun-if-env-changed=CC_x86_64-pc-windows-msvc
  CC_x86_64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=CC_x86_64_pc_windows_msvc
  CC_x86_64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_CC
  HOST_CC = None
  cargo:rerun-if-env-changed=CC
  CC = None
  cargo:rerun-if-env-changed=CFLAGS_x86_64-pc-windows-msvc
  CFLAGS_x86_64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=CFLAGS_x86_64_pc_windows_msvc
  CFLAGS_x86_64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_CFLAGS
  HOST_CFLAGS = None
  cargo:rerun-if-env-changed=CFLAGS
  CFLAGS = None
  cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS
  CRATE_CC_NO_DEFAULTS = None
  CARGO_CFG_TARGET_FEATURE = Some("fxsr,sse,sse2")
  DEBUG = Some("false")
  running: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX64\\x64\\cl.exe" "-nologo" "-MD" "-O1" "-Brepro" "-W4" "-c" "-FoD:\\a\\cargo-binstall\\cargo-binstall\\target\\x86_64-pc-windows-msvc\\release\\build\\sha1-asm-f3dce8890bea4dcb\\out\\src/x64.o" "-c" "src/x64.S"
  cargo:warning=cl : Command line warning D9024 : unrecognized source file type 'src/x64.S', object file assumed
  cargo:warning=cl : Command line warning D9027 : source file 'src/x64.S' ignored
  cl : Command line warning D9021 : no action performed
  exit code: 0
  cargo:rerun-if-env-changed=AR_x86_64-pc-windows-msvc
  AR_x86_64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=AR_x86_64_pc_windows_msvc
  AR_x86_64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_AR
  HOST_AR = None
  cargo:rerun-if-env-changed=AR
  AR = None
  cargo:rerun-if-env-changed=ARFLAGS_x86_64-pc-windows-msvc
  ARFLAGS_x86_64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=ARFLAGS_x86_64_pc_windows_msvc
  ARFLAGS_x86_64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_ARFLAGS
  HOST_ARFLAGS = None
  cargo:rerun-if-env-changed=ARFLAGS
  ARFLAGS = None
  running: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX64\\x64\\lib.exe" "-out:D:\\a\\cargo-binstall\\cargo-binstall\\target\\x86_64-pc-windows-msvc\\release\\build\\sha1-asm-f3dce8890bea4dcb\\out\\libsha1.a" "-nologo" "D:\\a\\cargo-binstall\\cargo-binstall\\target\\x86_64-pc-windows-msvc\\release\\build\\sha1-asm-f3dce8890bea4dcb\\out\\src/x64.o"
  LINK : fatal error LNK1181: cannot open input file 'D:\a\cargo-binstall\cargo-binstall\target\x86_64-pc-windows-msvc\release\build\sha1-asm-f3dce8890bea4dcb\out\src\x64.o'
  exit code: 1181

  --- stderr


  error occurred: Command "C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX64\\x64\\lib.exe" "-out:D:\\a\\cargo-binstall\\cargo-binstall\\target\\x86_64-pc-windows-msvc\\release\\build\\sha1-asm-f3dce8890bea4dcb\\out\\libsha1.a" "-nologo" "D:\\a\\cargo-binstall\\cargo-binstall\\target\\x86_64-pc-windows-msvc\\release\\build\\sha1-asm-f3dce8890bea4dcb\\out\\src/x64.o" with args "lib.exe" did not execute successfully (status code exit code: 1181).


warning: build failed, waiting for other jobs to finish...
      Timing report saved to D:\a\cargo-binstall\cargo-binstall\target\cargo-timings\cargo-timing-20230629T142057Z.html

Expected behavior 🤔

Build should succeed.

Steps to reproduce 🕹

Compiling gix on x64/arm windows with features max-performance enabled.

The text was updated successfully, but these errors were encountered:

Byron · 2023-06-29T19:13:47Z

Thanks a lot for letting me know! Is this something you would feel comfortable to contribute? It seems like this project's CI is missing that environment or configuration. I will also take a stab at it and at least try to get a reproduction on this project's CI, from there fixing it should get easier.

NobodyXu · 2023-06-30T01:52:48Z

Yes, I will have a try once I have time.
Reproducing it in CI should be easy.

NobodyXu · 2023-06-30T01:58:40Z

To be honest, I think this might be a upstream bug.
I googled it and this discussion from 2015 says target_env is internal to compiler, but it's clearly documented in user facing document.

Given that gix-features will not enable sha1/asm unless not(target_env = "msvc") and on x86/arm, it could only be an upstream bug.

NobodyXu · 2023-06-30T03:12:35Z

I think we can workaround this by doing not(target = "x86_64-windows-msvc"), not(target = "aarch64-windows-msvc") instead of not(target_env = "msvc") given that sha1/asm only supports arm 64 and x86 anyway.

But that will require quite a list of targets since there are many x86 targets (i686, i484)

Byron · 2023-06-30T06:33:51Z

What I don't understand about this is that as per the gix-features/Cargo.toml, it already excludes the respective environment:

[target.'cfg(all(any(target_arch = "aarch64", target_arch = "x86", target_arch = "x86_64"), not(target_env = "msvc")))'.dependencies]
sha1 = { version = "0.10.0", optional = true, features = ["asm"] }

Clearly, x86_64-pc-windows-msvc should not enable the asm feature, instead it should only pull in the sha1 crate without additional features.

To me this looks like something cargo gets wrong, but maybe I am just not getting how dependencies work with more complex per-target overrides.

My recommendation, in the meantime, is to use max-performance-safe for these platforms, and maybe do that based on target specific dependencies in cargo-binstall instead?

Some other strangeness is that CI already builds that platform - it will install the max version of gix which activates max-performance as well, and there it works as if the target-specific settings in gix-features do work, sometimes. The only difference I see is that it builds with --debug instead of --release.

When looking at the Cargo.toml of the sha1 crate, I see that they also try to make that available only on targets that are known to work, so something really isn't working as expected there, sometimes.

Maybe an avenue would be to understand why my CI seems to work on that environment, whereas the one at cargo-binstall doesn't? What's the difference, ideally this can be reproduced here. In the meantime, I think it's worth trying to workaround in cargo-binstall as well by selectively using max-performance-safe instead?

NobodyXu · 2023-06-30T06:41:43Z

Clearly, x86_64-pc-windows-msvc should not enable the asm feature, instead it should only pull in the sha1 crate without additional features.

Yeah I think this could be a bug in cargo.

My recommendation, in the meantime, is to use max-performance-safe for these platforms, and maybe do that based on target specific dependencies in cargo-binstall instead?

I agree, though I would probably still turns on flate2/zlib-ng since in cargo-bins/cargo-binstall#1184 we turned on support for custom registry and test it by pulling the crates.io git index.

It took much longer than I'd imagine, on my M1 it took at least 2m and on CI it took 11m.

NobodyXu · 2023-06-30T06:57:07Z

Some other strangeness is that CI already builds that platform - it will install the max version of gix which activates max-performance as well, and there it works as if the target-specific settings in gix-features do work, sometimes. The only difference I see is that it builds with --debug instead of --release.

Yeah that's really strange, because our CI also build binstall in debug and release and they both failed.

When looking at the Cargo.toml of the sha1 crate, I see that they also try to make that available only on targets that are known to work, so something really isn't working as expected there, sometimes.

Maybe an avenue would be to understand why my CI seems to work on that environment, whereas the one at cargo-binstall doesn't? What's the difference, ideally this can be reproduced here. In the meantime, I think it's worth trying to workaround in cargo-binstall as well by selectively using max-performance-safe instead?

Yeah, I will have another look at it once I'm free, it's really really strange.

Byron · 2023-06-30T07:00:26Z

It took much longer than I'd imagine, on my M1 it took at least 2m and on CI it took 11m.

You mean shallow cloning took 2m and 11m respectively?

It should be much faster, even without zlib-ng I'd expect the time dominated by transfer, and not pack resolution.

❯ gix clone --depth 1 https://github.com/rust-lang/crates.io-index --bare
 08:58:05 read pack done 82.7MB in 45.16s (1.8MB/s)
 08:58:05  indexing done 144.8k objects in 26.77s (5.4k objects/s)
 08:58:05 decompressing done 530.4MB in 26.77s (19.8MB/s)
 08:58:05     Resolving done 144.8k objects in 0.38s (382.3k objects/s)
 08:58:05      Decoding done 2.5GB in 0.38s (6.6GB/s)
 08:58:06 writing index file done 4.1MB in 0.01s (671.2MB/s)
 08:58:06  create index file done 144.8k objects in 27.17s (5.3k objects/s)
HEAD:refs/remotes/origin/HEAD (implicit)
        05fe54b35765137db5650c546a9efc5899cb378b HEAD symref-target:refs/heads/master -> refs/remotes/origin/HEAD [new]
+refs/heads/*:refs/remotes/origin/*
        05fe54b35765137db5650c546a9efc5899cb378b refs/heads/master -> refs/remotes/origin/master [new]

git ( master) [?] via 🐍 took 46s
❯ gix clone https://github.com/rust-lang/crates.io-index --bare crates-io-full
 08:58:52 read pack done 310.7MB in 29.54s (10.5MB/s)
 08:58:52  indexing done 531.1k objects in 29.17s (18.2k objects/s)
 08:58:52 decompressing done 1.8GB in 29.17s (61.9MB/s)
 08:58:54     Resolving done 531.1k objects in 1.59s (333.7k objects/s)
 08:58:54      Decoding done 18.4GB in 1.59s (11.6GB/s)
 08:58:54 writing index file done 14.9MB in 0.02s (638.8MB/s)
 08:58:54  create index file done 531.1k objects in 30.85s (17.2k objects/s)
HEAD:refs/remotes/origin/HEAD (implicit)
        05fe54b35765137db5650c546a9efc5899cb378b HEAD symref-target:refs/heads/master -> refs/remotes/origin/HEAD [new]
+refs/heads/*:refs/remotes/origin/*
        05fe54b35765137db5650c546a9efc5899cb378b refs/heads/master -> refs/remotes/origin/master [new]

Above it shows that --depth 1 is actually slower than a full clone as the remote has more work.

NobodyXu · 2023-06-30T08:47:15Z

You mean shallow cloning took 2m and 11m respectively?

Yes, I think it could be because the test is run in debug mode.

Even in release, cargo-binstall is compiled using O2, so a better option might be to always compile gix and its dependencies with O3, probably more reliable than max-performance.

NobodyXu · 2023-06-30T13:27:44Z

Yeah setting:

# Set the default for dependencies on debug.
[profile.dev.package."*"]
opt-level = "z"

speedup the cloning to 2m.
I guess it's time for binstall to use O2 or even O3 given that gix really need all that optimization.

NobodyXu · 2023-07-01T08:08:49Z

Yeah, I will have another look at it once I'm free, it's really really strange.

Building gix directly with:

cargo build --features max-performance,blocking-http-transport-reqwest-rust-tls --target x86_64-pc-windows-msvc

always succeed in dev and release mode, with or without blocking-http-transport-reqwest-rust-tls.

But somehow when building from cargo-binstall, it failed due to sha1-asm.

NobodyXu · 2023-07-01T08:10:42Z

Running cargo tree -i sha1-asm --target x86_64-pc-windows-msvc in cargo-binstall gives:

sha1-asm v0.5.1
└── sha1 v0.10.5
    └── gix-features v0.31.1
        ├── gix v0.48.0
        │   └── binstalk v0.13.0 (/Users/nobodyxu/Dev/cargo-binstall/crates/binstalk)
        ├── gix-commitgraph v0.17.1
        │   ├── gix v0.48.0 (*)
        │   ├── gix-negotiate v0.4.0
        │   │   └── gix v0.48.0 (*)
        │   ├── gix-revwalk v0.3.0
        │   │   ├── gix-negotiate v0.4.0 (*)
        │   │   ├── gix-revision v0.17.0
        │   │   │   ├── gix v0.48.0 (*)
        │   │   │   └── gix-refspec v0.13.0
        │   │   │       └── gix v0.48.0 (*)
        │   │   └── gix-traverse v0.29.0
        │   │       ├── gix v0.48.0 (*)
        │   │       ├── gix-index v0.20.0
        │   │       │   ├── gix v0.48.0 (*)
        │   │       │   └── gix-worktree v0.21.1
        │   │       │       └── gix v0.48.0 (*)
        │   │       └── gix-pack v0.39.1
        │   │           ├── gix v0.48.0 (*)
        │   │           └── gix-odb v0.49.1
        │   │               └── gix v0.48.0 (*)
        │   └── gix-traverse v0.29.0 (*)
        ├── gix-config v0.25.1
        │   └── gix v0.48.0 (*)
        ├── gix-fs v0.3.0
        │   ├── gix v0.48.0 (*)
        │   ├── gix-ref v0.32.1
        │   │   ├── gix v0.48.0 (*)
        │   │   ├── gix-config v0.25.1 (*)
        │   │   └── gix-discover v0.21.1
        │   │       └── gix v0.48.0 (*)
        │   ├── gix-tempfile v7.0.0
        │   │   ├── gix v0.48.0 (*)
        │   │   ├── gix-lock v7.0.1
        │   │   │   ├── gix v0.48.0 (*)
        │   │   │   ├── gix-index v0.20.0 (*)
        │   │   │   └── gix-ref v0.32.1 (*)
        │   │   ├── gix-pack v0.39.1 (*)
        │   │   └── gix-ref v0.32.1 (*)
        │   └── gix-worktree v0.21.1 (*)
        ├── gix-glob v0.9.1
        │   ├── gix v0.48.0 (*)
        │   ├── gix-attributes v0.14.1
        │   │   ├── gix v0.48.0 (*)
        │   │   └── gix-worktree v0.21.1 (*)
        │   ├── gix-config v0.25.1 (*)
        │   ├── gix-ignore v0.4.1
        │   │   ├── gix v0.48.0 (*)
        │   │   └── gix-worktree v0.21.1 (*)
        │   └── gix-worktree v0.21.1 (*)
        ├── gix-index v0.20.0 (*)
        ├── gix-object v0.32.0
        │   ├── gix v0.48.0 (*)
        │   ├── gix-diff v0.32.0
        │   │   ├── gix v0.48.0 (*)
        │   │   └── gix-pack v0.39.1 (*)
        │   ├── gix-index v0.20.0 (*)
        │   ├── gix-negotiate v0.4.0 (*)
        │   ├── gix-odb v0.49.1 (*)
        │   ├── gix-pack v0.39.1 (*)
        │   ├── gix-ref v0.32.1 (*)
        │   ├── gix-revision v0.17.0 (*)
        │   ├── gix-revwalk v0.3.0 (*)
        │   ├── gix-traverse v0.29.0 (*)
        │   └── gix-worktree v0.21.1 (*)
        ├── gix-odb v0.49.1 (*)
        ├── gix-pack v0.39.1 (*)
        ├── gix-protocol v0.35.0
        │   └── gix v0.48.0 (*)
        ├── gix-ref v0.32.1 (*)
        ├── gix-transport v0.33.1
        │   ├── gix v0.48.0 (*)
        │   └── gix-protocol v0.35.0 (*)
        ├── gix-url v0.20.1
        │   ├── gix v0.48.0 (*)
        │   ├── gix-credentials v0.16.1
        │   │   ├── gix v0.48.0 (*)
        │   │   ├── gix-protocol v0.35.0 (*)
        │   │   └── gix-transport v0.33.1 (*)
        │   └── gix-transport v0.33.1 (*)
        └── gix-worktree v0.21.1 (*)

But doing the same in gix gives:

$ cargo tree --target x86_64-pc-windows-msvc --features max-performance -i sha1-asm
warning: nothing to print.

To find dependencies that require specific target platforms, try to use option `--target all` first, and t
hen narrow your search scope accordingly.

Byron · 2023-07-01T08:44:28Z

Maybe there is a way to narrow down the cause of this by adding gix to a proxy-manifest that is freshly initialized, to see if it still somehow gets sha1-asm activated. If not, one can probably try to make the proxy more similar to the actual cargo-binstall manifest to learn what's causing this.

NobodyXu · 2023-07-06T14:19:59Z

Maybe there is a way to narrow down the cause of this by adding gix to a proxy-manifest that is freshly initialized, to see if it still somehow gets sha1-asm activated. If not, one can probably try to make the proxy more similar to the actual cargo-binstall manifest to learn what's causing this.

I am trying to do this, but it's really hard to setup env on windows.

Specifically, I have problem installing clang, I installed the clang component in vs installer but still doesn't work.

Byron · 2023-07-06T15:52:06Z

That's interesting, I thought such an environment is already available on the CI of cargo-binstall?

I also failed to setup the microsoft toolchain, and just barely got my own VM to work with the GNU toolchain, if that's somehow related. In any case, I wouldn't be able to help here :/.

NobodyXu · 2023-07-06T16:06:29Z

That's interesting, I thought such an environment is already available on the CI of cargo-binstall?

Yeah I could just use the Github Action for this.
Thanks, I would probably setup a new github repo for this.

NobodyXu · 2023-07-09T08:54:24Z

I was able to reproduce it on my arch64 windows by running JUST_EXTRA_FEATURES=git-max-perf just build on branch speedup/gix:

error: failed to run custom build command for `sha1-asm v0.5.1`

Caused by:
  process didn't exit successfully: `C:\Users\nobodyxu\cargo-binstall\target\debug\build\sha1-asm-75aa7a029eab4153\build-script-build` (exit code: 1)
  --- stdout
  TARGET = Some("aarch64-pc-windows-msvc")
  OPT_LEVEL = Some("0")
  HOST = Some("aarch64-pc-windows-msvc")
  cargo:rerun-if-env-changed=CC_aarch64-pc-windows-msvc
  CC_aarch64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=CC_aarch64_pc_windows_msvc
  CC_aarch64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_CC
  HOST_CC = None
  cargo:rerun-if-env-changed=CC
  CC = None
  cargo:rerun-if-env-changed=CFLAGS_aarch64-pc-windows-msvc
  CFLAGS_aarch64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=CFLAGS_aarch64_pc_windows_msvc
  CFLAGS_aarch64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_CFLAGS
  HOST_CFLAGS = None
  cargo:rerun-if-env-changed=CFLAGS
  CFLAGS = None
  cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS
  CRATE_CC_NO_DEFAULTS = None
  CARGO_CFG_TARGET_FEATURE = Some("neon")
  DEBUG = Some("true")
  running: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX86\\arm64\\cl.exe" "-nologo" "-MD" "-Z7" "-Brepro" "-W4" "-march=armv8-a+crypto" "-c" "-o" "C:\\Users\\nobodyxu\\cargo-binstall\\target\\aarch64-pc-windows-msvc\\debug\\build\\sha1-asm-600eae65c368c0c1\\out\\src/aarch64.o" "src/aarch64.S"
  cargo:warning=cl : Command line warning D9035 : option 'o' has been deprecated and will be removed in a future release
  cargo:warning=cl : Command line warning D9002 : ignoring unknown option '-march=armv8-a+crypto'
  cargo:warning=cl : Command line warning D9024 : unrecognized source file type 'src/aarch64.S', object file assumed
  cargo:warning=cl : Command line warning D9027 : source file 'src/aarch64.S' ignored
  cl : Command line warning D9021 : no action performed
  exit code: 0
  cargo:rerun-if-env-changed=AR_aarch64-pc-windows-msvc
  AR_aarch64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=AR_aarch64_pc_windows_msvc
  AR_aarch64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_AR
  HOST_AR = None
  cargo:rerun-if-env-changed=AR
  AR = None
  cargo:rerun-if-env-changed=ARFLAGS_aarch64-pc-windows-msvc
  ARFLAGS_aarch64-pc-windows-msvc = None
  cargo:rerun-if-env-changed=ARFLAGS_aarch64_pc_windows_msvc
  ARFLAGS_aarch64_pc_windows_msvc = None
  cargo:rerun-if-env-changed=HOST_ARFLAGS
  HOST_ARFLAGS = None
  cargo:rerun-if-env-changed=ARFLAGS
  ARFLAGS = None
  running: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX86\\arm64\\lib.exe" "-out:C:\\Users\\nobodyxu\\cargo-binstall\\target\\aarch64-pc-windows-msvc\\debug\\build\\sha1-asm-600eae65c368c0c1\\out\\libsha1.a" "-nologo" "C:\\Users\\nobodyxu\\cargo-binstall\\target\\aarch64-pc-windows-msvc\\debug\\build\\sha1-asm-600eae65c368c0c1\\out\\src/aarch64.o"
  LINK : fatal error LNK1181: cannot open input file 'C:\Users\nobodyxu\cargo-binstall\target\aarch64-pc-windows-msvc\debug\build\sha1-asm-600eae65c368c0c1\out\src\aarch64.o'
  exit code: 1181

  --- stderr


  error occurred: Command "C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX86\\arm64\\lib.exe" "-out:C:\\Users\\nobodyxu\\cargo-binstall\\target\\aarch64-pc-windows-msvc\\debug\\build\\sha1-asm-600eae65c368c0c1\\out\\libsha1.a" "-nologo" "C:\\Users\\nobodyxu\\cargo-binstall\\target\\aarch64-pc-windows-msvc\\debug\\build\\sha1-asm-600eae65c368c0c1\\out\\src/aarch64.o" with args "lib.exe" did not execute successfully (status code exit code: 1181).


error: Recipe `build` failed on line 204 with exit code 101

(in additional to failure to build ring due to clang not installed).

NobodyXu · 2023-07-09T09:02:55Z

This error I got on my aarch64 windows is very similar to our CI, it failed at link-time after build.rs is run.

Looking at the sha1-asm build.rs, its use of cc::Build::compile is deprecated:

For backwards compatibility, if output starts with “lib” and ends with “.a”, a second “lib” prefix and “.a” suffix do not get added on, but this usage is deprecated; please omit lib and .a in the argument that you pass.

It should pass just a name instead and msvc would deal with that:

The output string argument determines the file name for the compiled library. The Rust compiler will create an assembly named “lib”+output+“.a”. MSVC will create a file named output+“.lib”.

Byron · 2023-07-10T08:14:31Z

Thanks so much for pushing forward on the resolution for this issue, I appreciate it! Being able to reproduce it locally makes a fix possible! I never managed to get a VM that has MSVC installed (and I tried), it never worked with Rust and I thought the whole process is insane.

Looking at the sha1-asm build.rs, its use of cc::Build::compile is deprecated:

It's certainly worth fixing that as it may help to make it build better in general, but I have some doubts that it's going to make a difference as the problem is that it tries to use these ASM files at all. As far as I know, they don't exist for all platforms and builds should never be attempted, falling into the realm of cargo features. I absolute hope I am misunderstanding this though, it would be great if the attached PR could fix this.

Let me again thank you for your efforts and apologize for just cheering you on from the side-lines. I think once this issue is fixed for cargo-binstall, it will also be fixed for every current and future user of gitoxide which is a big deal for me. This build error has been popping up over and over again and it could not be resolved thus far.

NobodyXu · 2023-07-10T08:35:18Z

Thanks so much for pushing forward on the resolution for this issue, I appreciate it!

You are welcome!

Being able to reproduce it locally makes a fix possible! I never managed to get a VM that has MSVC installed (and I tried), it never worked with Rust and I thought the whole process is insane.

It's quite hard to setup dev env on windows.
I used visual studio installer to select the following packages:

It's certainly worth fixing that as it may help to make it build better in general, but I have some doubts that it's going to make a difference as the problem is that it tries to use these ASM files at all.

Yeah, though I still think it might help, since they also use the / on windows where it is supposed to use \.

Byron · 2023-07-10T17:46:46Z

Thanks for your help with this! I remember that I tried to reduce the amount of checkboxes on the right to safe space, but maybe that's the cause of my trouble in the first place.

Could you also tell me how you get a development shell that also includes git? I think that might also have been the reason it never worked out for me - either I have git, or the MS tool suite, but not both (maybe).

Thanks again

NobodyXu · 2023-07-11T00:44:05Z

Could you also tell me how you get a development shell that also includes git? I think that might also have been the reason it never worked out for me - either I have git, or the MS tool suite, but not both (maybe).

I uses "Git Bash", downloaded from https://git-scm.com/download/win

NobodyXu · 2023-07-11T00:46:10Z

I also installed strawberry perl for perl and cygwin.

Byron · 2023-07-11T06:35:02Z

It's so interesting that these compiler tools are visible in a standard git-bash shell. When I tried it, no PATH was set and no development tool was available. Maybe something is different when installing the whole package like seen in the screenshot.

NobodyXu · 2023-07-11T07:03:03Z

It's so interesting that these compiler tools are visible in a standard git-bash shell. When I tried it, no PATH was set and no development tool was available. Maybe something is different when installing the whole package like seen in the screenshot.

Yeah it look like an env with patched bash and there're a few catch with it:

if you run a built-in shell function, says mktemp, it will return a path looks like unix (e.g. /tmp/...)
if you pass it to other built-ins, like cd, then it will be handled correctly
if you pass it to other external tools, like cargo-binstall then it malfunctions as it cannot find the path

BTW, the windows CI on GHA demonstrates the same behaviour, so I suppose they might be also using the Git Bash.

Byron · 2023-09-04T07:02:37Z

Did anything change now that #1007 is merged? To me it looks like it all should work now, but I am definitely not sure. Thanks for chiming in.

NobodyXu · 2023-09-04T08:12:12Z

Thanks, I will try it again in cargo-binstall later

RivenSkaye · 2024-05-21T06:48:21Z

The current upstream fix in place throws a blanket compile error on all windows targets, rather than just with MSVC. This causes MSYS2/{MingW,Clang}-{32,64} environments to fail the build, despite not even touching the compiler that breaks the build. This is currently also preventing builds of dependents (such as starship in my case). I'll also open an issue over at the asm-hashes repo, but this should be noted so it can (temporarily) be disabled as dep in gix-features as well.

Scratch that, turns out it doesn't produce correct results on Windows regardless of the toolchain

Byron · 2024-05-21T06:58:24Z

Just to keep note, I ran into the issue described above and quick-fixed it by building the pure Rust version of gitoxide instead (see 52f203a).

An actual fix would now be in order to prevent the asm hashes to be enabled on Windows at all, something that would have to happen in the gix-features/Cargo.toml manifest. CC @EliahKagan

I am planning to create a new release today or tomorrow and definitely hope to get this issue fixed there as well.

RivenSkaye · 2024-05-21T07:33:53Z

The new release to fix this issue could simply change the msvc blocker on the asm feature to anything Windows. A single line fix, that wouldn't break any existing environment since it already doesn't build for Windows unless using MSVC.

That said, I wonder why no attention was paid to explicit warnings, or why it wasn't made opt-in (or out) instead. Perhaps an oversight or forgetting Windows is the special kid on the block?

Looking at the upstream RustCrypto/hashes repo it seems like the current release candidate is removing the feature entirely so this should not be a problem in the near future. I've run into this before, and the easiest workaround is building with MSVC, but special-casing a few builds because of a mismatch in cfg directives just feels off to me.

Byron · 2024-05-21T07:42:23Z

Is this one-line change something you could contribute (and ideally verify)? It seems a build-test is missing on Windows as well that could catch this, as right now the only time MSVC is built is during installation (which was neutered in 52f203a).

I am quite afraid of meddling with this as I remember it as quite painful to deal with, over time, so any help here is greatly appreciated.

RivenSkaye · 2024-05-21T08:48:26Z

Okay latest master doesn't seem to build, I'll grab the latest tag instead
All occurrences of the asm feature (only 1!):

cargo build --locked (my default toolchain is nightly-x86_64-pc-windows-gnu): fails to build time, including when I pin to the exact version in toml, or when I run frozen, or omit anything and ignore the lockfile.

error[E0282]: type annotations needed for `Box<_>`
  --> C:/msys64/home/martin/.rust/.cargo\registry\src\index.crates.io-6f17d22bba15001f\time-0.3.31\src\format_description\parse\mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items
   |              ++++++++

Running a cargo update fixes it, but it does give me pause as to what CI is doing differently that it doesn't break.

post-update cargo build --release:

The fix I was talking about (target_env = "msvc" -> target_os = "windows" on gix-features/Cargo.toml L162):

post-fix cargo build --release:

2 non-local impl warnings, but a perfectly successful and functional build. This change will not break any existing code or dependencies, and it will fix dependents being built with anything that isn't MSVC. It's a tiny fix, a huge net win, and not worth having my name in the contributor list. Especially considering the next sha1 bump will wipe this fix off the table anyways.

Byron · 2024-05-21T08:53:49Z

Thanks so much!

I will apply this change right away.

As for the compile-errors, I actually don't know what CI does differently. It all relies on lock-files, of course, but from what I can tell you did the same.

It's a good hint at updating those dependencies before making a release.

Especially considering the next sha1 bump will wipe this fix off the table anyways.

I didn't plan to upgrade as the new version seems to be slower in my tests, I don't think it has assembly anymore.

RivenSkaye · 2024-05-21T09:31:01Z

It seems to be using SHA hardware capabilities available on all consumer CPUs since Intel Goldmont(E-cores)/Ice Lake(normal cores) or AMD Zen1. Is the performance hit there as well, or only on older systems? Would be interesting if manual ASM was faster than a dedicated impl

Byron · 2024-05-21T14:01:50Z

Maybe I am just unlucky as I tested it on M1 only - there it was definitely slower, and too slow for gitoxide to be competitive, unfortunately.

Probably over time that will change. Maybe there should be more conditional compilation at some point, using the ASM version on MacOS and the latest one on other platforms.

…917)

NobodyXu mentioned this issue Jun 29, 2023

speedup git shallow cloning: Enable gix/max-performance cargo-bins/cargo-binstall#1186

Merged

NobodyXu mentioned this issue Jul 6, 2023

Enable gix/max-performance for more targets cargo-bins/cargo-binstall#1190

Closed

NobodyXu mentioned this issue Jul 9, 2023

Fix deprecated use of cc::Build::compile in build.rs RustCrypto/asm-hashes#59

Merged

Byron added the feedback requested label Sep 4, 2023

RivenSkaye mentioned this issue May 21, 2024

Bump gitoxide version to re-enable windows-gnu builds starship/starship#5984

Closed

Byron added a commit that referenced this issue May 22, 2024

fix: assure high-speed SHA1 assembly is only used in not on Windows (#…

2683235

…917)

Byron added acknowledged an issue is accepted as shortcoming to be fixed and removed feedback requested labels May 22, 2024

Byron mentioned this issue May 22, 2024

Various fixes #1374

Merged

3 tasks

Byron linked a pull request May 22, 2024 that will close this issue

Various fixes #1374

Merged

3 tasks

Byron closed this as completed in #1374 May 22, 2024

feature sha1/asm is enabled on windows #917

feature sha1/asm is enabled on windows #917

Comments

NobodyXu commented Jun 29, 2023

Duplicates

Current behavior 😯

Expected behavior 🤔

Steps to reproduce 🕹

Byron commented Jun 29, 2023

NobodyXu commented Jun 30, 2023

NobodyXu commented Jun 30, 2023

NobodyXu commented Jun 30, 2023 • edited

Byron commented Jun 30, 2023

NobodyXu commented Jun 30, 2023

NobodyXu commented Jun 30, 2023

Byron commented Jun 30, 2023

NobodyXu commented Jun 30, 2023

NobodyXu commented Jun 30, 2023

NobodyXu commented Jul 1, 2023

NobodyXu commented Jul 1, 2023 • edited

Byron commented Jul 1, 2023

NobodyXu commented Jul 6, 2023

Byron commented Jul 6, 2023

NobodyXu commented Jul 6, 2023

NobodyXu commented Jul 9, 2023

NobodyXu commented Jul 9, 2023

Byron commented Jul 10, 2023

NobodyXu commented Jul 10, 2023

Byron commented Jul 10, 2023

NobodyXu commented Jul 11, 2023 • edited

NobodyXu commented Jul 11, 2023

Byron commented Jul 11, 2023

NobodyXu commented Jul 11, 2023

Byron commented Sep 4, 2023

NobodyXu commented Sep 4, 2023

RivenSkaye commented May 21, 2024 • edited

Byron commented May 21, 2024

RivenSkaye commented May 21, 2024

Byron commented May 21, 2024

RivenSkaye commented May 21, 2024 • edited

Byron commented May 21, 2024 • edited

RivenSkaye commented May 21, 2024 • edited

Byron commented May 21, 2024

feature `sha1/asm` is enabled on windows #917

feature `sha1/asm` is enabled on windows #917

NobodyXu commented Jun 30, 2023 •

edited

NobodyXu commented Jul 1, 2023 •

edited

NobodyXu commented Jul 11, 2023 •

edited

RivenSkaye commented May 21, 2024 •

edited

RivenSkaye commented May 21, 2024 •

edited

Byron commented May 21, 2024 •

edited

RivenSkaye commented May 21, 2024 •

edited