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

Avoid using unstable target abi feature #2515

Merged
merged 1 commit into from
May 24, 2023

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented May 22, 2023

What's this all about?

TL;DR windows-rs crates stopped building on windows-gnullvm targets with stable Rust toolchains.
cfg_target_abi is unstable but due to bug in Cargo/Rust this is not detected when specifying target dependencies. Instead it seems to silently do nothing. On nightly this doesn't reproduce, probably because that feature is available for use.
Losing optionality of the target crate when using raw_dylib is unfortunate but having a working build sounds more valuable.

Fixes issue described in #2412 (comment) and msys2/MINGW-packages#16945 (comment)

@@ -24,8 +24,8 @@ windows_i686_gnu = { path = "../../targets/i686_gnu", version = "0.48.0" }
[target.'cfg(all(target_arch = "x86_64", target_env = "gnu", not(target_abi = "llvm"), not(windows_raw_dylib)))'.dependencies]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left not(target_abi = "llvm") because it doesn't break the build and might be necessary one day.

@riverar
Copy link
Collaborator

riverar commented May 23, 2023

Can we devise a test for this that reproduces the issue and demonstrates the problem has been fixed?

@riverar
Copy link
Collaborator

riverar commented May 23, 2023

Perhaps we could together a crate that has a build script that asserts the expected dependencies?

@kennykerr
Copy link
Collaborator

Clearly not well tested as it works before and after this change. 😉

@mati865
Copy link
Contributor Author

mati865 commented May 23, 2023

Regarding the test it's tricky one because Rust/Cargo bug that I mentioned in OP that allows to use cfg_target_abi on nightly inside Cargo.toml (or is it be the design? how would somebody enable it in Cargo.toml?).

To reproduce the problem compiler that forbids unstable features is required, so typically Beta or Stable release.
Since those targets are not avaialbe in rustup in prebuilt form the only compiler I know that can reproduce the issue is AArch64 native binary at MSYS2: https://packages.msys2.org/package/mingw-w64-clang-aarch64-rust?repo=clangarm64
I hope to create x86_64 binary as well but recent LLVM versions are crashing when building it.

It's going to take time but hopefully I'll be able to add prebuilt std for those targets to rustup which would allow testing on stable compilers and I if rust-lang/rust#80970 is not too time consuming to push it to the finish line I'll take care of it.

kennykerr

This comment was marked as outdated.

@kennykerr
Copy link
Collaborator

OK, well this effectively drops gnullvm support for raw-dylib but since gnullvm is still clearly experimental I think that's probably fine. Or rather, it will download the gnullvm target crates even though they aren't needed.

@mati865
Copy link
Contributor Author

mati865 commented May 24, 2023

Or rather, it will download the gnullvm target crates even though they aren't needed.

I believe this is the case actually. I'd consider it as a "minor inconvenience" for Tier 3 target so I haven't spent a lot of time thinking about possible solution.

@riverar
Copy link
Collaborator

riverar commented May 24, 2023

Can we get the manual steps to reproduce this? Should be easy to put into a test afterwards.

@mati865
Copy link
Contributor Author

mati865 commented May 24, 2023

If you can run AArch64 binaries then you can install MSYS2, install prebuilt Rust package (pacman -S mingw-w64-clang-aarch64-clang) and build with it any crate using windows-targets 0.48 like cargo-c.

@jeremyd2019
Copy link
Contributor

Note that's all ephemeral. Hopefully at some point a stable version of rust will be present that does support target_abi usage like this. Also, hopefully before that, this is merged and released and things like cargo-c can start pulling in the new version of this crate that works in the meantime.

As someone who really doesn't use rust, just tries to keep packages building for windows on arm64, I figure this could be reverted once a version of rust is released that properly handles the target_abi usage in Cargo.toml, and is packaged by MSYS2 (since we are apparently the only ones dealing in stable gnullvm targets). MSYS2 is a rolling release, so we aren't concerned about keeping older versions working.

@kennykerr kennykerr merged commit c5ab2c1 into microsoft:master May 24, 2023
@mati865 mati865 deleted the avoid_using_target_abi branch May 24, 2023 22:44
@micolous
Copy link

micolous commented Jun 28, 2023

@kennykerr any chance of a windows-* crate release for this too? This would have been picked up in 0.50.0, but it looks like y'all declined to bump the version of the other crates.

Building packages which use windows-targets on MSYS2's version of aarch64-pc-windows-gnullvm is entirely broken by this bug, and the only alternative is to use aarch64-pc-windows-msvc (which only works on Windows hosts).

I haven't yet got to a point where I'm cross-compiling ARM64 binaries from a non-Windows host; but I'd strongly suspect it's broken there too when using a non-nightly build, using the MSVC toolchain isn't an option, and there's no aarch64-pc-windows-gnu like for x86_64.

It looks like this project's CI only builds -gnullvm with Rust nightly, so entirely side-steps the issue.

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.

5 participants