Skip to content

Fix feature bands for side-by-side manifests #33937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dsplaisted merged 2 commits into from
Jul 12, 2023
Merged

Fix feature bands for side-by-side manifests #33937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
998d868
Correctly report manifest feature band for side-by-side manifests in …
dsplaisted Jul 12, 2023
2de943a
Allow trailing commas in workload set json
dsplaisted Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/Cli/dotnet/commands/dotnet-workload/WorkloadCommandParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,12 @@ internal static void ShowWorkloadsInfo(ParseResult parseResult = null, IWorkload
return;
}

var manifestInfoDict = workloadInfoHelper.WorkloadResolver.GetInstalledManifests().ToDictionary(info => info.Id, StringComparer.OrdinalIgnoreCase);

foreach (var workload in installedWorkloads.AsEnumerable())
{
var workloadManifest = workloadInfoHelper.WorkloadResolver.GetManifestFromWorkload(new WorkloadId(workload.Key));
var workloadFeatureBand = new WorkloadManifestInfo(
workloadManifest.Id,
workloadManifest.Version,
Path.GetDirectoryName(workloadManifest.ManifestPath)!).ManifestFeatureBand;
var workloadFeatureBand = manifestInfoDict[workloadManifest.Id].ManifestFeatureBand;
Copy link
Contributor

@Forgind Forgind Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@dsplaisted dsplaisted Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const int align = 10;
const string separator = " ";
Expand Down
7 changes: 5 additions & 2 deletions src/Cli/dotnet/commands/dotnet-workload/list/WorkloadListCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.CommandLine;
using System.CommandLine.Parsing;
Expand Down Expand Up @@ -81,15 +82,17 @@ public override int Execute()
}
else
{
var manifestInfoDict = _workloadListHelper.WorkloadResolver.GetInstalledManifests().ToDictionary(info => info.Id, StringComparer.OrdinalIgnoreCase);

InstalledWorkloadsCollection installedWorkloads = _workloadListHelper.AddInstalledVsWorkloads(installedList);
Reporter.WriteLine();
PrintableTable<KeyValuePair<string, string>> table = new();
table.AddColumn(InformationStrings.WorkloadIdColumn, workload => workload.Key);
table.AddColumn(InformationStrings.WorkloadManfiestVersionColumn, workload =>
{
var m = _workloadListHelper.WorkloadResolver.GetManifestFromWorkload(new WorkloadId(workload.Key));
return m.Version + "/" +
new WorkloadManifestInfo(m.Id, m.Version, Path.GetDirectoryName(m.ManifestPath)!).ManifestFeatureBand;
var manifestInfo = manifestInfoDict[m.Id];
return m.Version + "/" + manifestInfo.ManifestFeatureBand;
});
table.AddColumn(InformationStrings.WorkloadSourceColumn, workload => workload.Value);

Expand Down
5 changes: 4 additions & 1 deletion src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/ReadableWorkloadManifest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,19 @@ public class ReadableWorkloadManifest

public string ManifestPath { get; }

public string ManifestFeatureBand { get; }

readonly Func<Stream> _openManifestStreamFunc;


readonly Func<Stream?> _openLocalizationStream;

public ReadableWorkloadManifest(string manifestId, string manifestDirectory, string manifestPath, Func<Stream> openManifestStreamFunc, Func<Stream?> openLocalizationStream)
public ReadableWorkloadManifest(string manifestId, string manifestDirectory, string manifestPath, string manifestFeatureBand, Func<Stream> openManifestStreamFunc, Func<Stream?> openLocalizationStream)
{
ManifestId = manifestId;
ManifestPath = manifestPath;
ManifestDirectory = manifestDirectory;
ManifestFeatureBand = manifestFeatureBand;
_openManifestStreamFunc = openManifestStreamFunc;
_openLocalizationStream = openLocalizationStream;
}
Expand Down
67 changes: 36 additions & 31 deletions ...esolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,29 @@ internal SdkDirectoryWorkloadManifestProvider(string sdkRootPath, string sdkVers
public IEnumerable<ReadableWorkloadManifest> GetManifests()
{
// Scan manifest directories
var manifestIdsToDirectories = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
var manifestIdsToManifests = new Dictionary<string, ReadableWorkloadManifest>(StringComparer.OrdinalIgnoreCase);

void ProbeDirectory(string manifestDirectory)
void AddManifest(string manifestId, string manifestDirectory, string featureBand)
{
var workloadManifestPath = Path.Combine(manifestDirectory, "WorkloadManifest.json");

var readableManifest = new ReadableWorkloadManifest(
manifestId,
manifestDirectory,
workloadManifestPath,
featureBand,
() => File.OpenRead(workloadManifestPath),
() => WorkloadManifestReader.TryOpenLocalizationCatalogForManifest(workloadManifestPath));

manifestIdsToManifests[manifestId] = readableManifest;
}

void ProbeDirectory(string manifestDirectory, string featureBand)
{
(string? id, string? finalManifestDirectory) = ResolveManifestDirectory(manifestDirectory);
if (id != null && finalManifestDirectory != null)
{
manifestIdsToDirectories.Add(id, finalManifestDirectory);
AddManifest(id, finalManifestDirectory, featureBand);
}
}

Expand All @@ -147,7 +162,7 @@ void ProbeDirectory(string manifestDirectory)
{
foreach (var workloadManifestDirectory in Directory.EnumerateDirectories(manifestVersionBandDirectory))
{
ProbeDirectory(workloadManifestDirectory);
ProbeDirectory(workloadManifestDirectory, _sdkVersionBand.ToString());
}
}
}
Expand All @@ -169,7 +184,7 @@ void ProbeDirectory(string manifestDirectory)

foreach (var workloadManifestDirectory in directoriesWithManifests.Values)
{
ProbeDirectory(workloadManifestDirectory);
ProbeDirectory(workloadManifestDirectory, _sdkVersionBand.ToString());
}
}

Expand All @@ -184,7 +199,8 @@ void ProbeDirectory(string manifestDirectory)
{
throw new FileNotFoundException(string.Format(Strings.ManifestFromWorkloadSetNotFound, manifestSpecifier.ToString(), _workloadSet.Version));
}
manifestIdsToDirectories[kvp.Key.ToString()] = manifestDirectory;
AddManifest(manifestSpecifier.Id.ToString(), manifestDirectory, manifestSpecifier.FeatureBand.ToString());

}
}

Expand All @@ -199,26 +215,26 @@ void ProbeDirectory(string manifestDirectory)
{
throw new FileNotFoundException(string.Format(Strings.ManifestFromInstallStateNotFound, manifestSpecifier.ToString(), _installStateFilePath));
}
manifestIdsToDirectories[kvp.Key.ToString()] = manifestDirectory;
AddManifest(manifestSpecifier.Id.ToString(), manifestDirectory, manifestSpecifier.FeatureBand.ToString());
}
}

if (_knownManifestIdsAndOrder != null && _knownManifestIdsAndOrder.Keys.Any(id => !manifestIdsToDirectories.ContainsKey(id)))
if (_knownManifestIdsAndOrder != null && _knownManifestIdsAndOrder.Keys.Any(id => !manifestIdsToManifests.ContainsKey(id)))
{
var missingManifestIds = _knownManifestIdsAndOrder.Keys.Where(id => !manifestIdsToDirectories.ContainsKey(id));
var missingManifestIds = _knownManifestIdsAndOrder.Keys.Where(id => !manifestIdsToManifests.ContainsKey(id));
foreach (var missingManifestId in missingManifestIds)
{
var manifestDir = FallbackForMissingManifest(missingManifestId);
var (manifestDir, featureBand) = FallbackForMissingManifest(missingManifestId);
if (!string.IsNullOrEmpty(manifestDir))
{
manifestIdsToDirectories.Add(missingManifestId, manifestDir);
AddManifest(missingManifestId, manifestDir, featureBand);
}
}
}

// Return manifests in a stable order. Manifests in the KnownWorkloadManifests.txt file will be first, and in the same order they appear in that file.
// Then the rest of the manifests (if any) will be returned in (ordinal case-insensitive) alphabetical order.
return manifestIdsToDirectories
return manifestIdsToManifests
.OrderBy(kvp =>
{
if (_knownManifestIdsAndOrder != null &&
Expand All @@ -229,19 +245,7 @@ void ProbeDirectory(string manifestDirectory)
return int.MaxValue;
})
.ThenBy(kvp => kvp.Key, StringComparer.OrdinalIgnoreCase)
.Select(kvp =>
{
var manifestId = kvp.Key;
var manifestDirectory = kvp.Value;
var workloadManifestPath = Path.Combine(manifestDirectory, "WorkloadManifest.json");

return new ReadableWorkloadManifest(
manifestId,
manifestDirectory,
workloadManifestPath,
() => File.OpenRead(workloadManifestPath),
() => WorkloadManifestReader.TryOpenLocalizationCatalogForManifest(workloadManifestPath));
})
.Select(kvp => kvp.Value)
.ToList();
}

Expand Down Expand Up @@ -282,21 +286,21 @@ void ProbeDirectory(string manifestDirectory)
return (null, null);
}

private string FallbackForMissingManifest(string manifestId)
private (string manifestDirectory, string manifestFeatureBand) FallbackForMissingManifest(string manifestId)
{
// Only use the last manifest root (usually the dotnet folder itself) for fallback
var sdkManifestPath = _manifestRoots.Last();
if (!Directory.Exists(sdkManifestPath))
{
return string.Empty;
return (string.Empty, string.Empty);
}

var candidateFeatureBands = Directory.GetDirectories(sdkManifestPath)
.Select(dir => Path.GetFileName(dir))
.Select(featureBand => new SdkFeatureBand(featureBand))
.Where(featureBand => featureBand < _sdkVersionBand || _sdkVersionBand.ToStringWithoutPrerelease().Equals(featureBand.ToString(), StringComparison.Ordinal));

var matchingManifestFatureBandsAndResolvedManifestDirectories = candidateFeatureBands
var matchingManifestFeatureBandsAndResolvedManifestDirectories = candidateFeatureBands
// Calculate path to <FeatureBand>\<ManifestID>
.Select(featureBand => (featureBand, manifestDirectory: Path.Combine(sdkManifestPath, featureBand.ToString(), manifestId)))
// Filter out directories that don't exist
Expand All @@ -307,14 +311,15 @@ private string FallbackForMissingManifest(string manifestId)
.Where(t => t.res.id != null && t.res.manifestDirectory != null)
.ToList();

if (matchingManifestFatureBandsAndResolvedManifestDirectories.Any())
if (matchingManifestFeatureBandsAndResolvedManifestDirectories.Any())
{
return matchingManifestFatureBandsAndResolvedManifestDirectories.OrderByDescending(t => t.featureBand).First().res.manifestDirectory!;
var selectedFeatureBandAndManifestDirectory = matchingManifestFeatureBandsAndResolvedManifestDirectories.OrderByDescending(t => t.featureBand).First();
return (selectedFeatureBandAndManifestDirectory.res.manifestDirectory!, selectedFeatureBandAndManifestDirectory.featureBand.ToString());
}
else
{
// Manifest does not exist
return string.Empty;
return (string.Empty, string.Empty);
}
}

Expand Down
1 change: 1 addition & 0 deletions ...solvers/Microsoft.NET.Sdk.WorkloadManifestReader/TempDirectoryWorkloadManifestProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public IEnumerable<ReadableWorkloadManifest>
manifestId,
workloadManifestDirectory,
workloadManifestPath,
_sdkVersionBand,
() => File.OpenRead(workloadManifestPath),
() => WorkloadManifestReader.TryOpenLocalizationCatalogForManifest(workloadManifestPath)
);
Expand Down
4 changes: 2 additions & 2 deletions src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadManifestInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ namespace Microsoft.NET.Sdk.WorkloadManifestReader
{
public class WorkloadManifestInfo
{
public WorkloadManifestInfo(string id, string version, string manifestDirectory)
public WorkloadManifestInfo(string id, string version, string manifestDirectory, string manifestFeatureBand)
{
Id = id;
Version = version;
ManifestDirectory = manifestDirectory;
ManifestFeatureBand = Path.GetFileName(Path.GetDirectoryName(manifestDirectory))!;
ManifestFeatureBand = manifestFeatureBand;
}

public string Id { get; }
Expand Down
26 changes: 17 additions & 9 deletions src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace Microsoft.NET.Sdk.WorkloadManifestReader
/// </remarks>
public class WorkloadResolver : IWorkloadResolver
{
private readonly Dictionary<string, WorkloadManifest> _manifests = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, (WorkloadManifest manifest, WorkloadManifestInfo info)> _manifests = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<WorkloadId, (WorkloadDefinition workload, WorkloadManifest manifest)> _workloads = new();
private readonly Dictionary<WorkloadPackId, (WorkloadPack pack, WorkloadManifest manifest)> _packs = new();
private IWorkloadManifestProvider? _manifestProvider;
Expand Down Expand Up @@ -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)))
Copy link
Contributor

@Forgind Forgind Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be very unusual for readableManifest.ManifestId to already be in _manifests, correct? That seems like an install failure.

Copy link
Member Author

@dsplaisted dsplaisted Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, something would be very wrong, probably a bug in the manifest provider.

{
var existingManifest = _manifests[readableManifest.ManifestId];
var existingManifest = _manifests[readableManifest.ManifestId].manifest;
throw new WorkloadManifestCompositionException(Strings.DuplicateManifestID, manifestProvider.GetType().FullName, readableManifest.ManifestId, readableManifest.ManifestPath, existingManifest.ManifestPath);
}
}
Expand All @@ -134,14 +135,15 @@ private void ComposeWorkloadManifests()

Dictionary<WorkloadId, (WorkloadRedirect redirect, WorkloadManifest manifest)>? redirects = null;

foreach (var manifest in _manifests.Values)
foreach (var (manifest, info) in _manifests.Values)
{
if (manifest.DependsOnManifests != null)
{
foreach (var dependency in manifest.DependsOnManifests)
{
if (_manifests.TryGetValue(dependency.Key, out var resolvedDependency))
if (_manifests.TryGetValue(dependency.Key, out var t))
{
var resolvedDependency = t.manifest;
if (FXVersion.Compare(dependency.Value, resolvedDependency.ParsedVersion) > 0)
{
throw new WorkloadManifestCompositionException(Strings.ManifestDependencyVersionTooLow, dependency.Key, resolvedDependency.Version, dependency.Value, manifest.Id, manifest.ManifestPath);
Expand Down Expand Up @@ -711,11 +713,17 @@ private bool IsWorkloadPlatformCompatible(WorkloadDefinition workload, WorkloadM

private bool IsWorkloadImplicitlyAbstract(WorkloadDefinition workload, WorkloadManifest manifest) => !GetPacksInWorkload(workload, manifest).Any();

public string GetManifestVersion(string manifestId) =>
(_manifests.TryGetValue(manifestId, out WorkloadManifest? value)? value : null)?.Version
?? throw new Exception($"Manifest with id {manifestId} does not exist.");
public string GetManifestVersion(string manifestId)
{
if (_manifests.TryGetValue(manifestId, out var value))
{
return value.manifest.Version;
}
throw new Exception($"Manifest with id {manifestId} does not exist.");
}


public IEnumerable<WorkloadManifestInfo> GetInstalledManifests() => _manifests.Select(m => new WorkloadManifestInfo(m.Value.Id, m.Value.Version, Path.GetDirectoryName(m.Value.ManifestPath)!));
public IEnumerable<WorkloadManifestInfo> GetInstalledManifests() => _manifests.Select(t => t.Value.info);
}

static class DictionaryExtensions
Expand Down
7 changes: 6 additions & 1 deletion src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

@Forgind Forgind Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should have a "standard" JsonSerializerOptions that we use everywhere to avoid forgetting AllowTrailingCommas. What do you think?

{
AllowTrailingCommas = true,
ReadCommentHandling = JsonCommentHandling.Skip
};
return FromDictionaryForJson(JsonSerializer.Deserialize<IDictionary<string, string>>(json, jsonSerializerOptions)!, defaultFeatureBand);
#else
return FromDictionaryForJson(JsonConvert.DeserializeObject<IDictionary<string, string>>(json)!, defaultFeatureBand);
#endif
Expand Down
5 changes: 2 additions & 3 deletions src/Tests/Microsoft.NET.Sdk.WorkloadManifestReader.Tests/FakeManifestProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ public FakeManifestProvider(params (string manifest, string? localizationCatalog
_filePaths = filePaths;
}

public IEnumerable<string> GetManifestDirectories() => throw new NotImplementedException();

public IEnumerable<ReadableWorkloadManifest> GetManifests()
{
foreach (var filePath in _filePaths)
Expand All @@ -36,6 +34,7 @@ public IEnumerable<ReadableWorkloadManifest> GetManifests()
Path.GetFileNameWithoutExtension(filePath.manifest),
Path.GetDirectoryName(filePath.manifest)!,
filePath.manifest,
"8.0.100",
() => new FileStream(filePath.manifest, FileMode.Open, FileAccess.Read),
() => filePath.localizationCatalog != null ? new FileStream(filePath.localizationCatalog, FileMode.Open, FileAccess.Read) : null
);
Expand All @@ -50,13 +49,13 @@ internal class InMemoryFakeManifestProvider : IWorkloadManifestProvider, IEnumer
readonly List<(string id, byte[] content)> _manifests = new List<(string, byte[])>();

public void Add(string id, string content) => _manifests.Add((id, Encoding.UTF8.GetBytes(content)));
public IEnumerable<string> GetManifestDirectories() => throw new NotImplementedException();

public IEnumerable<ReadableWorkloadManifest> GetManifests()
=> _manifests.Select(m => new ReadableWorkloadManifest(
m.id,
$@"C:\fake\{m.id}",
$@"C:\fake\{m.id}\WorkloadManifest.json",
"8.0.100",
(Func<Stream>)(() => new MemoryStream(m.content)),
(Func<Stream?>)(() => null)
));
Expand Down
Loading