Implement workload redirects #18166

mhutch · 2021-06-09T05:19:55Z

Implements workload redirects as defined in dotnet/designs#221

ghost · 2021-06-09T05:19:57Z

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.

mhutch · 2021-06-09T05:21:08Z

I haven't added tests yet, but this demonstrates the transparent redirect approach

wli3 · 2021-06-10T23:40:05Z

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

+        /// <param name="advertisingManifestResolver">A resolver that composes the advertising manifests with the installed manifests that do not have corresponding advertising manifests</param>
+        /// <param name="existingWorkloads">The IDs of all of the installed workloads</param>
+        /// <returns></returns>
+        public IEnumerable<WorkloadId> GetUpdatedWorkloads(WorkloadResolver advertisingManifestResolver, IEnumerable<WorkloadId> installedWorkloads)


That is a new logic? WorkloadManifestUpdater.CalculateManifestUpdates is doing similar

This is to replace it. This one handles redirects and cross-manifest dependencies.

wli3 · 2021-06-11T00:00:41Z

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

@@ -34,4 +43,14 @@ public enum WorkloadDefinitionKind
        Dev,
        Build
    }
+
+    public class WorkloadRedirect : BaseWorkloadDefinition


I need more time to thing about it. So far to me, it is transparent in the workload manifest sense. However, workload ID is used many places outside manifest reader. Mostly installation record, and UI for display. As an ID, it should be unique. And now the assumption breaks.

Every time we compare workload id, we need to consider if they are just an alias. And when we iterate workloads, which "id" we should display?

The current code should not be able to resolve installation record mismatch. We need some kind of migration of installation record. But what if there is a downgrade?

We need to re-evaluate every place we have a workloadid.toString and comparison. In general, I don't like 2 ID can point to the same thing (nuget package id being inconsistently case sensitive/ in sensitive caused a lot bugs already). It is a lot for the caller to watch out. Maybe we could override "==" and "toString". But I still prefer to just have another layer of "display id" and never change the "true id"

wli3 · 2021-06-28T22:30:40Z

@mhutch hows the progress?

sfoslund · 2021-07-06T22:26:52Z

@mhutch is this PR ready for review?

mhutch · 2021-07-07T20:34:39Z

@sfoslund all the existing commits are ready for review, yes. I have a couple more commits to come.

sfoslund

LGTM overall but I'm not sure why CI isn't running. Can we move this out of draft mode? I'd like to get this in this week if possible.

sfoslund · 2021-07-08T19:51:50Z

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

@@ -512,7 +510,7 @@ public PackInfo(string id, string version, WorkloadPackKind kind, string path, s
            /// <summary>
            /// The workload pack ID. The NuGet package ID <see cref="ResolvedPackageId"/> may differ from this.
            /// </summary>
-            public string Id { get; }
+            public WorkloadPackId Id { get; }


I believe this PackInfo.Id type change caused #20350.

sfoslund · 2021-07-08T19:52:26Z

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

@@ -463,7 +463,8 @@ private static BaseWorkloadDefinition ReadWorkloadDefinition(WorkloadId id, ref
                            {
                                throw new WorkloadManifestFormatException(Strings.RedirectWorkloadHasOtherKeys, id);
                            }
-                            return new WorkloadRedirect (id, replacementId);
+                            throw new NotImplementedException("Workload redirects are not yet fully implemented");
+                            //return new WorkloadRedirect (id, replacementId);


Remove commented out code

sfoslund · 2021-07-08T19:55:24Z

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

+        /// <param name="advertisingManifestResolver">A resolver that composes the advertising manifests with the installed manifests that do not have corresponding advertising manifests</param>
+        /// <param name="existingWorkloads">The IDs of all of the installed workloads</param>
+        /// <returns></returns>
+        public IEnumerable<WorkloadId> GetUpdatedWorkloads(WorkloadResolver advertisingManifestResolver, IEnumerable<WorkloadId> installedWorkloads)


This looks great, I'll update the existing code to start using it when this is checked in.

sfoslund · 2021-07-08T19:58:12Z

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

                    {
-                        throw new Exception($"Duplicate workload manifest {manifestId}");
+                        throw new WorkloadManifestCompositionException($"Manifest '{manifestId}' from provider {manifestProvider} does not match payload id '{manifest.Id}'");


Do we expect users to see these errors? If so, they should be localized.

They probably should, yes. I didn't worry because it's possible they'll never be seen, and they aren't very useful to end-users. They're basically internal errors and indicate either the manifests themselves are broken or the combination of manifests is broken.

That said, a lot of the workload manifest errors are localized, so these probably should be too for consistency.

Perhaps the CLI should catch WorkloadManifestCompositionException and WorkloadManifestFormatException and report an error that says the installation (or nuget sources) is broken and suggests repairing or reinstalling.

marcpopMSFT · 2021-07-09T21:57:44Z

@mhutch Is this ready to take out of draft and get CI running as I believe some work from others (Sarah and Will) is waiting on this PR?

sfoslund · 2021-07-13T20:49:04Z

@mhutch do you mind resolving the conflicts so we can get CI running and merge? Let me know if you need a hand, I'm happy to help.

mhutch · 2021-07-13T21:49:42Z

@sfoslund sure, i can rebase & resolve. is this targeting the right branch?

sfoslund · 2021-07-13T22:24:09Z

is this targeting the right branch?

Yep!

Manifests have an "informational path" describing where they were loaded from. The workload resolver includes the manifest IDs and associated informational path in composition errors, so it's easier to see what caused the conflict.

IIRC WorkloadId used to be private but now it's public there's no reason not to use it

We are not fully certain that they are transparent and that they won't cause unintended knock-on effects.

* Better error for missing base workload * Now returns information about where the pack was referenced so better errors can be provided * Factored out a helper for enumerating base workloads

Platform checks should take restrictions on base workloads into account, as clarified in dotnet/designs#230. This means that the CLI can no longer trivially display the RIDs for the platforms on which the workloads are available. This could be added back in future using the RID graph to coalesce values, but would probably need additional work to ensure the values were user friendly. The resolver no longer returns workload definitions. The ensures clients use the resolver's APIs that work on the composed manifest data. Having all code that deals with manifest data direct in a central location helps keep behaviour consistent. It also does not be return abstract or empty workloads as these cannot be installed.

Ensures aliased pack IDs are always resolved

Also improves errors from unresolved redirects, and handles redirects in a second pass so they're not dependent on order of manifests and definitions

We now have a collection of the manifests that are loaded, so use that rather than rescanning. This makes it behave correctly for correct for overlay resolvers. Also return an "Info" object to be consistent with other API. We may want to add more information onto this object in future.

Fallout from the switch from string to id structs

When getting a suggestion for workloads to satisfy missing packs, first determine which cannot be installed on the current machine, and report them with a specific error code NETSDK1178.

dsplaisted · 2021-07-18T18:42:24Z

@mhutch @sfoslund I fixed some issues with the tests here. There are still more failing tests which I may be able to investigate later.

This prevents it from erroring out because it loads the same manifest twice in the workload update test.

…h isn't supported for the current platform

dsplaisted · 2021-07-19T15:34:30Z

@mhutch @sfoslund I think I've fixed the remainder of the test failures.

I replaced IWorkloadResolver.IsWorkloadPlatformCompatible with IsPlatformIncompatibleWorkload in order to support querying whether a workload which wasn't returned by GetAvailableWorkloads was a valid workload ID which wasn't available on the current platform, in order to generate the correct error message.

sfoslund

LGTM, a few nits from my previous review are still left but we can follow up on those later. @mhutch @dsplaisted can we merge this?

sfoslund

LGTM, a few nits from my previous review are still left but we can follow up on those later. @mhutch @dsplaisted can we merge this?

mhutch · 2021-07-19T21:21:27Z

Sounds good to me :)

mhutch added the Area-Workloads label Jun 9, 2021

mhutch requested review from wli3, sfoslund and dsplaisted June 9, 2021 05:21

mhutch force-pushed the workload-redirects branch 8 times, most recently from a5b4d60 to 4c178ef Compare June 10, 2021 22:50

wli3 reviewed Jun 10, 2021

View reviewed changes

wli3 reviewed Jun 11, 2021

View reviewed changes

mhutch force-pushed the workload-redirects branch 2 times, most recently from ce881ce to 5cdb5be Compare June 11, 2021 17:20

mhutch mentioned this pull request Jun 17, 2021

Two workloads using the same pack cause MSBuildWorkloadSdkResolver to fail #16700

Closed

mhutch force-pushed the workload-redirects branch from 88bc040 to 11d15c2 Compare July 6, 2021 21:14

sfoslund approved these changes Jul 8, 2021

View reviewed changes

lewing mentioned this pull request Jul 9, 2021

Update wasm workload name to wasm-tools dotnet/runtime#55413

Merged

mhutch marked this pull request as ready for review July 12, 2021 23:18

mhutch added 15 commits July 16, 2021 20:15

Improve diagnosability of manifest conflicts

0a8238c

Improve workload composition errors

accd669

Manifests have an "informational path" describing where they were loaded from. The workload resolver includes the manifest IDs and associated informational path in composition errors, so it's easier to see what caused the conflict.

Nullability checks in workload manifest reader tests

ed81dd4

Stronger typing of workload/pack ids in public API

3d64056

IIRC WorkloadId used to be private but now it's public there's no reason not to use it

Disable workload redirects for now

a6cd134

We are not fully certain that they are transparent and that they won't cause unintended knock-on effects.

Improve WorkloadResolver.GetPacksInWorkload

0254d95

* Better error for missing base workload * Now returns information about where the pack was referenced so better errors can be provided * Factored out a helper for enumerating base workloads

Include workload ID in exception when resolver cannot find it

619304d

Hide WorkloadResolver.GetPackPath in favor of ResolvePackPath

d867fef

Ensures aliased pack IDs are always resolved

When suggesting workloads, only consider available workloads

536f67c

Localize remaining workload composition errors

e4b36e3

Also improves errors from unresolved redirects, and handles redirects in a second pass so they're not dependent on order of manifests and definitions

Fix ResolvePackPath returning empty workload pack ids

c9d64aa

Fallout from the switch from string to id structs

Check updated composition error messages in workload tests

48720b8

Don't assume missing workload packs can always be satisfied

0537ca7

When getting a suggestion for workloads to satisfy missing packs, first determine which cannot be installed on the current machine, and report them with a specific error code NETSDK1178.

mhutch force-pushed the workload-redirects branch from 8cd699f to 0537ca7 Compare July 17, 2021 08:07

mhutch changed the base branch from main to release/6.0.1xx-preview7 July 17, 2021 23:26

Fix accidental removal that broke workload search tests

f4ed4c7

mhutch force-pushed the workload-redirects branch from 485bc39 to f4ed4c7 Compare July 19, 2021 06:57

dsplaisted added 4 commits July 19, 2021 08:28

Fix error message argument indices

b736168

Fix workload mocks to use correct manifest IDs

4d4f3ef

This prevents it from erroring out because it loads the same manifest twice in the workload update test.

Update test now that mock workload manifest has right ID

9f15943

Generate correct error message when trying to install a workload whic…

88fc6b4

…h isn't supported for the current platform

sfoslund approved these changes Jul 19, 2021

View reviewed changes

sfoslund merged commit a15fd60 into dotnet:release/6.0.1xx-preview7 Jul 19, 2021

dsplaisted mentioned this pull request Jul 20, 2021

Current 6.0 SDK building in VS errors with 'Microsoft.NET.SDK.WorkloadAutoImportPropsLocator' specified could not be found. #17461

Closed

Implement workload redirects #18166

Implement workload redirects #18166

Uh oh!

Conversation

mhutch commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 9, 2021

Uh oh!

mhutch commented Jun 9, 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!

wli3 commented Jun 28, 2021

Uh oh!

sfoslund commented Jul 6, 2021

Uh oh!

mhutch commented Jul 7, 2021

Uh oh!

sfoslund left a comment

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!

marcpopMSFT commented Jul 9, 2021

Uh oh!

sfoslund commented Jul 13, 2021

Uh oh!

mhutch commented Jul 13, 2021

Uh oh!

sfoslund commented Jul 13, 2021

Uh oh!

dsplaisted commented Jul 18, 2021

Uh oh!

dsplaisted commented Jul 19, 2021

Uh oh!

sfoslund left a comment

Choose a reason for hiding this comment

Uh oh!

sfoslund left a comment

Choose a reason for hiding this comment

Uh oh!

mhutch commented Jul 19, 2021

Uh oh!

Uh oh!

mhutch commented Jun 9, 2021 •

edited

Loading