-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
CurrentHost was not properly set when DisableInProcNode = false #7013
Conversation
…bove count, dotnet executable was looked up in sdk subfolder
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.
Thanks 😅
I remember having looked at this in a debugger to see how many folders up, but I assumed it would be constant.
I was going to ask about bitness, but I think this should always be 64-bit, so it isn't an issue.
Actually, I looked again, and I have a dotnet.exe in both |
Sounds like it's weird that I have dotnet.exe in sdk, so this looks good. 👍 |
Ha, that explains it! In that case, feel free to switch to the other approach |
I don't really have a preference between the two, but I can check in our PR review meeting whether anyone else does. |
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.
Thanks @xen2!
@Forgind can we update the version check in Locator to keep specifying the environment for 17.0?
I was going to ask about bitness, but I think this should always be 64-bit, so it isn't an issue.
The .NET SDK has a 32-bit x86 version. However, because MSBuild doesn't support cross-processor-architecture task execution on .NET Core, we don't have an amd64
directory and the position of dotnet.exe
relative to MSBuild.dll
doesn't change in the 32-bit SDK.
Ran exactly into this bug today, I was going mad! Then I debugged and found also the issue, but found that it was also already fixed here, thank you @xen2! 🤗 |
Small world! ;) |
The change in #135 was supposed to be made unnecessary by dotnet/msbuild#6890, but apparently my setup is the only one that has dotnet.exe under both the dotnet folder and the sdks folder, so it didn't work. @xen2 fixed the problem in dotnet/msbuild#7013, but that won't get in 'til 17.1, so we need to prolong the version check here. Co-authored-by: xen2 <virgile.bello@gmail.com>
Fixes #6782
Follow-up of #6890
Context
Tried
DisableInProcNode = false
and couldn't make it work.After some investigations, it turns out the code in
NodeProviderOutOfProcBase.GetCurrentHost()
has some issues:msbuild/src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs
Line 593 in 24b3318
In my case,
CurrentMSBuildToolsDirectory
returnsC:\Program Files\dotnet\sdk\6.0.100\MSBuild.dll
.FileUtilities.GetFolderAbove()
2 levels makes it try to resolvedotnet.exe
fromC:\Program Files\dotnet\sdk\dotnet.exe
instead ofC:\Program Files\dotnet\dotnet.exe
.I believe this is because the filename itself was not taken into account (it counts as 1 extra step for
FileUtilities.GetFolderAbove()
)Changes Made
Used
CurrentMSBuildToolsDirectory
rather thanCurrentMSBuildExePath
.Another option would be to use 3 for
FileUtilities.GetFolderAbove
, but since the function explicitely contains "folder" in its name, I preferred to avoid the case where it is later changed to ignore the top-level file rather than considering it as a folder.Testing
I tested with a simple console app.
(Note: without being careful it could quickly crash the PC as mentioned in #6782 because it calls itself in infinite loop (instead of
dotnet.exe
), unless you add some check onargs.Length
as done in this test code)Notes