Fix feature bands for side-by-side manifests #33937
Conversation
ghost
added
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.
| 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.
| { | ||
| #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.
nagilson
left a comment
nagilson
left a comment
There was a problem hiding this comment.
Overall the changes seem good to me, I am still testing it a bit more to see if I can find anything
.../Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Show resolved
Hide resolved
4 participants
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).