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

Find dotnet.exe instead of <name>.exe when building out-of-proc using MSBuildLocator #6890

Merged
merged 3 commits into from
Sep 30, 2021

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Sep 24, 2021

Fixes #6782

Context

When using MSBuildLocator, the first process you start is named after your app, not dotnet or msbuild, yet it still loads MSBuild assemblies. When it then starts an out-of-proc build on core, it tries to use the current host, which is normally dotnet.exe, but in this case is .exe. MSBuild can't find it to connect, so it assumes it launches another node. It should stop after that, but it just keeps going and makes hundreds or thousands of nodes until your computer crashes. That is bad. This defaults to looking for a nearby dotnet.exe (if you're on core) and uses that instead, falling back to .exe only on failure. This should handle any case in which you find MSBuild in an sdk installation.

Changes Made

Look for a dotnet.exe instead of your current host.

Testing

Broke repro.

Notes

Do not try to reproduce this naively. Your computer will be unhappy.

Also, I suspect this may have been the cause of my 9/10 comment here (internal link).

exeName = GetCurrentHost();
commandLineArgs = "\"" + msbuildLocation + "\" " + commandLineArgs;
string dotnetExe = Path.Combine(FileUtilities.GetFolderAbove(exeName, 2), "dotnet.exe");
exeName = File.Exists(dotnetExe) ? dotnetExe : GetCurrentHost();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to push this into GetCurrentHost() so we only have to do the file-exists check once?

@@ -480,8 +479,8 @@ private Process LaunchNode(string msbuildLocation, string commandLineArgs)
if (!NativeMethodsShared.IsMono)
{
// Run the child process with the same host as the currently-running process.
exeName = GetCurrentHost();
commandLineArgs = "\"" + msbuildLocation + "\" " + commandLineArgs;
string dotnetExe = Path.Combine(FileUtilities.GetFolderAbove(exeName, 2), "dotnet.exe");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot going on in this method:

  • On Windows, it uses native methods to launch the process so the path to MSBuild.exe needs to be prepended to the command-line arguments. @rainersigwald do you know why we use a different method to launch the process? The code doesn't seem to need to as far as I can tell. Maybe we can get rid of it?
  • For .NET Core, the command-line arguments must be prepended with the the path to MSBuild.dll and the EXE is dotnet.

I see some missing logic in the current implementation:

  • On Windows, its dotnet.exe, on non-Windows its just dotnet
  • The path to MSBuild.dll is prepended twice on Windows (which technically works but sees dirty)
  • GetCurrentHost() is only ever correct if you run dotnet directly so using it as a fallback seems not great

I would do the following:

  1. Make a method like GetNodeExe() that has all of the logic to find the path to the executable that represents the node, so MSBuild.exe on .NET Framework, dotnet.exe or dotnet for .NET Core.
  2. Make a method like GetNodeArguments() that adjusts the command-line arguments
    Prepend MSBuild.exe on Windows since its using native APIs
    Prepend MSBuild.dll on .NET Core

In my opinion that would simplify this method and make it easier to rationalize what its doing. You could also make these two methods internal and write a unit test to verify they return the correct values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as falling back to GetCurrentHost, I agree it isn't terribly likely to succeed, but I don't think we have a better option. There's a chance someone has fancy logic in their project to make it work, and if we don't fall back to GetCurrentHost, we would break that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure if you don't fall back to current-host our own dogfood builds will fail (they're invoked via dotnet.exe path\to\some\msbuild.dll).

Copy link
Contributor

@jeffkl jeffkl Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCurrentHost() is only ever correct if you run dotnet directly so using it as a fallback seems not great

Sorry what I meant was having this method here only to use as a fallback, when the entirety of the logic could live inside of it. I did not mean to get rid of the fallback all together. To have solid logic here that then falls back to a method seems clunky is all.

@@ -466,8 +465,8 @@ private Process LaunchNode(string msbuildLocation, string commandLineArgs)
creationFlags |= BackendNativeMethods.CREATE_NEW_CONSOLE;
}

BackendNativeMethods.SECURITY_ATTRIBUTES processSecurityAttributes = new BackendNativeMethods.SECURITY_ATTRIBUTES();
BackendNativeMethods.SECURITY_ATTRIBUTES threadSecurityAttributes = new BackendNativeMethods.SECURITY_ATTRIBUTES();
BackendNativeMethods.SECURITY_ATTRIBUTES processSecurityAttributes = new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be moved into the else block below where its actually used, unless we get rid of the different ways of launching the process in which case this can just go away.

@rainersigwald rainersigwald added this to the MSBuild 17.0 milestone Sep 29, 2021
@benvillalobos benvillalobos 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 Sep 29, 2021
@benvillalobos benvillalobos merged commit bbcce1d into dotnet:main Sep 30, 2021
rainersigwald pushed a commit to microsoft/MSBuildLocator that referenced this pull request Dec 1, 2021
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 <[email protected]>
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.

Out-of-proc builds in a custom .NET Core process are not possible
4 participants