.NET SDK managed workload installer #16530

sfoslund · 2021-03-25T23:01:30Z

Not included in this PR:

~~Garbage collection~~
Manifest updates

Testing notes:

Any workloads with all packs on https://pkgs.dev.azure.com/azure-public/vside/_packaging/xamarin-impl/nuget/v3/index.json will successfully install (ex macOS, iOS workloads), others won't until we expand the nuget package installer API
Example test command: dotnet workload install microsoft-ios-sdk-full --skip-manifest-update, make sure to include skip manifest update flag

ghost · 2021-03-25T23:01:39Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/Resolvers/Microsoft.DotNet.NativeWrapper/EnvironmentProvider.cs

…data folder

src/Resolvers/Microsoft.DotNet.NativeWrapper/EnvironmentProvider.cs

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadUnitInstaller.cs

wli3 · 2021-04-02T04:43:28Z

Sorry please give me sometime to review this change carefully. including run the tests and see the test coverage

wli3

not done yet

src/Cli/dotnet/NugetPackageInstaller/NuGetPackagePathResolver.cs

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadInstaller.cs

src/Cli/dotnet/NugetPackageInstaller/NuGetPackageInstaller.cs

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs

src/Tests/Microsoft.NET.Sdk.WorkloadManifestReader.Tests/WorkloadSuggestionFinderTests.cs

src/Tests/Microsoft.NET.TestFramework/TestAssetsManager.cs

src/Tests/dotnet-workload-install.Tests/MockPackWorkloadInstaller.cs

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallManager.cs

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallerFactory.cs

wli3

Finally finished a round of check.

Awesome work as always!

src/Cli/dotnet/commands/dotnet-workload/install/NetSdkManagedInstaller.cs

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadInstaller.cs

sfoslund · 2021-04-06T18:33:28Z

@dsplaisted @wli3 @joeloff I think I'm caught up on most of the feedback, is there anything else I should look at?

I'm also consistently getting a test failure that I can't repro anywhere that's caused by a System.UnauthorizedAccessException error when looking at a directory that I lay down in the test. Have any of you seen this kind of things before? https://dev.azure.com/dnceng/public/_build/results?buildId=1074516&view=ms.vss-test-web.build-test-results-tab&runId=32985386&resultId=100738&paneView=debug

wli3 · 2021-04-06T23:08:15Z

Let me check again

dsplaisted

Great job! Feel free to go ahead and merge and address the feedback separately if that works better for you.

I didn't review the tests yet, I will try to still do that.

src/Cli/dotnet/NugetPackageInstaller/INuGetPackageInstaller.cs

src/Cli/dotnet/NugetPackageInstaller/NuGetPackagePathResolver.cs

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadInstaller.cs

dsplaisted · 2021-04-06T23:15:58Z

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadInstaller.cs

+
+using System.Collections.Generic;
+
+namespace Microsoft.DotNet.Workloads.Workload.Install


We are going to want the workload resolver to be able to use the IAL (to check installation records for the current band). So can we factor out most of the workload installation code into a separate library that dotnet and Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver would reference (and would be included as source in the Microsoft.DotNet.MSBuildSdkResolver project)?

src/Cli/dotnet/commands/dotnet-workload/install/ManifestVersion.cs

src/Cli/dotnet/commands/dotnet-workload/install/NetSdkManagedInstaller.cs

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallManager.cs

sfoslund · 2021-04-08T21:46:48Z

@dsplaisted thanks for the feedback! I think I addressed all of it and there were some significant changes (especially in garbage collection) in the last commit, do you want to take a look before I merge?

The only piece I've left out of this PR is refactoring this into a separate project per this comment. I've added it to #16483 as a follow up task.

sfoslund · 2021-04-08T23:25:45Z

@wli3 I had to add a workaround for testing workload commands when the DEVENABLEWORKLOADCOMMAND environment variable isn't set, is there somewhere you're tracking removing that env var so I can add a note to remove 35900a5 as well?

dsplaisted · 2021-04-08T17:20:11Z

src/Tests/dotnet-workload-install.Tests/GivenDotnetWorkloadInstall.cs

+            installer.InstalledPacks.Count.Should().Be(8);
+            installer.InstalledPacks.Where(pack => pack.Id.Contains("Android")).Count().Should().Be(8);


I'd suggest calling Should().BeEquivalentTo() and passing the list of 8 packs that should be installed.

dsplaisted · 2021-04-08T17:30:46Z

src/Tests/dotnet-workload-install.Tests/GivenDotnetWorkloadInstall.cs

+            try
+            {
+                installManager.InstallWorkloads(mockWorkloadIds, true);
+
+                // Install should have failed
+                true.Should().BeFalse();
+            }
+            catch (Exception e)
+            {
+                e.Message.Should().Be("Failing workload: xamarin-android-build");
+                var expectedPacks = mockWorkloadIds
+                    .SelectMany(workloadId => workloadResolver.GetPacksInWorkload(workloadId.ToString()))
+                    .Select(packId => workloadResolver.TryGetPackInfo(packId));
+                installer.RolledBackPacks.ShouldBeEquivalentTo(expectedPacks);
+                installer.WorkloadInstallRecord.Should().BeEmpty();
+            }


Instead of a try/catch block and a true.Should().BeFalse() assertion, try using Assert.Throws:

Suggested change

try

{

installManager.InstallWorkloads(mockWorkloadIds, true);

// Install should have failed

true.Should().BeFalse();

}

catch (Exception e)

{

e.Message.Should().Be("Failing workload: xamarin-android-build");

var expectedPacks = mockWorkloadIds

.SelectMany(workloadId => workloadResolver.GetPacksInWorkload(workloadId.ToString()))

.Select(packId => workloadResolver.TryGetPackInfo(packId));

installer.RolledBackPacks.ShouldBeEquivalentTo(expectedPacks);

installer.WorkloadInstallRecord.Should().BeEmpty();

}

var exceptionThrown = Assert.Throws<Exception>(() => installManager.InstallWorkloads(mockWorkloadIds, true));

exceptionThrown.Message.Should().Be("Failing workload: xamarin-android-build");

var expectedPacks = mockWorkloadIds

.SelectMany(workloadId => workloadResolver.GetPacksInWorkload(workloadId.ToString()))

.Select(packId => workloadResolver.TryGetPackInfo(packId));

installer.RolledBackPacks.ShouldBeEquivalentTo(expectedPacks);

installer.WorkloadInstallRecord.Should().BeEmpty();

Separately, I think the test should hard-code the expected packs instead of calling into the workload resolver to figure out what packs make up the workload.

dsplaisted · 2021-04-08T17:45:35Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+            }
+        }
+
+        private (string, NetSdkManagedInstaller, INuGetPackageInstaller) GetTestInstaller([CallerMemberName] string identifier = "", bool failingInstaller = false)


Use testName here instead of identifier. The identifier is generally used to disambiguate between multiple instances of the same theory. As is, there's a mismatch on the next line where the identifier is being passed in as the testName parameter to CreateTestDirectory.

dsplaisted · 2021-04-09T17:14:43Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+        }
+
+        [Fact]
+        public void GivenManagedInstallItCanGetFeatureBands()


I think GetFeatureBandsWithInstallationRecords should be changed so it only returns bands where there are actually workload installation records. So this test should be updated to account for that, and to test both empty and non-empty InstalledWorkloads directories.

dsplaisted · 2021-04-09T17:16:03Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+        }
+
+        [Fact]
+        public void GivenManagedInstallItCanGetInstalledWorkloads()


For this test I'd suggest writing some different workloads for a different feature band to verify it doesn't pick those up.

dsplaisted · 2021-04-09T17:20:32Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+        }
+
+        [Fact]
+        public void GivenManagedInstallItDetectInstalledPacks()


Grammar nit

Suggested change

public void GivenManagedInstallItDetectInstalledPacks()

public void GivenManagedInstallItDetectsInstalledPacks()

dsplaisted · 2021-04-09T17:21:21Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+
+            installer.InstallWorkloadPack(packInfo, new SdkFeatureBand(version));
+
+            _reporter.Lines.Should().Contain(string.Format(LocalizableStrings.WorkloadPackAlreadyInstalledMessage, packInfo.Id, packInfo.Version));


Instead of (or in addition to) verifying the message, could this test check that the NuGet package installer was never called to download the package?

dsplaisted · 2021-04-09T17:22:13Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+                installer.InstallWorkloadPack(packInfo, new SdkFeatureBand(version));
+
+                // Install should have failed
+                true.Should().BeFalse();


Switch try/catch and true.Should().BeFalse() for Assert.Throws.

dsplaisted · 2021-04-09T17:24:09Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+        }
+
+        [Fact]
+        public void GivenManagedInstallItCanRollBackInstallFailures()


Could we have coverage of what happens when multiple packs are supposed to be installed, and some install successfully but then one fails and they all need to be rolled back?

That's what this test covers: https://github.com/dotnet/sdk/pull/16530/files#diff-c8bc89094b206b506f35f0cb9ac6e55591ae2468130efe0a19dab1810c0f0c1cR52, the first pack installs successfully, the second fails, and they are both rolled back.

dsplaisted · 2021-04-09T17:29:23Z

src/Tests/dotnet-workload-install.Tests/GivenNetSdkManagedWorkloadInstall.cs

+        }
+
+        [Fact]
+        public void GivenManagedInstallItDoesNotRemovePacksWithInstallRecords()


Can we add some packs to this test that aren't part of the xamarin-android-build workload and verify that they get garbage collected?

sfoslund · 2021-04-09T18:51:46Z

Thanks for the feedback, I'll address it in a follow up PR

sfoslund added 2 commits March 25, 2021 15:57

Add workload install orchestration logic

5d14e30

Adding .NET SDK managed workload resolver

24723fb

sfoslund added 2 commits March 26, 2021 09:43

Merge remote-tracking branch 'upstream/main' into WorkloadInstall

7bcfd8d

Cleaning up merge

6f32a64

sfoslund requested review from dsplaisted, joeloff and wli3 March 26, 2021 18:12

joeloff reviewed Mar 26, 2021

View reviewed changes

src/Resolvers/Microsoft.DotNet.NativeWrapper/EnvironmentProvider.cs Outdated Show resolved Hide resolved

Moving workload installation records from sdk-manifest folder to meta…

87e60fa

…data folder

joeloff reviewed Mar 27, 2021

View reviewed changes

src/Resolvers/Microsoft.DotNet.NativeWrapper/EnvironmentProvider.cs Outdated Show resolved Hide resolved

joeloff reviewed Mar 30, 2021

View reviewed changes

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadUnitInstaller.cs Outdated Show resolved Hide resolved

Adding garbage collection

0c0d530

wli3 reviewed Apr 2, 2021

View reviewed changes

joeloff reviewed Apr 2, 2021

View reviewed changes

src/Cli/dotnet/NugetPackageInstaller/NuGetPackageInstaller.cs Show resolved Hide resolved

wli3 reviewed Apr 2, 2021

View reviewed changes

Converting primitive types to workload types

663828f

sfoslund mentioned this pull request Apr 5, 2021

SDK managed workload installer #16483

Closed

15 tasks

PR feedback

cb48d0c

joeloff reviewed Apr 6, 2021

View reviewed changes

src/Cli/dotnet/commands/dotnet-workload/install/IWorkloadInstaller.cs Outdated Show resolved Hide resolved

Fixing garbage collection bug

e7f7383

Fixing workload managed install garbage collection bug

64729b7

dsplaisted approved these changes Apr 8, 2021

View reviewed changes

wli3 approved these changes Apr 8, 2021

View reviewed changes

PR feedback

4b7e87d

Add workaround for testing workload commands without env var enabled

35900a5

dsplaisted approved these changes Apr 9, 2021

View reviewed changes

sfoslund merged commit 023ed10 into dotnet:main Apr 9, 2021

sfoslund deleted the WorkloadInstall branch April 9, 2021 18:51

sfoslund mentioned this pull request Apr 9, 2021

Refactor SDK managed workload install #16823

Merged


		using System.Collections.Generic;

		namespace Microsoft.DotNet.Workloads.Workload.Install

		installer.InstalledPacks.Count.Should().Be(8);
		installer.InstalledPacks.Where(pack => pack.Id.Contains("Android")).Count().Should().Be(8);

	public void GivenManagedInstallItDetectInstalledPacks()
	public void GivenManagedInstallItDetectsInstalledPacks()


		installer.InstallWorkloadPack(packInfo, new SdkFeatureBand(version));

		_reporter.Lines.Should().Contain(string.Format(LocalizableStrings.WorkloadPackAlreadyInstalledMessage, packInfo.Id, packInfo.Version));

.NET SDK managed workload installer #16530

.NET SDK managed workload installer #16530

Uh oh!

Conversation

sfoslund commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wli3 commented Apr 2, 2021

Uh oh!

wli3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wli3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfoslund commented Apr 6, 2021

Uh oh!

wli3 commented Apr 6, 2021

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfoslund commented Apr 8, 2021

Uh oh!

sfoslund commented Apr 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfoslund commented Mar 25, 2021 •

edited

Loading