Support setting workload set versions in global.json #33441
Conversation
This will allow the workload manifest resolver to read any workload version specified in global.json
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
dsplaisted
requested review from
mhutch,
a team,
joeloff,
MiYanni,
nagilson and
JL03-Yue
| @@ -43,7 +43,7 @@ internal class WorkloadUninstallCommand : WorkloadCommandBase | |||
| userProfileDir = userProfileDir ?? CliFolderPathCalculator.DotnetUserProfileFolderPath; | |||
| _sdkVersion = WorkloadOptionsExtensions.GetValidatedSdkVersion(parseResult.GetValueForOption(WorkloadUninstallCommandParser.VersionOption), version, dotnetPath, userProfileDir, true); | |||
|
|
|||
| var workloadManifestProvider = new SdkDirectoryWorkloadManifestProvider(dotnetPath, _sdkVersion.ToString(), userProfileDir); | |||
| var workloadManifestProvider = new SdkDirectoryWorkloadManifestProvider(dotnetPath, _sdkVersion.ToString(), userProfileDir, SdkDirectoryWorkloadManifestProvider.GetGlobalJsonPath(Environment.CurrentDirectory)); | |||
There was a problem hiding this comment.
Can this be moved into a base constructor? Feels like a lot of repetition.
There was a problem hiding this comment.
@joeloff I've now refactored most of the resolver creation calls into a helper class. Try to look closely at whether the new code has the same behavior as the old code, I tried to be careful but it would be easy to make a mistake.
There was a problem hiding this comment.
Looks consistent to me. All the default behavior appears to be on the factory side.
...soft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.GlobalJsonReader.cs
Outdated
Show resolved
Hide resolved
| { | ||
| "sdk": { | ||
| "version": "8.0.200", | ||
| "workloadversion": "8.0.201" |
There was a problem hiding this comment.
The global.json reader looks for workloadVersion. Shouldn't this through an exception because the property is all lower case?
There was a problem hiding this comment.
It's doing case-insensitive comparison. I'm not sure whether global.json is supposed to be case-sensitive or not, but I followed the pattern from the workload manifest parsing code, which is case-insensitive.
There was a problem hiding this comment.
I went over the global.json docs. It doesn't mention anything from what I could tell. I'm fine if it's case-insensitive
There was a problem hiding this comment.
As I mentioned to you in chat, the runtime treats global.json properties as case sensitive. We should probably be consistent with the runtime.
There was a problem hiding this comment.
It's hard for me to see a downside for being case insensitive. What would the would that break? On the other hand, if we are case sensitive, some people will probably get the casing wrong and be confused as to why the setting isn't taking effect.
There was a problem hiding this comment.
We can mitigate that somewhat by updating the Schemastore json schema for global.json (which lives here) with this field - this enables editors to flag this error.
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/Strings.resx
Outdated
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Show resolved
Hide resolved
src/Cli/dotnet/commands/dotnet-workload/uninstall/WorkloadUninstallCommand.cs
Outdated
Show resolved
Hide resolved
| DotnetPath = dotnetDir, | ||
| UserProfileDir = userProfileDir, | ||
| GlobalJsonStartDir = null, | ||
| VersionFromOption = parseResult.GetValueForOption(InstallingWorkloadCommandParser.VersionOption), |
There was a problem hiding this comment.
What kind of version is this versionFromOption? The SDK Version? Workload version? Workload set version? I would suggest making the variable name a little more specific
.../Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs
Show resolved
Hide resolved
src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
Show resolved
Hide resolved
...soft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.GlobalJsonReader.cs
Show resolved
Hide resolved
Implements part of dotnet/designs#294
We may want to add some end-to-end tests.