Skip to content

Identify 64-bit MSBuildToolsPath from 64-bit API consumer#6683

Merged
ladipro merged 5 commits into
dotnet:mainfrom
rainersigwald:64-bit-environment
Jul 28, 2021
Merged

Identify 64-bit MSBuildToolsPath from 64-bit API consumer#6683
ladipro merged 5 commits into
dotnet:mainfrom
rainersigwald:64-bit-environment

Conversation

@rainersigwald
Copy link
Copy Markdown
Member

Fixes #6681.

Context

The change to make Visual Studio 64-bit means that projects loaded in Visual Studio get the 64-bit MSBuild, and other tools are more likely to be loading our API in a 64-bit process as well. That found this difference between the way projects are evaluated in devenv.exe and in vcxprojReader.exe.

Changes Made

Find the VS root from the current assembly and then reconstruct the path to the appropriate MSBuild.exe based on that + the current process's bitness, ensuring that API consumers and MSBuild.exe/devenv.exe see the same MSBuildToolsPath.

Testing

Manual overlay with a trivial API consumer.

@rainersigwald
Copy link
Copy Markdown
Member Author

Hmm. The failure in FindOlderVisualStudioEnvironmentByEnvironmentVariable is interesting. Do we care about supporting environment variables with paths that are sufficiently old that they still have a version number in them? That's two major versions behind, and I think "no". We've been telling people to use MSBuildLocator for roughly all of the 15 and 16 timeframe and "load the MSBuild that comes with the environment you want to build in for all of those cycles. I'm going to just delete the test but this is up for debate if someone wants to argue.

@cdmihai
Copy link
Copy Markdown
Contributor

cdmihai commented Jul 16, 2021

Hmm. The failure in FindOlderVisualStudioEnvironmentByEnvironmentVariable is interesting. Do we care about supporting environment variables with paths that are sufficiently old that they still have a version number in them? That's two major versions behind, and I think "no". We've been telling people to use MSBuildLocator for roughly all of the 15 and 16 timeframe and "load the MSBuild that comes with the environment you want to build in for all of those cycles. I'm going to just delete the test but this is up for debate if someone wants to argue.

Maybe a new PR that removes the concept from MSBuild altogether?

Comment thread src/Shared/BuildEnvironmentHelper.cs Outdated
Comment thread src/Shared/BuildEnvironmentHelper.cs Outdated
@ladipro
Copy link
Copy Markdown
Member

ladipro commented Jul 27, 2021

In the latest version of the PR, how are you making sure that the IntPtr.Size == 8 check runs in the non-VS hosted scenario? Apologies if this is a stupid question, I am just not seeing it with my tired eyes 😫

@rainersigwald
Copy link
Copy Markdown
Member Author

Apologies if this is a stupid question, I am just not seeing it with my tired eyes

Your tired eyes are better than my midafternoon eyes, it seems. Reverting that change.

Also eliminates the unspecified need to only pass in MSBuild.exe as the
assembly.
Fixes dotnet#6681 by finding the VS root from the current assembly and then
reconstructing the path to the appropriate MSBuild.exe based on that +
the current process's bitness, ensuring that API consumers and
MSBuild.exe/devenv.exe see the same MSBuildToolsPath.
The new process to find MSBuild cares only about the real runtime architecture
of the running process, not the path passed in, so these tests no longer
successfully simulate the 64-bit case. Dropping them instead.
I can't think of a reason to want to attempt to load (part of) an MSBuild 15.0 toolset with MSBuild 17.0+.
@ladipro ladipro added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jul 28, 2021
@ladipro ladipro merged commit d592862 into dotnet:main Jul 28, 2021
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Aug 9, 2021
A 64-bit application is now (after dotnet#6683) getting a good VS root path,
but may still use toolsets from the wrong directory, because if it
loaded (AnyCPU) assemblies from the 'x86' location it wouldn't pass
a path containing �md64 to the BuildEnvironment constructor, so
the logic there wouldn't find the right MSBuildToolsDirectory and
config file.

Fix this in the lowest-impact way by rederiving the path to the
'correct' MSBuild.exe from the VS root and passing that to the
BuildEnvironment constructor.
rokonec pushed a commit that referenced this pull request Aug 11, 2021
A 64-bit application is now (after #6683) getting a good VS root path,
but may still use toolsets from the wrong directory, because if it
loaded (AnyCPU) assemblies from the 'x86' location it wouldn't pass
a path containing �md64 to the BuildEnvironment constructor, so
the logic there wouldn't find the right MSBuildToolsDirectory and
config file.

Fix this in the lowest-impact way by rederiving the path to the
'correct' MSBuild.exe from the VS root and passing that to the
BuildEnvironment constructor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

64-bit API clients can wind up with imports that don't match MSBuild.exe/VS's

4 participants