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.

Sign up for GitHub

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

Jump to bottom

Compiling from msbuild incorrectly uses target build tools for build scripts #1308

Closed
dlon opened this issue Dec 1, 2024 · 12 comments · Fixed by #1310
Closed

Compiling from msbuild incorrectly uses target build tools for build scripts #1308

dlon opened this issue Dec 1, 2024 · 12 comments · Fixed by #1310
Labels
O-windows Windows targets and toolchains

Comments

@dlon
Copy link
Contributor

dlon commented Dec 1, 2024 •
edited
Loading

The problem arises when running cargo build from within a VS project and targeting something other than the native arch (e.g., using msbuild.exe myproj.sln /p:Platform=otherarch). When cargo builds a build script, it uses the target tools (added to PATH by Visual Studio) instead of the host tools, which naturally fails.

This is a problem as of Rust 1.83 due to the following fix: #1201. That PR also contains some discussion about this issue.
@dlon dlon mentioned this issue Dec 1, 2024
Merged
@dlon
Copy link
Contributor Author

dlon commented Dec 1, 2024 •
edited
Loading

@NobodyXu @ChrisDenton Would you accept a PR based on your suggestion of parsing the output of cl.exe? :)

The odd thing about it is that the msbuild tools will be used for the native target build scripts, but not otherwise. That's a bit counter-intuitive, perhaps.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Dec 2, 2024

Yes I'm willing to review and merge it

@NobodyXu
Copy link
Collaborator

NobodyXu commented Dec 2, 2024

The odd thing about it is that the msbuild tools will be used for the native target build scripts, but not otherwise. That's a bit counter-intuitive, perhaps.

Well cc doesn't link anything, only compile and then archive.

@ChrisDenton
Copy link
Member

ChrisDenton commented Dec 2, 2024 •
edited
Loading

Looking at the code, for Visual Studio proper (e.g. after vcvars.bat) we use VSCMD_ARG_TGT_ARCH to verify the environment is set up for the target and then VSINSTALLDIR to find the path to Visual Studio, which we then use to set the toolchain to the right target:

cc-rs/src/windows/find_tools.rs

Lines 390 to 393 in 5daf14e

if is_vscmd_target(target, env_getter) == Some(false) {
// We will only get here with versions 15+.
let vs_install_dir: PathBuf = env_getter.get_env("VSINSTALLDIR")?.into();
tool_from_vs15plus_instance(tool, target, &vs_install_dir, env_getter)

cc-rs/src/windows/find_tools.rs

Lines 358 to 370 in 5daf14e

fn is_vscmd_target(target: TargetArch<'_>, env_getter: &dyn EnvGetter) -> Option<bool> {
let vscmd_arch = env_getter.get_env("VSCMD_ARG_TGT_ARCH")?;
// Convert the Rust target arch to its VS arch equivalent.
let arch = match target.into() {
"x86_64" => "x64",
"aarch64" | "arm64ec" => "arm64",
"i686" | "i586" => "x86",
"thumbv7a" => "arm",
// An unrecognized arch.
_ => return Some(false),
};
Some(vscmd_arch.as_ref() == arch)
}

MSBuild does not set either of those nor does it set anything equivalent. For VSCMD_ARG_TGT_ARCH we can instead parse the header of cl.exe. For VSINSTALLDIR I guess in the case where there's a target mismatch we can instead just return None from find_msvc_environment so that it'll fallback to finding the latest matching tools.

@dlon dlon mentioned this issue Dec 2, 2024
Merged
@NobodyXu
Copy link
Collaborator

NobodyXu commented Dec 2, 2024

For VSCMD_ARG_TGT_ARCH we can instead parse the header of cl.exe.

Hmmm how hard would it be?
Would it be portable?

For VSINSTALLDIR I guess in the case where there's a target mismatch we can instead just return None from find_msvc_environment so that it'll fallback to finding the latest matching tools.

👍

@ChrisDenton
Copy link
Member

Hmmm how hard would it be?

I meant the thing I mentioned earlier about parsing the line that appears at the top of stderr from cl.exe (which cl.exe actually calls the "logo").

@NobodyXu
Copy link
Collaborator

NobodyXu commented Dec 2, 2024 •
edited
Loading

Thanks for clarification that's pretty portable and quite reliable way of doing it, I don't think we have a better way to do it

dlon added a commit to mullvad/mullvadvpn-app that referenced this issue Dec 2, 2024
@dlon
Add workaround for cargo linker issues in MSVC when targeting i686
7135f17 
When running cargo from MSVC, as of Rust 1.83, it uses build tools
for the target MSVC architecture rather than host, when building
build scripts. Unsetting 'VSTEL_MSBuildProjectFullPath' works around
this issue.

This workaround can be removed once the upstream issue has been fixed:
rust-lang/cc-rs#1308
dlon added a commit to mullvad/mullvadvpn-app that referenced this issue Dec 2, 2024
@dlon
Add workaround for cargo linker issues in MSVC when targeting i686
8f63994 
When running cargo from MSVC, as of Rust 1.83, it uses build tools
for the target MSVC architecture rather than host, when building
build scripts. Unsetting 'VSTEL_MSBuildProjectFullPath' works around
this issue.

This workaround can be removed once the upstream issue has been fixed:
rust-lang/cc-rs#1308
@dlon dlon changed the title msbuild uses target linker for build scripts msbuild uses target build tools for build scripts Dec 2, 2024
@dlon dlon mentioned this issue Dec 2, 2024
Merged
@dlon
Copy link
Contributor Author

dlon commented Dec 2, 2024 •
edited
Loading

I've now opened #1310 to fix this issue. Thank you both for the help!

@dlon dlon changed the title msbuild uses target build tools for build scripts Compiling from msbuild incorrectly uses target build tools for build scripts Dec 2, 2024
@dlon dlon mentioned this issue Dec 3, 2024
Closed
@extiop extiop mentioned this issue Dec 16, 2024
Open
@jschwe
Copy link

jschwe commented Jan 3, 2025

Does this issue and the related fix also affect the build of proc macros? And in which Rust version will the fix be available?

Background: I received an issue in corrosion, that when using Visual Studio as the Generator proc macros fail to link as of Rust 1.83 when cross-compiling, unless VSTEL_MSBuildProjectFullPath is unset - which sounds very similar to this issue.

@madsmtm madsmtm added the O-windows Windows targets and toolchains label Jan 3, 2025
@ChrisDenton
Copy link
Member

Proc macros, like build scripts, run using the host compiler so it's possible for them to be affected by this, yes.

The fix will be available in the next Rust release (scheduled for Thursday 09 January). The beta channel could be used to try it early. As we're close to a release it should be mostly release ready (but still, there might be last minute backports that have yet to be applied).

@madsmtm
Copy link
Collaborator

madsmtm commented Jan 3, 2025 •
edited
Loading

I see you've gotten an answer that this isn't fixed on nightly Rust, so might be a different issue. I'd suggest you open a new issue here if you believe it to be an issue with cc

@sevenc-nanashi
Copy link

sevenc-nanashi commented Jan 3, 2025 •
edited
Loading

this isn't fixed on nightly Rust

I haven't updated my nightly.
I updated my nightly and the issue was solved, sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
O-windows Windows targets and toolchains
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve detection of msvc environment when compiling from msvc dlon/cc-rs
6 participants
@dlon @ChrisDenton @madsmtm @NobodyXu @jschwe @sevenc-nanashi