Mono: Improve MSBuildFinder logic on Windows #41375

Thaina · 2020-08-19T16:21:15Z

Shouldn't 32bit path be x86 ?

akien-mga · 2020-08-19T17:40:05Z

Does this fix something specific, or is it a proofreading change?

If the latter, I think the logic is that "ProgramFiles(x86)" is the name of the 32-bit install path on 64-bit systems, so the current logic is correct.

CC @neikeq

Thaina · 2020-08-19T17:48:15Z

Does this fix something specific, or is it a proofreading change?

If the latter, I think the logic is that "ProgramFiles(x86)" is the name of the 32-bit install path on 64-bit systems, so the current logic is correct.

CC @neikeq

I'm not sure but as for me, My machine is windows x64 but I have no Microsoft Visual Studio\\Installer folder in ProgramFiles(x86). I have install them in ProgramFiles and so godot cannot find vswhere in my machine

When I change it like this then it fixed

If that was your logic then it sound reasonable. I would retract this request

Thaina · 2020-08-19T18:14:37Z

On another thought. I think it would be better if we will always try to find from both place, how about this?

neikeq

I'm surprised about this as I thought this was guaranteed to always be on the 32-bit install location, but I guess that's not the case.
Some corrections are needed though.

neikeq · 2020-08-19T20:38:38Z

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs

+            string vsWherePath = null;
+            foreach(var envName in envNames)
+            {
+                vsWherePath = Environment.GetEnvironmentVariable(Internal.GodotIs32Bits() ? "ProgramFiles(x86)" : "ProgramFiles");


This should be:

Suggested change

vsWherePath = Environment.GetEnvironmentVariable(Internal.GodotIs32Bits() ? "ProgramFiles(x86)" : "ProgramFiles");

vsWherePath = Environment.GetEnvironmentVariable(envName);

neikeq · 2020-08-19T20:39:32Z

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs

@@ -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","ProgramFiles(x86)" } : new[] { "ProgramFiles(x86)","ProgramFiles" };


There's no ProgramFiles(x86) on 32-bit Windows AFAIK:

Suggested change

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

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

neikeq · 2020-08-19T20:48:06Z

Also the code needs to be reformatted to follow the C# style guidelines. Spaces between if/for and the opening parenthesis as well as space after a comma. There may be more but we don't tend to be very strict about this with C# for now (would be too annoying for contributors unless we had a CI check and a commit hook like we do for C++) and I myself tend to forget a few rules sometimes.

akien-mga · 2020-08-20T18:25:03Z

Could you squash commits into one and make the commit message more explicit about what it does?

Thaina · 2020-08-21T01:39:05Z

Could you squash commits into one and make the commit message more explicit about what it does?

I can't. I have edit this with github directly, not from my local

akien-mga · 2020-08-21T11:17:34Z

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs

+                        break;
+                }
+
+                vsWherePath = null;


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

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.....

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

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs

aaronfranke · 2020-08-28T07:06:19Z

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs

+                vsWherePath = Environment.GetEnvironmentVariable(envName);
+                if (!string.IsNullOrEmpty(vsWherePath))
+                {
+                    vsWherePath += "\\Microsoft Visual Studio\\Installer\\vswhere.exe";


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.

aaronfranke · 2020-08-28T07:12:57Z

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs

@@ -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" };


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)" };

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

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.

Support detecting both 32-bit and 64-bit installations of `vswhere.exe`.

akien-mga · 2020-08-29T10:56:19Z

Force pushed to squash commits and make the commit message clearer.

akien-mga · 2020-08-29T12:30:36Z

Thanks!

same as godotengine#41375

Thaina · 2020-08-30T17:39:05Z

@akien-mga Could this also picked into 3.2 branch? I currently only able to use that branch

akien-mga · 2020-08-31T09:45:45Z

@akien-mga Could this also picked into 3.2 branch? I currently only able to use that branch

That's what this label means:

Thaina · 2020-08-31T09:49:15Z

@akien-mga Could this also picked into 3.2 branch? I currently only able to use that branch

That's what this label means:

Sorry then, Yesterday I try to pull 3.2 branch and this code are not in there yet

akien-mga · 2020-08-31T10:57:09Z

Cherry-picked for 3.2.3.

Thaina requested a review from neikeq as a code owner August 19, 2020 16:21

neikeq suggested changes Aug 19, 2020

View reviewed changes

Thaina requested a review from neikeq August 20, 2020 02:00

neikeq approved these changes Aug 20, 2020

View reviewed changes

akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:dotnet labels Aug 21, 2020

akien-mga added this to the 4.0 milestone Aug 21, 2020

akien-mga reviewed Aug 21, 2020

View reviewed changes

bruvzg reviewed Aug 21, 2020

View reviewed changes

modules/mono/editor/GodotTools/GodotTools/Build/MsBuildFinder.cs Outdated Show resolved Hide resolved

neikeq self-assigned this Aug 21, 2020

aaronfranke reviewed Aug 28, 2020

View reviewed changes

Mono: Improve MSBuildFinder logic on Windows

5dc3900

Support detecting both 32-bit and 64-bit installations of `vswhere.exe`.

akien-mga force-pushed the patch-1 branch from 377ca0f to 5dc3900 Compare August 29, 2020 10:55

akien-mga changed the title ~~Update MsBuildFinder.cs~~ Mono: Improve MSBuildFinder logic on Windows Aug 29, 2020

akien-mga merged commit f10c381 into godotengine:master Aug 29, 2020

Thaina deleted the patch-1 branch August 29, 2020 12:50

Thaina added a commit to Thaina/godot that referenced this pull request Aug 30, 2020

Mono: Improve MSBuildFinder logic on Windows

91f0208

same as godotengine#41375

akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Aug 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mono: Improve MSBuildFinder logic on Windows #41375

Mono: Improve MSBuildFinder logic on Windows #41375

Thaina commented Aug 19, 2020

akien-mga commented Aug 19, 2020

Thaina commented Aug 19, 2020 •

edited

Loading

Thaina commented Aug 19, 2020

neikeq left a comment

neikeq Aug 19, 2020

neikeq Aug 19, 2020

neikeq commented Aug 19, 2020

akien-mga commented Aug 20, 2020

Thaina commented Aug 21, 2020

akien-mga Aug 21, 2020

neikeq Aug 21, 2020

Thaina Aug 22, 2020

aaronfranke Aug 28, 2020

aaronfranke Aug 28, 2020

Thaina Aug 28, 2020

aaronfranke Aug 28, 2020 •

edited

Loading

akien-mga commented Aug 29, 2020

akien-mga commented Aug 29, 2020

Thaina commented Aug 30, 2020

akien-mga commented Aug 31, 2020

Thaina commented Aug 31, 2020

akien-mga commented Aug 31, 2020

	vsWherePath = Environment.GetEnvironmentVariable(Internal.GodotIs32Bits() ? "ProgramFiles(x86)" : "ProgramFiles");
	vsWherePath = Environment.GetEnvironmentVariable(envName);

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

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

Mono: Improve MSBuildFinder logic on Windows #41375

Mono: Improve MSBuildFinder logic on Windows #41375

Conversation

Thaina commented Aug 19, 2020

akien-mga commented Aug 19, 2020

Thaina commented Aug 19, 2020 • edited Loading

Thaina commented Aug 19, 2020

neikeq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neikeq commented Aug 19, 2020

akien-mga commented Aug 20, 2020

Thaina commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfranke Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Aug 29, 2020

akien-mga commented Aug 29, 2020

Thaina commented Aug 30, 2020

akien-mga commented Aug 31, 2020

Thaina commented Aug 31, 2020

akien-mga commented Aug 31, 2020

Thaina commented Aug 19, 2020 •

edited

Loading

aaronfranke Aug 28, 2020 •

edited

Loading