Fix feature bands for side-by-side manifests #33937
Conversation
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
dsplaisted
force-pushed
the
fix-manifest-feature-bands
branch
2 times, most recently
from
5b4d3fd
to
2433fff
Compare
…WorkloadResolver.GetInstalledManifests
dsplaisted
force-pushed
the
fix-manifest-feature-bands
branch
from
2433fff
to
2de943a
Compare
| workloadManifest.Id, | ||
| workloadManifest.Version, | ||
| Path.GetDirectoryName(workloadManifest.ManifestPath)!).ManifestFeatureBand; | ||
| var workloadFeatureBand = manifestInfoDict[workloadManifest.Id].ManifestFeatureBand; |
There was a problem hiding this comment.
I'm not 100% sure I understand the point of this change. It seems like it is trying to make it so you don't have to allocate a new WorkloadManifestInfo in the foreach loop...so is your goal just to move the allocations to all be before the loop?
There was a problem hiding this comment.
Previously it was just saving the path for each manifest and then creating the WorkloadManifestInfo at the end. But now we want to keep track of the feature band together with the manifest path, so it's easier to create the WorkloadManifestInfo with the feature band information when we find each manifest.
| @@ -118,9 +118,10 @@ private void LoadManifestsFromProvider(IWorkloadManifestProvider manifestProvide | |||
| using (Stream? localizationStream = readableManifest.OpenLocalizationStream()) | |||
| { | |||
| var manifest = WorkloadManifestReader.ReadWorkloadManifest(readableManifest.ManifestId, manifestStream, localizationStream, readableManifest.ManifestPath); | |||
| if (!_manifests.TryAdd(readableManifest.ManifestId, manifest)) | |||
| var manifestInfo = new WorkloadManifestInfo(manifest.Id, manifest.Version, readableManifest.ManifestDirectory, readableManifest.ManifestFeatureBand); | |||
| if (!_manifests.TryAdd(readableManifest.ManifestId, (manifest, manifestInfo))) | |||
There was a problem hiding this comment.
It should be very unusual for readableManifest.ManifestId to already be in _manifests, correct? That seems like an install failure.
There was a problem hiding this comment.
Yeah, something would be very wrong, probably a bug in the manifest provider.
| @@ -71,7 +71,12 @@ public static WorkloadSet FromDictionaryForJson(IDictionary<string, string> dict | |||
| public static WorkloadSet FromJson(string json, SdkFeatureBand defaultFeatureBand) | |||
| { | |||
| #if USE_SYSTEM_TEXT_JSON | |||
| return FromDictionaryForJson(JsonSerializer.Deserialize<IDictionary<string, string>>(json)!, defaultFeatureBand); | |||
| var jsonSerializerOptions = new JsonSerializerOptions() | |||
There was a problem hiding this comment.
I'm wondering if we should have a "standard" JsonSerializerOptions that we use everywhere to avoid forgetting AllowTrailingCommas. What do you think?
|
|
||
| string manifestContents3 = """ | ||
| { | ||
| "version": "12.0.1", |
There was a problem hiding this comment.
Do we have any tests covering manifest versions that are prereleases and/or have full buildmetadata, e.g. "1.0.0-preview.1.23361.1+c291ddefb05ff3dd6a0b51df152bba794908dc64"?
Just curious, not asking we change this test now and rerun.
There was a problem hiding this comment.
We have various tests that cover prerelease versions. We don't have any with build metadata. Is there anything specific with build metadata you think we should be testing? I don't think we'll be using it in the versions we use for workloads.
There was a problem hiding this comment.
No, if we have ones with prerelease parts then that's fine, I couldn't recall if we did.
.../Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Show resolved
Hide resolved
This PR contains two fixes:
dotnet workload updatewould fail with side-by-side workload manifests, as the manifest ID would be used for the SDK feature band, and wouldn't parse correctly as a version.We will want to port these to 7.0.4xx (probably via #33643).