Workload manifest updates #16891

sfoslund · 2021-04-13T21:16:28Z

No description provided.

dotnet-issue-labeler · 2021-04-13T21:16:31Z

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/Cli/dotnet/commands/dotnet-workload/install/NetSdkManagedInstaller.cs

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

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

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

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

wli3 · 2021-04-13T23:02:47Z

not done. But I should really finish my PR

sfoslund · 2021-04-15T22:44:11Z

Addressed feedback I've received so far, waiting on workload teams to start generating manifest packages to test with.

wli3 · 2021-04-16T00:19:57Z

do you mind to do a rebase in the meantime? It is easier to see the change. Thanks!

dsplaisted

So far I've just looked at the implementation, but not the test updates.

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

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

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

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

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

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/WorkloadManifestUpdater.cs

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

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

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

sfoslund · 2021-04-26T19:06:56Z

@wli3 @dsplaisted I think I'm caught up to feedback now. I'm going to start testing end to end with the new manifest packages but in the meantime feel free to add more feedback.

…ates

sfoslund · 2021-04-29T18:26:27Z

I've finished testing locally and I believe everything is good to go. A couple of notes:

Since workload teams have not started producing properly formatted packages I tested by creating my own and using a local package source. Ideally I'd like to test end to end with real packages before this is merged.
Manifest updates will only work for workloads that have properly formatted packages (id: {manifest id}.manifest-6.0.100, version matching manifest version, and workload manifest/targets in a data directory inside of the package)
If installation fails and we have to roll back, we need to be able to re-install the baseline manifest. As a result, we'll need workload teams to have the baseline manifest properly formatted and available as well as any new updated manifests.

sfoslund · 2021-04-30T16:13:50Z

@jonathanpeppers how soon will properly named and versioned manifest packages be ready? Is it worth waiting on them to merge this PR or should we proceed with the testing I've done locally?

@dsplaisted let me know if you want me to port this to preview 4, for now I'm going to try to merge to main before there are any more conflicts.

jonathanpeppers · 2021-04-30T20:04:40Z

@sfoslund I started on the Android side: dotnet/android#5898

It will probably be next week some time before we have the 4 Apple workloads renamed, and these changes will be in Preview 5. Assuming this PR wouldn't break anything, you don't have to wait on us for this.

dsplaisted

@sfoslund This is already merged but I went back and took another look at it since I hadn't had a chance to do so before. There's a few (non-critical) things I think would be nice to follow up on and fix.

dsplaisted · 2021-05-05T17:41:44Z

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

                    },
                    rollback: () => {
                        try
                        {
                            _reporter.WriteLine(LocalizableStrings.RollingBackInstall);
+
+							 foreach (var manifest in manifestsToUpdate)


Nit: this indentation seems messed up.

I'm not sure why github is displaying it like that, it seems fine in main: https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs#L237

dsplaisted · 2021-05-05T17:54:16Z

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

+
+        public async Task UpdateAdvertisingManifestsAsync(SdkFeatureBand featureBand, bool includePreviews)
+        {
+            var manifests = GetInstalledManifestIds();


I still think there's a mismatch here. The IWorkloadManifestProvider is passed into the constructor, and is tied to a specific feature band. But there are instance methods on this class that also take a feature band. I think things won't work if there's a mismatch - you'll need to create a new workload manifest provider for the new feature band and then create a new workload manifest updater.

So I think we should remove the arguments from the methods in this class that take a feature band, and instead either take the feature band in the constructor, or better yet update the workload manifest provider so that it can return the feature band and we get the information from that.

dsplaisted · 2021-05-05T18:02:41Z

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

+            }
+            catch (Exception e)
+            {
+                _reporter.WriteLine(string.Format(LocalizableStrings.FailedAdManifestUpdate, manifestId, e.Message));


It sounds like the failure we would expect is if there are no manifest packages available on the NuGet feed. I think we should check for that explicitly and treat it not as an error (ie when we call DownloadPackageAsync). I think other failures are probably not expected and we may want to fail.

dsplaisted · 2021-05-05T18:08:39Z

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

@@ -101,6 +96,53 @@ public void GivenWorkloadInstallOnFailingRollbackItDisplaysTopLevelError()
            exceptionThrown.Message.Should().Be("Failing workload: xamarin-android-build");
            string.Join(" ", _reporter.Lines).Should().Contain("Rollback failure");

+        }
+
+		[Fact]


Nit: Indentation

Same here:

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

Line 101 in cb5b3ff

[Fact]

sfoslund force-pushed the WorkloadManifestUpdates branch from 9a402f8 to b51159e Compare April 13, 2021 22:28

sfoslund commented Apr 13, 2021

View reviewed changes

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

wli3 reviewed Apr 13, 2021

View reviewed changes

sfoslund force-pushed the WorkloadManifestUpdates branch from e32a73a to 0412aaa Compare April 16, 2021 17:31

dsplaisted reviewed Apr 17, 2021

View reviewed changes

wli3 reviewed Apr 21, 2021

View reviewed changes

sfoslund added 5 commits April 23, 2021 09:13

Update advertising manifests on workload install

df0fa22

Add workload command logic for updating installed workload manifests

37cfa89

Adding SDK managed installer implementation of workload manifest install

ba4a1fe

PR feedback

98f289a

PR feedback

c5440f5

sfoslund force-pushed the WorkloadManifestUpdates branch from 0412aaa to c5440f5 Compare April 26, 2021 19:05

sfoslund mentioned this pull request Apr 26, 2021

Removing skip manifest update requirement for prev 4 #17210

Merged

marcpopMSFT mentioned this pull request Apr 28, 2021

Optional Workload Acquisition #14957

Closed

25 tasks

sfoslund added 2 commits April 29, 2021 10:32

Merge remote-tracking branch 'upstream/main' into WorkloadManifestUpd…

d3b3b8d

…ates

Clean up merge

7129955

sfoslund marked this pull request as ready for review April 29, 2021 18:26

sfoslund requested review from joeloff, dsplaisted and wli3 April 29, 2021 18:28

wli3 approved these changes Apr 29, 2021

View reviewed changes

Merge branch 'main' into WorkloadManifestUpdates

fb8a075

sfoslund enabled auto-merge April 30, 2021 20:43

sfoslund merged commit cb5b3ff into dotnet:main Apr 30, 2021

sfoslund deleted the WorkloadManifestUpdates branch April 30, 2021 21:23

sfoslund mentioned this pull request May 3, 2021

SDK managed workload installer #16483

Closed

15 tasks

dsplaisted reviewed May 5, 2021

View reviewed changes

This was referenced May 5, 2021

Workload manifest updates PR feedback #17455

Merged

Port manifest updates to prev 4 #17480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workload manifest updates #16891

Workload manifest updates #16891

sfoslund commented Apr 13, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Apr 13, 2021

wli3 commented Apr 13, 2021

sfoslund commented Apr 15, 2021

wli3 commented Apr 16, 2021 •

edited

Loading

dsplaisted left a comment

sfoslund commented Apr 26, 2021

sfoslund commented Apr 29, 2021

sfoslund commented Apr 30, 2021

jonathanpeppers commented Apr 30, 2021

dsplaisted left a comment

dsplaisted May 5, 2021

sfoslund May 5, 2021

dsplaisted May 5, 2021

dsplaisted May 5, 2021

dsplaisted May 5, 2021

sfoslund May 5, 2021

Workload manifest updates #16891

Workload manifest updates #16891

Conversation

sfoslund commented Apr 13, 2021 • edited Loading

dotnet-issue-labeler bot commented Apr 13, 2021

wli3 commented Apr 13, 2021

sfoslund commented Apr 15, 2021

wli3 commented Apr 16, 2021 • edited Loading

dsplaisted left a comment

Choose a reason for hiding this comment

sfoslund commented Apr 26, 2021

sfoslund commented Apr 29, 2021

sfoslund commented Apr 30, 2021

jonathanpeppers commented Apr 30, 2021

dsplaisted left a comment

Choose a reason for hiding this comment

dsplaisted May 5, 2021

Choose a reason for hiding this comment

sfoslund May 5, 2021

Choose a reason for hiding this comment

dsplaisted May 5, 2021

Choose a reason for hiding this comment

dsplaisted May 5, 2021

Choose a reason for hiding this comment

dsplaisted May 5, 2021

Choose a reason for hiding this comment

sfoslund May 5, 2021

Choose a reason for hiding this comment

sfoslund commented Apr 13, 2021 •

edited

Loading

wli3 commented Apr 16, 2021 •

edited

Loading