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.

Sign up for GitHub

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

Mono: Improve MSBuildFinder logic on Windows #41375

Merged
merged 1 commit into from
Aug 29, 2020
Merged

Mono: Improve MSBuildFinder logic on Windows #41375

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,21 @@ private static string FindMsBuildToolsPathOnWindows()

// Try to find 15.0 with vswhere

string vsWherePath = Environment.GetEnvironmentVariable(Internal.GodotIs32Bits() ? "ProgramFiles" : "ProgramFiles(x86)");
vsWherePath += "\\Microsoft Visual Studio\\Installer\\vswhere.exe";
var envNames = Internal.GodotIs32Bits() ? new[] { "ProgramFiles", "ProgramW6432" } : new[] { "ProgramFiles(x86)", "ProgramFiles" };
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to check the matching architecture's path first? This would mean that the "ProgramFiles" environment variable is checked first, since for a 32-bit process it points to 32-bit Program Files, and for a 64-bit process it points to 64-bit Program Files.

Suggested change
var envNames = Internal.GodotIs32Bits() ? new[] { "ProgramFiles", "ProgramW6432" } : new[] { "ProgramFiles(x86)", "ProgramFiles" };
var envNames = Internal.GodotIs32Bits() ? new[] { "ProgramFiles", "ProgramW6432" } : new[] { "ProgramFiles", "ProgramFiles(x86)" };

We might be able to optimize this further by only changing the fallback environment variable. I haven't compiled this but I wonder if this works:

Suggested change
var envNames = Internal.GodotIs32Bits() ? new[] { "ProgramFiles", "ProgramW6432" } : new[] { "ProgramFiles(x86)", "ProgramFiles" };
var envNames = new[] { "ProgramFiles", Internal.GodotIs32Bits() ? "ProgramW6432" : "ProgramFiles(x86)" };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I have heard is, in windows 64 bit, Visual Studio Installer most likely be installed in ProgramFiles(x86) by default. Only me or some people that change the default path of installer would met this problem. So I think it would be more efficient to ordering this way

Copy link
Member

@aaronfranke aaronfranke Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I'm just thinking of future proofing it, in case there is a 64-bit Visual Studio Installer in the future and then it would be placed in the 64-bit folder instead (and if both exist, 64-bit Godot would prefer the 64-bit one). I'll let @neikeq decide what's best here.


string vsWherePath = null;
foreach (var envName in envNames)
{
vsWherePath = Environment.GetEnvironmentVariable(envName);
if (!string.IsNullOrEmpty(vsWherePath))
{
vsWherePath += "\\Microsoft Visual Studio\\Installer\\vswhere.exe";
Copy link
Member

Choose a reason for hiding this comment

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

Not directly about this PR's changes, but can we just use / for the directory separator on Windows in the C# code? Windows has supported / as a directory separator for decades now, and I think all of the tooling that Godot uses supports it.

if (File.Exists(vsWherePath))
break;
}

vsWherePath = null;
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this is reached? What's the result of Godot.OS.Execute(null, ...)? @neikeq

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same as passing an empty string. OS.execute may print an error. Albeit to be fair the old code didn't check if vsWherePath existed in the file system either.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will there be any possible chance that someone would install their vs installer outside of ProgramFiles and just directly in the drive though?

}

var vsWhereArgs = new[] {"-latest", "-products", "*", "-requires", "Microsoft.Component.MSBuild"};

Expand Down