Skip to content

Change default workload set use to 'true' #45936

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
Forgind merged 11 commits into from
Feb 6, 2025
Merged

Change default workload set use to 'true' #45936

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e10404f
Change default workload set use to 'true'
Forgind Jan 13, 2025
bc1473f
Use --> ShouldUse
Forgind Jan 13, 2025
d17a367
revert config change
Forgind Jan 13, 2025
5648eb1
Resolve two cases
Forgind Jan 16, 2025
a1e7775
fix one test?
Forgind Jan 30, 2025
188f8a7
Try to fix some more
Forgind Jan 30, 2025
b1ae3c6
More fixes
Forgind Jan 30, 2025
f46a230
Change a few more tests
Forgind Jan 31, 2025
3b3b5ba
Add usings
Forgind Jan 31, 2025
72e69d6
More fixes
Forgind Jan 31, 2025
014d733
EditorBrowsableState.Never
Forgind Feb 6, 2025
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
2 changes: 1 addition & 1 deletion src/Cli/dotnet/commands/InstallingWorkloadCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ static InstallStateContents GetCurrentInstallState(SdkFeatureBand sdkFeatureBand

public static bool ShouldUseWorkloadSetMode(SdkFeatureBand sdkFeatureBand, string dotnetDir)
{
return GetCurrentInstallState(sdkFeatureBand, dotnetDir).UseWorkloadSets ?? false;
return GetCurrentInstallState(sdkFeatureBand, dotnetDir).ShouldUseWorkloadSets();
}

protected void UpdateWorkloadManifests(WorkloadHistoryRecorder recorder, ITransactionContext context, DirectoryPath? offlineCache)
Expand Down
2 changes: 2 additions & 0 deletions src/Cli/dotnet/commands/dotnet-workload/InstallStateContents.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ public override string ToString()
{
return JsonSerializer.Serialize<InstallStateContents>(this, s_options);
}

public bool ShouldUseWorkloadSets() => UseWorkloadSets ?? true;
Copy link
Member

@baronfel baronfel Jan 13, 2025

Choose a reason for hiding this comment

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

excellent idea - nitpick though - should use workload sets be hidden in some way so that we don't use it accidentally in the codebase, instead always going through your new helper?

Copy link
Contributor Author

@Forgind Forgind Jan 16, 2025

Choose a reason for hiding this comment

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

Unfortunately, JsonSerializer doesn't serialize private properties without a good bit of extra work:
https://stackoverflow.com/questions/61869393/get-net-core-jsonserializer-to-serialize-private-members

Good idea, though 🙂

Copy link
Member

@baronfel baronfel Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

crazier idea - maybe add the [EditorBrowsable(EditorBrowsableState.Never)] attribute to UseWorkloadSets to hide it from completion? I love this little hack: https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.editorbrowsableattribute?view=net-9.0

Copy link
Contributor Author

@Forgind Forgind Jan 16, 2025

Choose a reason for hiding this comment

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

Never heard of that, but that should be doable!

Copy link
Member

@nagilson nagilson Feb 4, 2025

Choose a reason for hiding this comment

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

Did you get a chance to try this out?

Copy link
Contributor Author

@Forgind Forgind Feb 4, 2025

Choose a reason for hiding this comment

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

Ah, forgot to. Now I'm split because I think it's a positive change, but I'm pretty reluctant to retrigger tests when they're passing for a once...

Copy link
Contributor Author

@Forgind Forgind Feb 5, 2025

Choose a reason for hiding this comment

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

Apparently this doesn't work anyway, unfortunately. It isn't available on some of the frameworks where it's needed:
image

Copy link
Contributor Author

@Forgind Forgind Feb 5, 2025

Choose a reason for hiding this comment

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

Wait, I'm probably stupid? That seems like an editor bug; it should be available on net10.0

Copy link
Contributor Author

@Forgind Forgind Feb 6, 2025

Choose a reason for hiding this comment

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

After a bit more poking around, I suspect this just doesn't work properly in my IDE, so I'll push the change. If it fails, I can revert it.

}
}

Expand Down
4 changes: 2 additions & 2 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 @@ -53,8 +53,8 @@ internal static void ShowWorkloadsInfo(ParseResult parseResult = null, WorkloadI

void WriteUpdateModeAndAnyError(string indent = "")
{
var useWorkloadSets = InstallStateContents.FromPath(Path.Combine(WorkloadInstallType.GetInstallStateFolder(workloadInfoHelper._currentSdkFeatureBand, workloadInfoHelper.UserLocalPath), "default.json")).UseWorkloadSets;
var workloadSetsString = useWorkloadSets == true ? "workload sets" : "loose manifests";
var useWorkloadSets = InstallStateContents.FromPath(Path.Combine(WorkloadInstallType.GetInstallStateFolder(workloadInfoHelper._currentSdkFeatureBand, workloadInfoHelper.UserLocalPath), "default.json")).ShouldUseWorkloadSets();
var workloadSetsString = useWorkloadSets ? "workload sets" : "loose manifests";
reporter.WriteLine(indent + string.Format(CommonStrings.WorkloadManifestInstallationConfiguration, workloadSetsString));

if (!versionInfo.IsInstalled)
Expand Down
2 changes: 1 addition & 1 deletion src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public static bool ShouldUseWorkloadSetMode(SdkFeatureBand sdkFeatureBand, strin
{
string path = Path.Combine(WorkloadInstallType.GetInstallStateFolder(sdkFeatureBand, dotnetDir), "default.json");
var installStateContents = File.Exists(path) ? InstallStateContents.FromString(File.ReadAllText(path)) : new InstallStateContents();
return installStateContents.UseWorkloadSets ?? false;
return installStateContents.ShouldUseWorkloadSets();
}

private void WriteUpdatableWorkloadsFile()
Expand Down
2 changes: 1 addition & 1 deletion src/Cli/dotnet/commands/dotnet-workload/list/WorkloadListCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public override int Execute()
Reporter.WriteLine();
var shouldPrintTable = versionInfo.IsInstalled;
var shouldShowWorkloadSetVersion = versionInfo.GlobalJsonPath is not null ||
InstallStateContents.FromPath(Path.Combine(WorkloadInstallType.GetInstallStateFolder(_workloadListHelper._currentSdkFeatureBand, _workloadListHelper.UserLocalPath), "default.json")).UseWorkloadSets == true;
InstallStateContents.FromPath(Path.Combine(WorkloadInstallType.GetInstallStateFolder(_workloadListHelper._currentSdkFeatureBand, _workloadListHelper.UserLocalPath), "default.json")).ShouldUseWorkloadSets();

if (shouldShowWorkloadSetVersion)
{
Expand Down
4 changes: 2 additions & 2 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 @@ -199,7 +199,7 @@ bool TryGetWorkloadSet(string workloadSetVersion, out WorkloadSet? workloadSet)
_manifestsFromInstallState = installState.Manifests is null ? null : WorkloadSet.FromDictionaryForJson(installState.Manifests!, _sdkVersionBand);
}

if (workloadSet == null && installState.UseWorkloadSets == true && availableWorkloadSets.Any())
if (workloadSet == null && installState.ShouldUseWorkloadSets() && availableWorkloadSets.Any())
{
var maxWorkloadSetVersion = availableWorkloadSets.Keys.Aggregate((s1, s2) => VersionCompare(s1, s2) >= 0 ? s1 : s2);
workloadSet = availableWorkloadSets[maxWorkloadSetVersion.ToString()];
Expand Down Expand Up @@ -278,7 +278,7 @@ public WorkloadVersionInfo GetWorkloadVersion()
sb.Append(bytes[b].ToString("x2"));
}

return new WorkloadVersionInfo($"{_sdkVersionBand.ToStringWithoutPrerelease()}-manifests.{sb}", IsInstalled: true, WorkloadSetsEnabledWithoutWorkloadSet: installState.UseWorkloadSets == true);
return new WorkloadVersionInfo($"{_sdkVersionBand.ToStringWithoutPrerelease()}-manifests.{sb}", IsInstalled: true, WorkloadSetsEnabledWithoutWorkloadSet: installState.ShouldUseWorkloadSets());
}
}

Expand Down
6 changes: 4 additions & 2 deletions ...crosoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,7 @@ public void ItUsesWorkloadManifestFromInstallState()
CreateMockInstallState("8.0.200",
"""
{
"useWorkloadSets": false,
"manifests": {
"ios": "11.0.1/8.0.100",
}
Expand Down Expand Up @@ -776,9 +777,10 @@ public void ItFailsIfManifestFromInstallStateIsNotInstalled()
var installStatePath = CreateMockInstallState("8.0.200",
"""
{
"manifests": {
"useWorkloadSets": false,
"manifests": {
"ios": "12.0.2/8.0.200",
}
},
}
""");

Expand Down
26 changes: 15 additions & 11 deletions test/dotnet-workload-install.Tests/GivenDotnetWorkloadInstall.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.DotNet.Cli.NuGetPackageDownloader;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.DotNet.Workloads.Workload;
using Microsoft.DotNet.Workloads.Workload.Config;
using Microsoft.DotNet.Workloads.Workload.Install;
using Microsoft.DotNet.Workloads.Workload.List;
using Microsoft.Extensions.EnvironmentAbstractions;
Expand Down Expand Up @@ -67,7 +68,7 @@ public void GivenWorkloadInstallItCanInstallPacks(bool userLocal, string sdkVers
{
var mockWorkloadIds = new WorkloadId[] { new WorkloadId("xamarin-android") };
var parseResult = Parser.Instance.Parse(new string[] { "dotnet", "workload", "install", "xamarin-android", "--skip-manifest-update" });
(_, var installManager, var installer, _, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, installedFeatureBand: sdkVersion);
(_, var installManager, var installer, _, _, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, installedFeatureBand: sdkVersion);

installManager.Execute()
.Should().Be(0);
Expand All @@ -88,7 +89,7 @@ public void GivenWorkloadInstallItCanRollBackPackInstallation(bool userLocal, st
{
var mockWorkloadIds = new WorkloadId[] { new WorkloadId("xamarin-android"), new WorkloadId("xamarin-android-build") };
var parseResult = Parser.Instance.Parse(new string[] { "dotnet", "workload", "install", "xamarin-android", "xamarin-android-build", "--skip-manifest-update" });
(_, var installManager, var installer, var workloadResolver, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, failingWorkload: "xamarin-android-build", installedFeatureBand: sdkVersion);
(_, var installManager, var installer, var workloadResolver, _, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, failingWorkload: "xamarin-android-build", installedFeatureBand: sdkVersion);

var exceptionThrown = Assert.Throws<GracefulException>(() => installManager.Execute());
exceptionThrown.Message.Should().Contain("Failing workload: xamarin-android-build");
Expand Down Expand Up @@ -128,8 +129,9 @@ public void GivenWorkloadInstallOnFailingRollbackItDisplaysTopLevelError()
public void GivenWorkloadInstallItCanUpdateAdvertisingManifests(bool userLocal, string sdkVersion)
{
var parseResult = Parser.Instance.Parse(new string[] { "dotnet", "workload", "install", "xamarin-android" });
(_, var installManager, var installer, _, var manifestUpdater, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, installedFeatureBand: sdkVersion);
(_, var installManager, var installer, _, var manifestUpdater, _, var resolverFactory) = GetTestInstallers(parseResult, userLocal, sdkVersion, installedFeatureBand: sdkVersion);

new WorkloadConfigCommand(Parser.Instance.Parse(["dotnet", "workload", "config", "--update-mode", "manifests"]), workloadResolverFactory: resolverFactory).Execute().Should().Be(0);
installManager.Execute()
.Should().Be(0);

Expand Down Expand Up @@ -216,9 +218,10 @@ public void GivenWorkloadInstallItCanUpdateInstalledManifests(bool userLocal, st
{
new(new ManifestVersionUpdate(new ManifestId("mock-manifest"), new ManifestVersion("2.0.0"), featureBand.ToString()), null),
};
(_, var installManager, var installer, _, _, _) =
(_, var installManager, var installer, _, _, _, var resolverFactory) =
GetTestInstallers(parseResult, userLocal, sdkVersion, manifestUpdates: manifestsToUpdate, installedFeatureBand: sdkVersion);

new WorkloadConfigCommand(Parser.Instance.Parse(["dotnet", "workload", "config", "--update-mode", "manifests"]), workloadResolverFactory: resolverFactory).Execute().Should().Be(0);
installManager.Execute()
.Should().Be(0);

Expand Down Expand Up @@ -247,9 +250,10 @@ public void GivenWorkloadInstallFromCacheItInstallsCachedManifest(bool userLocal
{
"dotnet", "workload", "install", "xamarin-android", "--from-cache", cachePath
});
(_, var installManager, var installer, _, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion,
(_, var installManager, var installer, _, _, _, var resolverFactory) = GetTestInstallers(parseResult, userLocal, sdkVersion,
tempDirManifestPath: _manifestPath, manifestUpdates: manifestsToUpdate, installedFeatureBand: sdkVersion);

new WorkloadConfigCommand(Parser.Instance.Parse(["dotnet", "workload", "config", "--update-mode", "manifests"]), workloadResolverFactory: resolverFactory).Execute().Should().Be(0);
installManager.Execute();

installer.InstalledManifests[0].manifestUpdate.ManifestId.Should().Be(manifestsToUpdate[0].ManifestUpdate.ManifestId);
Expand All @@ -267,7 +271,7 @@ public void GivenWorkloadInstallItCanDownloadToOfflineCache(bool userLocal, stri
{
var cachePath = Path.Combine(_testAssetsManager.CreateTestDirectory(identifier: AppendForUserLocal("mockCache_", userLocal) + sdkVersion).Path, "mockCachePath");
var parseResult = Parser.Instance.Parse(new string[] { "dotnet", "workload", "install", "xamarin-android", "--download-to-cache", cachePath });
(_, var installManager, _, _, var manifestUpdater, var packageDownloader) = GetTestInstallers(parseResult, userLocal, sdkVersion, tempDirManifestPath: _manifestPath, installedFeatureBand: sdkVersion);
(_, var installManager, _, _, var manifestUpdater, var packageDownloader, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, tempDirManifestPath: _manifestPath, installedFeatureBand: sdkVersion);

installManager.Execute();

Expand All @@ -291,7 +295,7 @@ public void GivenWorkloadInstallItCanInstallFromOfflineCache(bool userLocal, str
var mockWorkloadIds = new WorkloadId[] { new WorkloadId("xamarin-android") };
var cachePath = "mockCachePath";
var parseResult = Parser.Instance.Parse(new string[] { "dotnet", "workload", "install", "xamarin-android", "--from-cache", cachePath });
(_, var installManager, var installer, _, _, var nugetDownloader) = GetTestInstallers(parseResult, userLocal, sdkVersion, installedFeatureBand: sdkVersion);
(_, var installManager, var installer, _, _, var nugetDownloader, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, installedFeatureBand: sdkVersion);

installManager.Execute();

Expand All @@ -311,7 +315,7 @@ public void GivenWorkloadInstallItCanInstallFromOfflineCache(bool userLocal, str
public void GivenWorkloadInstallItPrintsDownloadUrls(bool userLocal, string sdkVersion)
{
var parseResult = Parser.Instance.Parse(new string[] { "dotnet", "workload", "install", "xamarin-android", "--print-download-link-only" });
(_, var installManager, _, _, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, tempDirManifestPath: _manifestPath, installedFeatureBand: sdkVersion);
(_, var installManager, _, _, _, _, _) = GetTestInstallers(parseResult, userLocal, sdkVersion, tempDirManifestPath: _manifestPath, installedFeatureBand: sdkVersion);

installManager.Execute();

Expand Down Expand Up @@ -455,7 +459,7 @@ static void CreateFile(string path)
.Should().BeEquivalentTo(new string[] { prev7FormattedFeatureVersion, rc1FormattedFeatureVersion });
}

private (string, WorkloadInstallCommand, MockPackWorkloadInstaller, IWorkloadResolver, MockWorkloadManifestUpdater, MockNuGetPackageDownloader) GetTestInstallers(
private (string, WorkloadInstallCommand, MockPackWorkloadInstaller, IWorkloadResolver, MockWorkloadManifestUpdater, MockNuGetPackageDownloader, IWorkloadResolverFactory) GetTestInstallers(
ParseResult parseResult,
bool userLocal,
string sdkVersion,
Expand Down Expand Up @@ -491,7 +495,7 @@ static void CreateFile(string path)
nugetPackageDownloader: nugetDownloader,
workloadManifestUpdater: manifestUpdater);

return (testDirectory, installManager, installer, workloadResolver, manifestUpdater, nugetDownloader);
return (testDirectory, installManager, installer, workloadResolver, manifestUpdater, nugetDownloader, workloadResolverFactory);
}

[Fact]
Expand Down Expand Up @@ -627,7 +631,7 @@ public void ShowManifestUpdatesWhenVerbosityIsDetailedOrDiagnostic(string verbos
{
new(new ManifestVersionUpdate(new ManifestId("mock-manifest"), new ManifestVersion("2.0.0"), sdkFeatureBand), null),
};
(_, var installManager, _, _, _, _) =
(_, var installManager, _, _, _, _, _) =
GetTestInstallers(parseResult, true, sdkFeatureBand, manifestUpdates: manifestsToUpdate);

installManager.Execute().Should().Be(0);
Expand Down
Loading
Loading