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

fix finding toolchains when invoked by msbuild #1201

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

russelltg
Copy link
Contributor

As noted in #1189 (comment), when invoking msbuild from the commandline VisualStudioDir is not set. VSTEL_MSBuildProjectFullPath does seem to be set reliably, even if it's undocumented....

I'm open to alternative fixes here as well. It's possible that we should check that the value of it ends in .vcxproj as if it's a C# library or something it may not have the proper environment set (I'm rather unsure).

@NobodyXu
Copy link
Collaborator

NobodyXu commented Sep 6, 2024

cc @ChrisDenton could you take a look at this PR please?

@russelltg
Copy link
Contributor Author

Another note is that VSTEL_MSBuildProjectFullPath is also set by VS, so I suppose we could remove VisualStudioDir and just use VSTEL_MSBuildProjectFullPath instead

@ChrisDenton
Copy link
Member

Another note is that VSTEL_MSBuildProjectFullPath is also set by VS, so I suppose we could remove VisualStudioDir and just use VSTEL_MSBuildProjectFullPath instead

This would be my preference. I think this should be kept as simple as possible.

Though I wonder, is the VsInstallRoot property not exposed as an environment variable?

@russelltg
Copy link
Contributor Author

Doesn't seem like it. I dumped the env here #1189 (comment)

@ChrisDenton
Copy link
Member

Fair enough. So I'd agree using VSTEL_MSBuildProjectFullPath is the best option we have.

@russelltg russelltg force-pushed the msbuild_from_cmdline branch from 8bb266c to 3b58faf Compare September 6, 2024 15:59
@NobodyXu NobodyXu merged commit e4a1a88 into rust-lang:main Sep 6, 2024
26 checks passed
@NobodyXu
Copy link
Collaborator

NobodyXu commented Sep 6, 2024

Thank you russelltg ChrisDenton !

@github-actions github-actions bot mentioned this pull request Sep 6, 2024
@dlon
Copy link
Contributor

dlon commented Nov 30, 2024

We're running cargo build from the Pre-Build Events in a Visual Studio project, using msbuild, and we're having linker issues since Rust 1.83. It seems to be caused by this check that VSTEL_MSBuildProjectFullPath set, introduced in this PR.

The problem arises when we build for a different target than the host (e.g., using msbuild.exe myproj.sln /p:Platform=otherarch). When cargo builds a build script, it uses the target linker (added to PATH by Visual Studio) instead of the host linker, which naturally fails.

Is this expected behavior?

@NobodyXu
Copy link
Collaborator

Hmmm...not exactly sure, cc @ChrisDenton @russelltg any thoughts on this?

@ChrisDenton
Copy link
Member

Yes it was intentional. Many people using external build systems want the build system to be in full control of selecting the tools to use, etc. However, it's also clear that many other people were relying on us to automatically correct any tool mismatches. These two things are hard to reconcile, especially with MSBuild as it does not give us any information to go on. So we'd either need to not trust the environment (the previous behaviour) or else try to find another way to tell if there's a mismatch between targets.

Off the top of my head I can think of a few things we could try:

  • Run cl.exe and read the version line to see what the compiler is targetting. This should be something like: "Microsoft (R) C/C++ Optimizing Compiler Version 19.42.34433 for ARM64".
  • Inspect the path to cl.exe and parse out the target. It should end with something like bin\Hostx64\arm64\cl.exe.

I've not yet investigated or tested either of these options though. It would be good to have an issue rather than a closed PR.

@ChrisDenton
Copy link
Member

ChrisDenton commented Dec 1, 2024

Thinking about it some more, part of the problem here is that cc is both a dependency of rustc (for finding link.exe + libraries) and a tool to use in build scripts. So another option would be to have some way for rustc to disable this behaviour for rustc only. Though I've not thought through the implications of that.

EDIT: Ideally there would be a way to know if Cargo is building a build script.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Dec 1, 2024

So another option would be to have some way for rustc to disable this behaviour for rustc only.

Yeah I think about that too, build-script are quite special in that it's always built for native.

Maybe we can create another function that takes the target to build for?

Oh we already take the target, so

Run cl.exe and read the version line to see what the compiler is targetting. This should be something like: "Microsoft (R) C/C++ Optimizing Compiler Version 19.42.34433 for ARM64".

O think this makes sense

@dlon
Copy link
Contributor

dlon commented Dec 1, 2024

I've created an issue for this here: #1308

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.

4 participants