Select tools path relative to the current devenv.exe #1012

ILMTitan · 2018-06-28T00:28:13Z

Fixes #1011
Change how many tools are found, finding them relative to devenv.exe (rather than a program files directory).
Add a bunch of unit tests.
Change the async powershell call to actually shutdown powershell on cancelation.

…nv.exe.

Remove unnecessicary file header.

codecov · 2018-06-28T01:12:34Z

Codecov Report

Merging #1012 into master will increase coverage by 0.66%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
+ Coverage   41.07%   41.74%   +0.66%     
==========================================
  Files         477      478       +1     
  Lines       11551    11533      -18     
==========================================
+ Hits         4745     4814      +69     
+ Misses       6806     6719      -87

Impacted Files	Coverage Δ
...loudExtension/Services/FileSystem/IoFileService.cs	`100% <ø> (ø)`	⬆️
...uggerDialog/InstallStartRemoteToolStepViewModel.cs	`0% <0%> (ø)`	⬆️
...udExtension.PowerShellUtils/RemoteToolInstaller.cs	`0% <0%> (ø)`	⬆️
...loudExtension.PowerShellUtils/RemoteToolSession.cs	`0% <0%> (ø)`	⬆️
...ogleCloudExtension.PowerShellUtils/RemoteTarget.cs	`0% <0%> (ø)`	⬆️
...eCloudExtension/VsVersion/ToolsPathProviderBase.cs	`100% <100%> (ø)`	⬆️
...CloudExtension/VsVersion/VS14/ToolsPathProvider.cs	`100% <100%> (+100%)`	⬆️
...CloudExtension/VsVersion/VS15/ToolsPathProvider.cs	`100% <100%> (+100%)`	⬆️
...Extension/Services/FileSystem/FileSystemService.cs	`100% <100%> (ø)`	⬆️
...Extension.PowerShellUtils/RemotePowerShellUtils.cs	`100% <100%> (+100%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f677b6...ad086c4. Read the comment docs.

Change the error prompt text to be more informative. Simplify the powershell cancellation code.

quoctruong · 2018-06-28T17:07:13Z

GoogleCloudExtension/GoogleCloudExtension.PowerShellUtils/RemotePowerShellUtils.cs

            }
-            return await completedTask;
+            // PowerShell can sometimes complete without error if the cancellation happend quickly enough.


nit: happened

quoctruong · 2018-06-29T18:44:14Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/ToolsPathProviderBase.cs

+        public string GetRemoteDebuggerToolsPath()
+        {
+            string devenvPath = Dte.FullName;
+            string ideDirctoryPath = Path.GetDirectoryName(devenvPath);


nit: ideDirectoryPath

quoctruong · 2018-06-29T18:49:55Z

GoogleCloudExtension/GoogleCloudExtension.PowerShellUtils/RemoteToolSession.cs

-                }
-            });
+
+            _powerShellTask = ExecuteScript(target, script, unsubscribeClosingEvent);


Should we change this function to async and await ExecuteScript?

It is a constructor, so we can not.

Can you explain on what we gain from this? As quoc pointed out, this seems confusing.

What is the this you are referring to?

Creating the ExecuteScript method? We avoid having to wrap an anonymous method in Task.Run.

The async powershell changes as a whole? It is cleaning up code and making it easier to reason about. Instead of a Task.Run wrapping an synchronous method that is blocking on an IAsyncHandle, it is Task all the way down.

quoctruong · 2018-06-29T19:00:50Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/VS15/ToolsPathProvider.cs

-            var programFilesPath = Environment.GetEnvironmentVariable("ProgramFiles(x86)");
-            var result = Path.Combine(programFilesPath, $@"Microsoft Visual Studio\2017\{_edition}\MSBuild\15.0\Bin\MSBuild.exe");
-            GcpOutputWindow.Default.OutputDebugLine($"Program Files: {programFilesPath}");
+            string devenvPath = Dte.FullName;


nit: should be devEnvPath

I go back and forth on that. I settled on devenvPath becuase much of the documentation capitalizes "devenv" as one word.

quoctruong · 2018-06-29T19:01:40Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/VS15/ToolsPathProvider.cs

+            string ideDirectoryPath = Path.GetDirectoryName(devenvPath);
+            string common7DirectoryPath = Path.GetDirectoryName(ideDirectoryPath);
+            string vsRootDirectoryPath = Path.GetDirectoryName(common7DirectoryPath);
+            Debug.Assert(vsRootDirectoryPath != null);


Do these Debug.Assert shows the error to the users?

No, they don't even exist in the release build. They are there to tell resharper something that I know will be true. In C# 8, this would be string vsRootDirectoryPath = Path.GetDirectoryName(common7DirectoryPath)!; to indicate I now vsRootDirectoryPath is not null.

quoctruong · 2018-06-29T19:04:50Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/VS15/ToolsPathProvider.cs

+            string devenvPath = Dte.FullName;
+            string ideDirectoryPath = Path.GetDirectoryName(devenvPath);
+            string common7DirectoryPath = Path.GetDirectoryName(ideDirectoryPath);
+            string vsRootDirectoryPath = Path.GetDirectoryName(common7DirectoryPath);


Maybe we should factor out the logic to get a vsRootDirectoryPath so it can be used here and above.

redborian · 2018-06-29T23:29:04Z

GoogleCloudExtension/GoogleCloudExtension.PowerShellUtils/RemoteToolSession.cs

-                }
-            });
+
+            _powerShellTask = ExecuteScript(target, script, unsubscribeClosingEvent);


Can you explain on what we gain from this? As quoc pointed out, this seems confusing.

redborian · 2018-06-29T23:30:32Z

...udExtension/GoogleCloudExtension.PowerShellUtils/GoogleCloudExtension.PowerShellUtils.csproj

@@ -42,6 +42,9 @@
    <Reference Include="System.Management.Automation, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\packages\System.Management.Automation.dll.10.0.10586.0\lib\net40\System.Management.Automation.dll</HintPath>
    </Reference>
+    <Reference Include="System.ValueTuple, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
+      <HintPath>..\packages\System.ValueTuple.4.5.0\lib\netstandard1.0\System.ValueTuple.dll</HintPath>
+    </Reference>


Why is this needed?

At some point, I had used a C# 7 value tuple, but I removed it before the final change. This nuget package enables that feature, and is no longer needed. Deleted.

redborian · 2018-06-29T23:41:13Z

GoogleCloudExtension/GoogleCloudExtension.PowerShellUtils.Tests/RemotePowerShellUtilsTests.cs

@@ -0,0 +1,166 @@
+using Microsoft.VisualStudio.TestTools.UnitTesting;


Header text is missing

ILMTitan · 2018-07-02T19:37:55Z

@quoctruong, @redborian
PTAL

quoctruong · 2018-07-02T20:34:59Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/ToolsPathProviderBase.cs

+        /// <summary>
+        /// The root directory of the current Visual Studio installation.
+        /// </summary>
+        protected string VsRootDirectoryPath


Should we cache the result in a private field so the getter won't have to perform the computation every time it is called?

I'm not too worried about that. There isn't anything here particularly expensive.

Jim Przybylinski added 3 commits June 27, 2018 16:47

Find msbuild and other tools relative to the path to the current deve…

2747535

…nv.exe.

Fix some unit tests.

885ffe3

Update comments.

5e2c331

Remove unnecessicary file header.

ILMTitan requested review from quoctruong, iantalarico and redborian June 28, 2018 00:28

Attempt a fix of an odd unit test failure.

38214c2

Jim Przybylinski added 2 commits June 28, 2018 10:02

Remove error prompt on operation canceled.

38f7ba8

Change the error prompt text to be more informative. Simplify the powershell cancellation code.

Remove unused resource string.

710767b

quoctruong reviewed Jun 29, 2018

View reviewed changes

redborian reviewed Jun 29, 2018

View reviewed changes

Resolve PR comments.

ad086c4

quoctruong reviewed Jul 2, 2018

View reviewed changes

quoctruong approved these changes Jul 2, 2018

View reviewed changes

ILMTitan merged commit eb2edc3 into GoogleCloudPlatform:master Jul 2, 2018

ILMTitan deleted the issue-1011 branch July 2, 2018 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select tools path relative to the current devenv.exe #1012

Select tools path relative to the current devenv.exe #1012

ILMTitan commented Jun 28, 2018

codecov bot commented Jun 28, 2018 •

edited

quoctruong Jun 28, 2018

ILMTitan Jul 2, 2018

quoctruong Jun 29, 2018

ILMTitan Jul 2, 2018

quoctruong Jun 29, 2018

ILMTitan Jun 29, 2018

redborian Jun 29, 2018

ILMTitan Jul 2, 2018 •

edited

quoctruong Jun 29, 2018

ILMTitan Jul 2, 2018 •

edited

quoctruong Jun 29, 2018

ILMTitan Jun 29, 2018

quoctruong Jun 29, 2018

ILMTitan Jul 2, 2018

redborian Jun 29, 2018

redborian Jun 29, 2018

ILMTitan Jul 2, 2018

redborian Jun 29, 2018

ILMTitan Jul 2, 2018

ILMTitan commented Jul 2, 2018

quoctruong Jul 2, 2018

ILMTitan Jul 2, 2018

		@@ -0,0 +1,166 @@
		using Microsoft.VisualStudio.TestTools.UnitTesting;

Select tools path relative to the current devenv.exe #1012

Select tools path relative to the current devenv.exe #1012

Conversation

ILMTitan commented Jun 28, 2018

codecov bot commented Jun 28, 2018 • edited

Codecov Report

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

Choose a reason for hiding this comment

ILMTitan Jul 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ILMTitan Jul 2, 2018 • edited

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

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

ILMTitan commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 28, 2018 •

edited

ILMTitan Jul 2, 2018 •

edited

ILMTitan Jul 2, 2018 •

edited