Fix baseline manifest version for RTM branding #43737

dsplaisted · 2024-09-27T01:40:06Z

In #43483, we hit a test failure where a workload uninstall command was failing. On investigation, this was because during the SDK build when the baseline workload set was being created, its feature band was being calculated incorrectly. This led to the workload set being included in a folder such as sdk-manifests\9.0.100-rtm.24476\workloadsets\9.0.100-rtm.24476.1, using 9.0.100-rtm.24476 for the feature band instead of 9.0.100 which would have been correct.

When the SDK scanned for workload sets it would find this as workload set 9.0.100-rtm.24476.1, but when during garbage collection it tried to load the workload set, it would calculate its feature band as 9.0.100 and look for the workload set under the sdk-manifests\9.0.100 folder, and fail when it couldn't be found. This meant garbage collection would always fail. For most workload commands this would be reported as a warning, but for the uninstall command it would cause the whole command to fail.

This PR fixes the issue in two ways:

Correctly calculate the baseline workload set feature band when creating the baseline workload set
When scanning for workload sets, ignore any where the feature band folder name isn't a valid feature band, or where there is an inconsistency between the feature band in the folder name and the feature band of the workload set

Correctly calculate baseline workload set feature band

The LayoutManifests target of the build was using a regex to calculate the feature band based on the workload set version. This PR changes that to use a build task that calls the same code that the product does to do the workload set version mapping. This involves:

Consolidating the workload set version mapping logic into a common WorkloadSetVersion class
Allowing nullable annotations in the dotnet project, so that the WorkloadSetVersion class could use nullability annotations (since it also is consumed by projects that have nullability enabled)
Switching from NuGetVersion to ReleaseVersion in the WorkloadSetVersion.FromWorkloadSetPackageVersion logic. Previously, the methods to convert in different directions where in different locations, to avoid adding a dependency on the NuGet APIs where it wasn't necessary. ReleaseVersion appears to be available in all the locations that these methods are used, so it is a better choice. However, making this change introduces risk that a difference in behavior between NuGetVersion and ReleaseVersion could cause this to be a change in the SDK behavior.
Creating a GetWorkloadSetFeatureBand task in core-sdk-tasks and using it in the LayoutManifests target to calculate the feature band for the baseline workload set

Ignore workload sets with invalid or mismatched feature bands when scanning for available workload sets

This is a change to SdkDirectoryWorkloadManifestProvider. When scanning for workload sets, it ignores any that it would not be able to find later based on the workload set version due to a mismatched feature band folder. This will avoid errors if the SDK later tries to load that workload set and can't find it.

However, this does somewhat reduce the diagnosibility of issues, as it will silently ignore any workload sets that are in the wrong feature band folder. The SdkDirectoryWorkloadManifestProvider doesn't currently have any way to write warnings or similar messages to a log.

Forgind

This is a bit confusing with nullable annotations being added and moving the workload set version <--> workload set package version logic to a new class on top of the real change (a little error catching code in SdkDirectoryWorkloadManifestProvider and making a new task to return the workload set feature band).

I'm a little unclear as to whether this is the right direction. My impression was that a preview SDK should use a preview feature band for its baseline manifest. It should find its preview baseline set (along with closely related preview baseline sets). A release 9.0.100 SDK shouldn't find the preview baseline set, as it should find its own.

In that case, the only real bug here is that GC is doing the feature band calculation wrong, which should be a smaller/easier fix anyway.

As an aside, that regex was pulled from how we calculate feature bands for some individual workloads. (Example) If you think we should be using a different feature band for the folder we put workload sets into, do we also need to update those?

dsplaisted · 2024-09-29T18:02:47Z

I'm a little unclear as to whether this is the right direction. My impression was that a preview SDK should use a preview feature band for its baseline manifest. It should find its preview baseline set (along with closely related preview baseline sets). A release 9.0.100 SDK shouldn't find the preview baseline set, as it should find its own.

Preview SDKs do use preview feature bands. If you have SDK version 9.0.100-rc.1.24476.1, then its feature band is 9.0.100-rc.1. However, SDK versions with -rtm in them are treated specially. Basically when we switch to -rtm, it's not treated as preview anymore. So if you have SDK version 9.0.100-rtm.24476.1 then the feature band is just 9.0.100.

As an aside, that regex was pulled from how we calculate feature bands for some individual workloads. (Example) If you think we should be using a different feature band for the folder we put workload sets into, do we also need to update those?

Yes, we should be consistent in how we calculate feature bands, so if we can switch from MSBuild regexes to a task which calls the same code the product uses, that would be good.

…on API

joeloff · 2024-10-01T15:24:03Z

src/Installer/core-sdk-tasks/GetWorkloadSetFeatureBand.cs

+
+        public override bool Execute()
+        {
+            WorkloadSetFeatureBand = Microsoft.DotNet.Workloads.Workload.WorkloadSetVersion.GetFeatureBand(WorkloadSetVersion).ToString();


nit: Can we capture some of this in a using statement?

joeloff · 2024-10-01T15:25:47Z

src/Installer/core-sdk-tasks/core-sdk-tasks.csproj

  <ItemGroup>
    <PackageReference Include="Microsoft.Build.Tasks.Core" />
    <PackageReference Include="Microsoft.Build.Utilities.Core" />
    <PackageReference Include="Newtonsoft.Json" />
    <PackageReference Include="NuGet.Versioning" />
    <PackageReference Include="NuGet.Packaging" />
    <PackageReference Include="System.Reflection.Metadata" VersionOverride="$(SystemReflectionMetadataLoadContextVersion)" />
+    <PackageReference Include="Microsoft.Deployment.DotNet.Releases" />


Will this trigger an additional module load for VS perf tests?

This is in the core-sdk-tasks project, which is the tasks used by the .NET SDK build itself. So it won't affect the tasks that are part of the .NET SDK and get used by most developers when building projects with the .NET SDK. Those tasks (and the resolvers) which are loaded into VS already reference this package.

joeloff · 2024-10-01T16:31:58Z

test/dotnet-workload-install.Tests/WorkloadSetVersionMappingTests.cs

@@ -48,7 +49,7 @@ public static IEnumerable<object[]> WorkloadVersionsData
        [MemberData(nameof(WorkloadVersionsData))]


Should we include an item in the member data for version that contain rtm since that's one of the special cases handled by SdkFeatureBand?

joeloff · 2024-10-01T16:59:39Z

test/dotnet-MsiInstallation.Tests/WorkloadSetTests.cs

@@ -11,6 +11,7 @@
 using System.Text.Json.Serialization;
 using System.Threading.Tasks;
 using Microsoft.DotNet.MsiInstallerTests.Framework;
+using Microsoft.DotNet.Workloads.Workload;


we should probably sort these usings

I've removed the unused usings. I think the System namespace should generally be sorted before other namespaces in the usings, so I think other than that they are sorted.

dotnet-issue-labeler bot added Area-Workloads untriaged Request triage from a team member labels Sep 27, 2024

dsplaisted requested a review from a team September 27, 2024 01:40

Forgind reviewed Sep 27, 2024

View reviewed changes

dsplaisted mentioned this pull request Sep 30, 2024

[NETSDKE2E] With 9.0.100-rtm SDK installed, Warning: "Workload garbage collection failed with error: Workload version 9.0.100-rtm.24477.19 was not found.." happens when installing maui workload. #43777

Closed

dsplaisted added 5 commits September 30, 2024 17:07

Don't load workload sets from incorrect folders

6b01ca1

Allow nullable annotations for dotnet.csproj

da59bd5

Move workload set mapping logic to common WorkloadSetVersion class

56eb66b

Correctly calculate baseline workload set feature band

61bbc4b

Switch all workload version mapping calls to use new WorkloadSetVersi…

c2d4357

…on API

dsplaisted force-pushed the fix-baseline-manifest-version branch from a0cdfec to c2d4357 Compare September 30, 2024 21:07

dsplaisted changed the base branch from release/9.0.1xx to release/9.0.2xx September 30, 2024 21:07

dsplaisted mentioned this pull request Sep 30, 2024

Disable baseline manifest generation #43797

Merged

joeloff reviewed Oct 1, 2024

View reviewed changes

joeloff approved these changes Oct 1, 2024

View reviewed changes

Code review feedback

3d89cc5

build-analysis bot mentioned this pull request Oct 4, 2024

Track signature errors #42870

Closed

dsplaisted merged commit ccfae96 into dotnet:release/9.0.2xx Oct 5, 2024
31 checks passed

rnelson mentioned this pull request Feb 17, 2025

Bump dotnet-sdk from 9.0.102 to 9.0.200 rnelson/Libexec.AspnetCore.EnvironmentHeaders#11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix baseline manifest version for RTM branding #43737

Fix baseline manifest version for RTM branding #43737

dsplaisted commented Sep 27, 2024

Forgind left a comment

dsplaisted commented Sep 29, 2024

joeloff Oct 1, 2024

dsplaisted Oct 4, 2024

joeloff Oct 1, 2024

dsplaisted Oct 4, 2024

joeloff Oct 1, 2024

dsplaisted Oct 4, 2024

joeloff Oct 1, 2024

dsplaisted Oct 4, 2024

		@@ -48,7 +49,7 @@ public static IEnumerable<object[]> WorkloadVersionsData
		[MemberData(nameof(WorkloadVersionsData))]

Fix baseline manifest version for RTM branding #43737

Fix baseline manifest version for RTM branding #43737

Conversation

dsplaisted commented Sep 27, 2024

Correctly calculate baseline workload set feature band

Ignore workload sets with invalid or mismatched feature bands when scanning for available workload sets

Forgind left a comment

Choose a reason for hiding this comment

dsplaisted commented Sep 29, 2024

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