-
Notifications
You must be signed in to change notification settings - Fork 349
Put 64-bit MSBuild on PATH #971
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
Conversation
mthalman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This failed the build due to the Dockerfile templates not being updated. See https://github.com/microsoft/dotnet-framework-docker/blob/main/CONTRIBUTING.md#editing-dockerfiles.
Specifically:
| + \";${Env:ProgramFiles(x86)}\Microsoft Visual Studio\2022\BuildTools\MSBuild\Current\Bin\" ` |
and
| + \";${Env:ProgramFiles(x86)}\Microsoft Visual Studio\2022\BuildTools\MSBuild\Current\Bin\" ` |
That will ensure that the changes are consistently applied to all relevant Dockerfiles.
| + \";${Env:ProgramFiles}\NuGet\" ` | ||
| + \";${Env:ProgramFiles(x86)}\Microsoft Visual Studio\2022\TestAgent\Common7\IDE\CommonExtensions\Microsoft\TestWindow\" ` | ||
| + \";${Env:ProgramFiles(x86)}\Microsoft Visual Studio\2022\BuildTools\MSBuild\Current\Bin\" ` | ||
| + \";${Env:ProgramFiles(x86)}\Microsoft Visual Studio\2022\BuildTools\MSBuild\Current\Bin\arm64\" ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo? Were you intending to use amd64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to keep things spicy for the 2016 folks 🤦🏻♂️
|
Is there more context around the discovery of this that can be linked to? |
|
Are there scenarios that are broken by default by not having the PATH set to amd64? |
|
FWIW we hit this internally - everywhere was using MSBuild 64-bit except official builds using this image which wasn't an issue until another fix actually changed a Task DLL from MSIL to AMD64 alerting us to the difference. That's when we found out all other builds, local sec, etc. are 64-bit but docker image is firing up the 32-bit path. Practically, I'd want the image to match all other build experiences and would call the current behavior unexpected! |
|
Our policy should and has been to match VS, so this change looks good. We'll have to make an announcement for it. |
Visual Studio 2022 changed the "Developer Command Prompt for VS 2022" PATH setting to reference the 64-bit copy of MSBuild folder, corresponding with the change to make devenv.exe/Visual Studio itself a 64-bit process. While both versions of MSBuild can build most projects, the 64-bit copy is now used by default in more places and is a better default.
The basic scenario is that a (very slightly) misauthored build can work in VS and the VS desktop command line but fail in a Docker-based build, like @NickCraver's team hit. The flip side is that if a build is (slightly) misauthored the other direction, it might work only in a 32-bit MSBuild--that's "reference a 32-bit-only task but don't say it's 32-bit-only". However since the change to the Docker images is coming well after the VS change itself, hopefully those builds have been fixed already. https://devblogs.microsoft.com/dotnet/msbuild-and-64-bit-visual-studio-2022/ has symptoms and mitigations for the problems folks are likely to encounter and hopefully has enough search-engine juice that any issues can be quickly resolved. I do tend to agree that the strongest argument to take this is "consistency". |
|
FWIW, we hit another set of issues ultimately do to this today (ASPNETCOMPILER pathing being 32-bit as a downstream result). What can we to do help get this into an image as soon as possible? |
We release updates from this repo on a monthly basis. This change will be rolled out next Tuesday. See #973. |
|
Ah awesome, thanks for the update! I guess I was confused with this not being merged yet - does everything to merged in and upstreamed around the same time with the process here? |
Yes. This isn't a very active repo so we only really maintain a single branch. The main branch is kept "clean" until release day to allow us to release any unexpected hotfixes if necessary. So that's all to say that this will get merged Tuesday morning. |
|
Great to know for the future - I appreciate the additional detail :) Thanks for helping us out here! |
Visual Studio 2022 changed the "Developer Command Prompt for VS 2022" PATH setting to reference the 64-bit copy of MSBuild folder, corresponding with the change to make devenv.exe/Visual Studio itself a 64-bit process.
I think the Docker container experience should match, but there is some risk here: some projects that build with a 32-bit MSBuild will fail with a 64-bit one. Those projects won't build on the standard VS command line, so there should be very few, but it's worth considering the impact (and I don't have a great way to quantify either way).