Permit specifying to use workload versions in global.json #47421
Conversation
ghost
added
|
nit: try to make this align with the existing |
That seems reasonable. I'll probably just handle that in the parsing bit so I don't have to change anything else, but no one else will be able to tell 🙂 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for specifying to use workload versions in global.json by updating how workload set modes are determined and applied. Key changes include:
- Updating the logic in WorkloadConfigCommand, InstallingWorkloadCommand, and WorkloadUpdateCommand to integrate global.json workload version specifications.
- Adjusting GlobalJsonWorkloadSetFile and WorkloadListCommand to properly handle workload set configurations.
- Extending IWorkloadManifestProvider’s record struct to carry global.json workload set information.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Cli/dotnet/commands/dotnet-workload/config/WorkloadConfigCommand.cs | Integrated logic to derive workload set mode from global.json |
| src/Cli/dotnet/commands/dotnet-workload/list/WorkloadListCommand.cs | Added condition to check for global.json workload set specification |
| src/Cli/dotnet/commands/InstallingWorkloadCommand.cs | Updated workload installation and error handling to consider global.json values |
| src/Cli/dotnet/commands/dotnet-workload/GlobalJsonWorkloadSetFile.cs | Modified parsing of global.json to include an out parameter for workload version |
| src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs | Revised manifest update invocation to apply global.json workload set configuration |
| src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/IWorkloadManifestProvider.cs | Extended the workload version info record with a new global.json field |
src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs
Outdated
Show resolved
Hide resolved
|
At a glance this looks good. Can you update the description with examples of how this should be specified in global.json? |
| @@ -78,7 +78,8 @@ 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")).ShouldUseWorkloadSets(); | |||
| versionInfo.GlobalJsonSpecifiesWorkloadSets == true || | |||
There was a problem hiding this comment.
Is this logic that should go in WorkloadManifestUpdater.ShouldUseWorkloadSetMode instead? Same with shouldUseWorkloadSetsPerGlobalJson in the update command. Not sure how feasible that is but I'm surprised that is still using 'default.json' which I'm not sure what thats doing but I assume its like a default global.json state.
There was a problem hiding this comment.
Two things to disentangle here:
First, this can't really go in WorkloadManifestUpdater.ShouldUseWorkloadSetMode because we have different ideas about where global.json files should be found in different places. Specifically, this uses the workload resolver's idea, which can be influenced by how the resolver is created—important for gc—whereas update just uses the current directory. Could WorkloadListCommand skip the resolver entirely? For this yes, but for other things no, and I'd rather be consistent.
Second, these are different 'kinds' of should-use-workload-sets checks. The global.json check is obviously from the global.json, but the default.json check is from the install state file, which is global unlike the (repo-local) global.json.
Good question, though
|
|
||
| var installStatePath = Path.Combine(dotnetDir, "metadata", "workloads", RuntimeInformation.ProcessArchitecture.ToString(), sdkVersion, "InstallState", "default.json"); | ||
| var contents = new InstallStateContents(); | ||
| contents.UseWorkloadSets = true; | ||
| contents.UseWorkloadSets = installStateUseWorkloadSet; |
There was a problem hiding this comment.
I'm glad you updated this test case and it feels much more valuable now 👏
| @@ -68,7 +68,11 @@ public override int Execute() | |||
| } | |||
| else if (string.IsNullOrEmpty(_updateMode)) | |||
| { | |||
| if (InstallingWorkloadCommand.ShouldUseWorkloadSetMode(_sdkFeatureBand, _dotnetPath)) | |||
| string globalJsonPath = SdkDirectoryWorkloadManifestProvider.GetGlobalJsonPath(Environment.CurrentDirectory); | |||
There was a problem hiding this comment.
Note to self: I should probably turn this into errors if it doesn't align properly
Forgind
force-pushed
the
globaljsonworkloadsets
branch
from
4e009df to
b833843
Compare
Uses workloads-update-mode next to where you'd put a specific workload version to determine if we should be in workload set mode when doing install/update. If there's a conflict between this and other flags, it should error.
Example: