Support package@version syntax for dotnet workload search/update/install version Fixes #42367#45156
Conversation
Using `dotnet workload search version`, you should now be able to provide package@version as an argument, and it will find the highest-valued set that has that package at that version
…search-version-with-component
| Reporter.WriteLine(string.Join('\n', versions)); | ||
| } | ||
| } | ||
| else if (_workloadVersion.Contains('@')) |
There was a problem hiding this comment.
Because of this usage, I don't think _workloadVersion is really a good name anymore. I'm leaving it for now for two reasons:
- We might switch to --component (or something) later, in which case changing it is unnecessary.
- Having two possible meanings means I'm not quickly coming up with a good name.
We should change it (if we don't switch to --component) before merging.
There was a problem hiding this comment.
Yeah, it seems we are kind of mixing up different commands. It looks like currently dotnet workload search version gives you a list of available workload set versions for the current feature band, dotnet workload search version 9.0.100 gives you the manifest versions for the specified workload set version (same as the rollback file basically), and dotnet workload search version maui@9.0.0 gives you the workload set versions with the specified manifest version that contains the specified workload.
@baronfel What do you think? Should we be designing these commands differently?
dsplaisted
left a comment
There was a problem hiding this comment.
Thanks, this is good progress.
| { | ||
| return stableVersions.OrderByDescending(r => r.package.Identity.Version).Take(numberOfResults); | ||
| var results = stableVersions.OrderByDescending(r => r.package.Identity.Version); | ||
| return numberOfResults > 0 ? results.Take(numberOfResults) : results; |
There was a problem hiding this comment.
What's the reason for this change? Does Take not work the way we want when there are fewer available items than requested?
There was a problem hiding this comment.
I don't think I made this very clear. I initially made this method to return a certain number of workload sets, but once I added support for @ syntax, I needed a way to indicate 'get all versions', and 0 is sometimes used as a special indicator for 'all'
How would you suggest making that clearer? Would a comment suffice?
There was a problem hiding this comment.
OK, that makes sense, I was thinking numberOfResults was the actual number of results returned by the query, not the requested number. So it makes sense now.
| return; | ||
| } | ||
|
|
||
| if (resolvedWorkloadSetVersion?.Contains('@') == true) |
There was a problem hiding this comment.
For install and update, it's the workload ID argument that should be used to specify the version. For example, dotnet workload install ios@17.5.9231-net9-p7 tvos@17.5.9232-net9-p7. Note that the different workloads are separated by a space here, not a semicolon.
There was a problem hiding this comment.
This doesn't align with the guidance marcpopMSFT provided:
We decided to just use the existing --version or workloadVersion in the global.json for this and look for the @ symbol.
Did I miss a meeting on this in which we changed the plan?
| Reporter.WriteLine(string.Join('\n', versions)); | ||
| } | ||
| } | ||
| else if (_workloadVersion.Contains('@')) |
There was a problem hiding this comment.
Yeah, it seems we are kind of mixing up different commands. It looks like currently dotnet workload search version gives you a list of available workload set versions for the current feature band, dotnet workload search version 9.0.100 gives you the manifest versions for the specified workload set version (same as the rollback file basically), and dotnet workload search version maui@9.0.0 gives you the workload set versions with the specified manifest version that contains the specified workload.
@baronfel What do you think? Should we be designing these commands differently?
| } | ||
| catch (NuGetPackageNotFoundException) | ||
| { | ||
| Microsoft.DotNet.Cli.Utils.Reporter.Error.WriteLine(string.Format(LocalizableStrings.NoWorkloadVersionsFound, featureBand)); |
There was a problem hiding this comment.
I would probably not output a message from this method if no packages were found, and leave it to callers to handle that. That's the behavior I would be more likely to expect when calling a method to get the versions, and it would let us have more specific messages for each case (ie "No workload sets were found for the current feature band" vs "No workload sets were found with the specified workload version(s)").
|
|
||
| public string FindBestWorkloadSetFromComponents() | ||
| { | ||
| var versions = GetVersions(0); |
There was a problem hiding this comment.
Does 0 here mean to get all the versions? It would probably be good to clarify this.
There was a problem hiding this comment.
Yes. Do you think a comment would suffice, or how else would you recommend making it clearer?
| }); | ||
|
|
||
| // Since these are ordered by version (descending), the first is the highest version | ||
| return versions.FirstOrDefault(version => |
There was a problem hiding this comment.
When doing a search, we should display all of the matching workload set versions. When doing an update or install, we should take the highest workload set version that matches.
| public WorkloadManifest(string id) : this(id, new FXVersion(7, 3, 5), null, string.Empty, [], [], []) { } | ||
|
|
There was a problem hiding this comment.
This looks like it's just for tests, but that wouldn't be obvious if you were trying to create a WorkloadManifest in the product code. Could you do one of the following?
- Create a helper method in the tests that creates a WorkloadManifest with some default values given an ID?
- Instead of a constructor, have a static
CreateForTestsfactory method on theWorkloadManifestclass so that it's clear what the purpose of the method is.
There was a problem hiding this comment.
Good point. I like the idea of the static CreateForTests factory method; will implement that.
| if (resolvedWorkloadSetVersion?.Contains('@') == true) | ||
| { | ||
| var searchVersionsCommand = new WorkloadSearchVersionsCommand( | ||
| Parser.Instance.Parse("dotnet workload search version " + resolvedWorkloadSetVersion), |
There was a problem hiding this comment.
If we have to create an instance of another command class, I think it's better if we don't generate a command line and parse it. Rather, we should have constructor parameters or class properties that let us pass in the data we need.
| var packageNamesAndVersions = workloadVersions.Select(version => | ||
| { | ||
| var split = version.Split('@'); | ||
| return (new ManifestId(_resolver.GetManifestFromWorkload(new WorkloadId(split[0])).Id), new ManifestVersion(split[1])); |
There was a problem hiding this comment.
GetManifestFromWorkload has to actually download and extract the NuGet package. We might want to add some parallelism to those downloads, but it's probably a bit tricky to do so safely, so I would leave it as it is for now.
|
Other variants are:
|
|
@dsplaisted I think I addressed all your feedback and the feedback from the design meeting earlier except for tweaking the help text and making it clearer what the 0 means 🙂 I'll fix the help text and add a comment tomorrow. |
I don't think this is really connected to this PR, but improving our CLI is a direction we'd like to go, and I agree that @ seems like a better separator for this sort of thing than :: I'm going to turn this into a separate issue for triage if that's ok with you! |
…search-version-with-component
|
|
||
| if (versions is null) | ||
| { | ||
| return; |
There was a problem hiding this comment.
What does it mean if versions is null here?
There was a problem hiding this comment.
This can only happen if we got a NuGetPackageNotFoundException when we tried to find any workload sets (regardless of whether they matched). In that case, a message should already have been sent to the user about what the issue was, so I just added a return; to prevent duplicate messages.
| } | ||
| else if (SpecifiedWorkloadSetVersionOnCommandLine) | ||
| { | ||
| resolvedWorkloadSetVersion = _workloadSetVersionFromCommandLine.Single(); |
There was a problem hiding this comment.
What happens if you run dotnet workload search version 9.0.100 9.0.101? It looks like it might fail with a LINQ error, which isn't user-friendly.
There was a problem hiding this comment.
Yep! I noticed that as part of my testing and added a validator to the command parser:
https://github.com/dotnet/sdk/pull/45156/files#diff-77f29f3a81e78911bdd0a0c1415d295295f17b18a8dbc622f89e920e243554daL63
There was a problem hiding this comment.
Note that for this case, I specifically requested exactly 1 workload set, so there should never be more than 1, and we already checked for 0.
| public static readonly CliOption<IEnumerable<string>> WorkloadSetVersionOption = new("--version") | ||
| { | ||
| Description = Strings.WorkloadSetVersionOptionDescription | ||
| Description = Strings.WorkloadSetVersionOptionDescription, |
There was a problem hiding this comment.
The help text you added is great, but I think the option text should also be updated to indicate it can be used in two ways. Something like "A workload version to display, or a workload and its version joined by the '@' character.
| // This method should never be called for this kind of installer. It is challenging to get this information from an MSI | ||
| // and totally unnecessary as the information is identical from a file-based installer. It was added to IInstaller only | ||
| // to facilitate testing. As a consequence, it does not need to be implemented. | ||
| public WorkloadSet GetWorkloadSetContents(string workloadVersion) => throw new NotImplementedException(); |
There was a problem hiding this comment.
We actually do this already for MSIs. Check out the ExtractManifestAsync implementation. In fact, you may be able to implement GetWorkloadSetContents in terms of that, possibly only in test code instead of in the product.
There was a problem hiding this comment.
I don't know if I'm understanding this correctly, but it looks like it extracts information by installing the MSI?
That seems like it would change what you have installed, which wouldn't necessarily be what we'd want.
There was a problem hiding this comment.
It does an "admin install", which means it doesn't actually install it, it just extracts the MSI to the target path.
| microsoft.net.workload.mono.toolchain.net7 9.0.100-rc.1 9.0.0-rc.1.24431.7 | ||
| microsoft.net.workload.mono.toolchain.net8 9.0.100-rc.1 9.0.0-rc.1.24431.7 | ||
| microsoft.net.sdk.aspire 8.0.100 8.2.0 | ||
| 3. If a set of workloads are provided along with their versions, it outputs workload versions that match the provided versions. Takes the --take option to specify how many to provide and --format to alter the format. |
There was a problem hiding this comment.
| 3. If a set of workloads are provided along with their versions, it outputs workload versions that match the provided versions. Takes the --take option to specify how many to provide and --format to alter the format. | |
| 3. If one or more workloads are provided along with their versions (by joining them with the '@' character), it outputs workload versions that match the provided versions. Takes the --take option to specify how many to provide and --format to alter the format. |
| else | ||
| { | ||
| var workloadSet = _installer.GetWorkloadSetContents(_workloadVersion); | ||
| var workloadSet = _installer.GetWorkloadSetContents(_workloadVersion.Last()); |
There was a problem hiding this comment.
Probably it should be an error if you specify two workload set versions, instead of just taking the last one.
There was a problem hiding this comment.
It is! (Via the command parser rather than the command). Last, First, and Single should all be equivalent here. I only chose Last because I'd initially been thinking about not making it an error and decided on taking the last one, then decided that wasn't the best design and ended up at the same place as you.
There was a problem hiding this comment.
I changed it to Single for clarity
| return null; | ||
| } | ||
|
|
||
| var packageNamesAndVersions = workloadVersions.Select(version => |
There was a problem hiding this comment.
Nit: better name
| var packageNamesAndVersions = workloadVersions.Select(version => | |
| var manifestIdsAndVersions = workloadVersions.Select(version => |
Does |
I was surprised by this, but this appears to be intentional...there's a WriteSubcommand function when printing help text that is intended to tell you about all the subcommands, and that's exactly what this does. On the one hand, this does feel too verbose to me. Perhaps we should cut it for dotnet workload search version. But on the other hand, what exactly that subcommand does is a bit confusing in the first place, hence the unusually complete description; I'm not sure I could craft a short description that clearly explains what all three possibilities are. @baronfel, do you think we should keep the current (verbose but clear) text or try to shorten it? If the latter, I think I'd just put in a "if you're writing help for dotnet workload search, use this instead of the normal subcommand description." |
|
NTS: Something like Line 240 in 9c97bb9 |
Apparently ios isn't found on windows x64?
|
@Forgind Is this ready to merge, or are you waiting for more feedback or for the CI issues to be fixed? |
| </data> | ||
| <data name="ShortWorkloadSearchVersionDescription" xml:space="preserve"> | ||
| <value>Search for available workload versions or what comprises a workload version. Use 'dotnet workload search version --help' for more information.</value> | ||
| <comment>dotnet workload search version --help should not be localized.</comment> |
There was a problem hiding this comment.
nit: use the locking syntax for pinned translation strings so that the Loc team's tooling helps prevent unintentional translation.


Fixes #42367
Using
dotnet workload search version, you should now be able to provide package@version as an argument, and it will find the highest-valued workload version that has that package at that version.Sample when no such workload version was found:

When 3 were found:

(Note that the other two were 9.0.200.1 and 9.0.200, so this is the highest version number.)
There are three total uses of dotnet workload search version that previously existed:
Without any argument, it lists out the most recent SDKs (in the current feature band):
When provided with a workload version, it lists out all the workloads in that workload version:
(Note that this was a manually created 9.0.201 version, so those versions do not align with a real one.)
And as mentioned earlier, when one or more workloads along with versions (concatenated with '@'), it finds a workload version that includes all of the provided workloads at the associated versions.
This PR also dramatically changes the help text either when you use

dotnet workload search version -?or when you mistype a command. It now looks like this: