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

Relax the build target checks in windows-targets sub-crates #2774

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

seritools
Copy link
Contributor

I've done this change for Rust9x initially, but realized that the recently-added tier3 win7 targets also fail to link with the strict check in the target crates:

> cargo +nightly build --target x86_64-win7-windows-msvc -Zbuild-std
[...]

error: linking with `link.exe` failed: exit code: 1181
  |
  = note: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Preview\\VC\\Tools\\MSVC\\14.39.33321\\bin\\HostX64\\x64\\link.exe" "/NOLOGO" [...]
  = note: LINK : fatal error LNK1181: cannot open input file 'windows.0.52.0.lib'

This change aligns the checks for the msvc targets with the checks in crates\libs\targets\Cargo.toml.
I didn't touch the gnu/gnullvm ones since I'm unclear about target_abi actually being stable/checkable yet. LMK if I should add the respective checks.

Also please feel free to close if this doesn't align with windows-rs. For example, technically it could be interpreted as support for unsupported target vendors.

CC @roblabla

This allows e.g. x86_64-win7-windows-msvc and other alternative vendors to link.
Copy link
Collaborator

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

Being less sensitive to the precise target name seems like it might lessen churn in the future. Obviously win9x support is off-label use of this crate but if it works then fair enough.

crates/targets/aarch64_msvc/build.rs Show resolved Hide resolved
@ChrisDenton
Copy link
Collaborator

My only other comment is that if this is worth doing then it's worth doing for all the target crates, not just the msvc ones.

@seritools
Copy link
Contributor Author

seritools commented Jan 4, 2024

Agreed, just wasn't sure about the target_abi (for gnu vs gnullvm) checks. If someone could chime in there I'll update it asap!

@ChrisDenton
Copy link
Collaborator

Ah, unfortunately cfg_target_abi is not stable yet rust-lang/rust#80970

So it'd need to be parsed from the target string.

@seritools
Copy link
Contributor Author

seritools commented Jan 4, 2024

That's what I thought as well, but wasn't sure anymore after seeing

[target.'cfg(all(target_arch = "x86_64", target_env = "gnu", not(target_abi = "llvm"), not(windows_raw_dylib)))'.dependencies]

Anyways, would a check for .ends_width("-gnu")/.ends_width("-gnullvm") be good enough for now, then?

@seritools
Copy link
Contributor Author

I went ahead and added those changes as a separate commit 👍

@seritools seritools force-pushed the relax-target-checks branch from 3c5c3d5 to 01b0ae0 Compare January 4, 2024 18:10
@kennykerr
Copy link
Collaborator

It's quite a mess since (a) cfg_target_abi isn't stable and (b) gnu/llvm in general seems to be quite a mess. (#2515).

@ChrisDenton
Copy link
Collaborator

gnu/llvm is potentially getting promoted to a tier 2 target soon so I'll try to push for stabilization of target_abi.

@riverar
Copy link
Collaborator

riverar commented Jan 4, 2024

Yikes, these conditionals are hard to review without loading up a bunch of toolchain context. If the tests pass, that's great. But it would be helpful to see table of supported targets and the attributes being used to identify them to understand the composition. (Not a blocker here.)

@kennykerr
Copy link
Collaborator

Documented here: https://doc.rust-lang.org/cargo/reference/environment-variables.html

Be good to have some targeted yml workflows that verify each target.

@kennykerr kennykerr merged commit 7219668 into microsoft:master Jan 5, 2024
65 checks passed
roblabla pushed a commit to roblabla/windows-rs that referenced this pull request Jan 5, 2024
roblabla pushed a commit to roblabla/windows-rs that referenced this pull request Jan 5, 2024
@seritools seritools deleted the relax-target-checks branch January 5, 2024 15:22
roblabla pushed a commit to roblabla/windows-rs that referenced this pull request Feb 9, 2024
@evmar
Copy link

evmar commented Apr 18, 2024

Another positive consequence of this change is that you can now use windows-sys with i586-pc-windows-msvc, where before it would filter specifically on i686.

(I discovered this PR because I was trying to figure out why the released windows-sys didn't work for me even though the code at Git head did...)

Hagb added a commit to Hagb/giuroll that referenced this pull request Jun 26, 2024
The panic hook is to show the panic payload with a message box.

Updating `windows` to 0.53.0 is to fix the build with
`i686-win7-msvc-windows` target, which provides binaries with Windows7
support. (microsoft/windows-rs#2774)
Guiguiprim pushed a commit to JustRustThings/windows-rs that referenced this pull request Aug 6, 2024
Hagb added a commit to Hagb/giuroll that referenced this pull request Oct 2, 2024
The panic hook is to show the panic payload with a message box.

Updating `windows` to 0.53.0 is to fix the build with
`i686-win7-msvc-windows` target, which provides binaries with Windows7
support. (microsoft/windows-rs#2774)
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