Support package@version syntax for dotnet workload search/update/install version Fixes #42367 #45156

Forgind · 2024-11-26T18:58:00Z

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):

dotnet workload search version --format json

When provided with a workload version, it lists out all the workloads in that workload version:

dotnet workload search version 9.0.201

(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.

dotnet workload search version maui@9.0.0-rc.1.24453.9 ios@17.5.9270-net9-rc1

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:

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

Forgind · 2024-11-27T00:12:53Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

@@ -100,6 +90,30 @@ public override int Execute()
                    Reporter.WriteLine(string.Join('\n', versions));
                }
            }
+            else if (_workloadVersion.Contains('@'))


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.

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

Thanks, this is good progress.

dsplaisted · 2024-12-11T01:51:26Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

@@ -567,7 +567,8 @@ await Task.WhenAll(

                if (stableVersions.Any())
                {
-                    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;


What's the reason for this change? Does Take not work the way we want when there are fewer available items than requested?

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?

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.

dsplaisted · 2024-12-11T02:07:04Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

@@ -242,6 +243,20 @@ protected void UpdateWorkloadManifests(WorkloadHistoryRecorder recorder, ITransa
                return;
            }

+            if (resolvedWorkloadSetVersion?.Contains('@') == true)


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.

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.

#42367 (comment)

Did I miss a meeting on this in which we changed the plan?

dsplaisted · 2024-12-11T02:19:21Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

@@ -100,6 +90,30 @@ public override int Execute()
                    Reporter.WriteLine(string.Join('\n', versions));
                }
            }
+            else if (_workloadVersion.Contains('@'))


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 · 2024-12-11T02:22:46Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

+            }
+            catch (NuGetPackageNotFoundException)
+            {
+                Microsoft.DotNet.Cli.Utils.Reporter.Error.WriteLine(string.Format(LocalizableStrings.NoWorkloadVersionsFound, featureBand));


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)").

dsplaisted · 2024-12-11T02:26:19Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

+
+        public string FindBestWorkloadSetFromComponents()
+        {
+            var versions = GetVersions(0);


Does 0 here mean to get all the versions? It would probably be good to clarify this.

Yes. Do you think a comment would suffice, or how else would you recommend making it clearer?

dsplaisted · 2024-12-11T02:28:24Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

+            });
+
+            // Since these are ordered by version (descending), the first is the highest version
+            return versions.FirstOrDefault(version =>


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.

dsplaisted · 2024-12-11T02:31:20Z

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadManifest.cs

+        public WorkloadManifest(string id) : this(id, new FXVersion(7, 3, 5), null, string.Empty, [], [], []) { }
+


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 CreateForTests factory method on the WorkloadManifest class so that it's clear what the purpose of the method is.

Good point. I like the idea of the static CreateForTests factory method; will implement that.

dsplaisted · 2024-12-11T02:36:19Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+            if (resolvedWorkloadSetVersion?.Contains('@') == true)
+            {
+                var searchVersionsCommand = new WorkloadSearchVersionsCommand(
+                     Parser.Instance.Parse("dotnet workload search version " + resolvedWorkloadSetVersion),


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.

dsplaisted · 2024-12-11T02:38:25Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

+            var packageNamesAndVersions = workloadVersions.Select(version =>
+            {
+                var split = version.Split('@');
+                return (new ManifestId(_resolver.GetManifestFromWorkload(new WorkloadId(split[0])).Id), new ManifestVersion(split[1]));


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.

am11 · 2024-12-11T10:32:37Z

Other variants are:

dotnet add <project> package <name> --version <ver>
dotnet new install <name>::<ver>

@ syntax is certainly much better than :: syntax chosen for dotnet-new template install command. e.g. https://www.nuget.org/packages/BenchmarkDotNet.Templates shows dotnet new install BenchmarkDotNet.Templates::0.14.0. It would be nice to streamline these by deprecating :: delimiter in favor of @ and support @ in dotnet add package as well.

Forgind · 2024-12-12T02:47:15Z

@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.

Forgind · 2024-12-12T02:51:39Z

Other variants are:

dotnet add <project> package <name> --version <ver>

dotnet new install <name>::<ver>

@ syntax is certainly much better than :: syntax chosen for dotnet-new template install command. e.g. https://www.nuget.org/packages/BenchmarkDotNet.Templates shows dotnet new install BenchmarkDotNet.Templates::0.14.0. It would be nice to streamline these by deprecating :: delimiter in favor of @ and support @ in dotnet add package as well.

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

dsplaisted

Looks good, thanks!

dsplaisted · 2024-12-13T20:56:14Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+
+                if (versions is null)
+                {
+                    return;


What does it mean if versions is null here?

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.

dsplaisted · 2024-12-13T20:57:21Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

+            }
+            else if (SpecifiedWorkloadSetVersionOnCommandLine)
+            {
+                resolvedWorkloadSetVersion = _workloadSetVersionFromCommandLine.Single();


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.

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

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.

dsplaisted · 2024-12-13T21:02:08Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

        {
-            Description = Strings.WorkloadSetVersionOptionDescription
+            Description = Strings.WorkloadSetVersionOptionDescription,


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.

dsplaisted · 2024-12-13T21:05:29Z

src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs

+        // 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();


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.

I don't know if I'm understanding this correctly, but it looks like it extracts information by installing the MSI?

sdk/src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs

Line 792 in 0113afe

var result = WindowsInstaller.InstallProduct(msiPath, $"TARGETDIR={msiExtractionPath} ACTION=ADMIN");

That seems like it would change what you have installed, which wouldn't necessarily be what we'd want.

It does an "admin install", which means it doesn't actually install it, it just extracts the MSI to the target path.

dsplaisted · 2024-12-13T21:25:11Z

src/Cli/dotnet/commands/dotnet-workload/search/LocalizableStrings.resx

+           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.


Suggested change

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.

dsplaisted · 2024-12-13T21:34:20Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

@@ -102,7 +123,7 @@ public override int Execute()
            }
            else
            {
-                var workloadSet = _installer.GetWorkloadSetContents(_workloadVersion);
+                var workloadSet = _installer.GetWorkloadSetContents(_workloadVersion.Last());


Probably it should be an error if you specify two workload set versions, instead of just taking the last one.

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.

I changed it to Single for clarity

dsplaisted · 2024-12-13T21:39:36Z

src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchVersionsCommand.cs

+                return null;
+            }
+
+            var packageNamesAndVersions = workloadVersions.Select(version =>


Nit: better name

Suggested change

var packageNamesAndVersions = workloadVersions.Select(version =>

var manifestIdsAndVersions = workloadVersions.Select(version =>

KalleOlaviNiemitalo · 2024-12-16T14:32:54Z

This PR also dramatically changes the help text either when you use dotnet workload search version -? or when you mistype a command.

Does dotnet workload search --help (without version) also show the 29-line description of dotnet workload search version? IMO it shouldn't output so much text. Instead of putting all of that into the description, I think it would better as a separate help section.

Forgind · 2024-12-17T16:38:11Z

This PR also dramatically changes the help text either when you use dotnet workload search version -? or when you mistype a command.

Does dotnet workload search --help (without version) also show the 29-line description of dotnet workload search version? IMO it shouldn't output so much text. Instead of putting all of that into the description, I think it would better as a separate help section.

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."

Picture to give you an idea of what it looks like:

Forgind · 2024-12-17T16:46:16Z

NTS: Something like

sdk/src/Cli/dotnet/Parser.cs

Line 240 in 9c97bb9

builder.CustomizeSymbol(option, secondColumnText: descriptionCallback);

should work

Apparently ios isn't found on windows x64?

Forgind · 2024-12-20T21:50:00Z

New version:

dsplaisted · 2025-01-06T20:35:24Z

@Forgind Is this ready to merge, or are you waiting for more feedback or for the CI issues to be fixed?

baronfel · 2025-01-06T20:38:34Z

src/Cli/dotnet/CommonLocalizableStrings.resx

@@ -715,6 +715,10 @@ The default is 'false.' However, when targeting .NET 7 or lower, the default is
  <data name="ResponseFileNotFound" xml:space="preserve">
    <value>Response file '{0}' does not exist.</value>
  </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>


nit: use the locking syntax for pinned translation strings so that the Loc team's tooling helps prevent unintentional translation.

Attempt to support package@version syntax

a007add

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

dotnet-issue-labeler bot added Area-Workloads untriaged Request triage from a team member labels Nov 26, 2024

Forgind marked this pull request as draft November 26, 2024 21:04

Forgind added 2 commits November 26, 2024 19:05

Support multiple workloads

60f0fd2

Merge branch 'release/9.0.2xx' of https://github.com/dotnet/sdk into …

0ab26de

…search-version-with-component

Forgind commented Nov 27, 2024

View reviewed changes

Forgind marked this pull request as ready for review November 27, 2024 00:30

build-analysis bot mentioned this pull request Nov 27, 2024

Failure on Helix tests - Tests timing out #40074

Open

Switch to using workload instead of workload manifest

b7a88e7

Forgind changed the title ~~Support package@version syntax for dotnet workload search version~~ Support package@version syntax for dotnet workload search version Fixes #42367 Nov 27, 2024

Forgind added 2 commits November 27, 2024 12:51

Support install + update via @ syntax

0b0a000

Add test

7c04c40

Forgind changed the title ~~Support package@version syntax for dotnet workload search version Fixes #42367~~ Support package@version syntax for dotnet workload search/update/install version Fixes #42367 Nov 27, 2024

Add workload search tests

5ded48b

dsplaisted reviewed Dec 11, 2024

View reviewed changes

Forgind added 2 commits December 11, 2024 19:28

Most comments

bcc85ec

Feedback from design meeting

69019c1

Forgind mentioned this pull request Dec 12, 2024

Deprecate :: in favor of @ in dotnet new install command #45422

Open

Forgind added 3 commits December 12, 2024 13:59

More feedback

a194d99

Merge branch 'release/9.0.2xx' of https://github.com/dotnet/sdk into …

731b996

…search-version-with-component

Small improvements

59a023c

build-analysis bot mentioned this pull request Dec 12, 2024

Workload Feature Band Test Sometimes Fails #45314

Open

dsplaisted approved these changes Dec 13, 2024

View reviewed changes

Feedback

e463f09

This was referenced Dec 14, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

Change 'multiple components' style in tests

3085184

This was referenced Dec 15, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

dotnet-watch tests failing #40006

Open

Forgind added 2 commits December 17, 2024 08:58

ios --> android

d03ac02

Apparently ios isn't found on windows x64?

Tweak test to use a mock resolver

f82ef86

Shorten description for version as a subcommand

38e0cf5

baronfel reviewed Jan 6, 2025

View reviewed changes

baronfel approved these changes Jan 6, 2025

View reviewed changes

Use {Locked}

f88b724

Forgind merged commit 0c58854 into dotnet:release/9.0.2xx Jan 9, 2025
28 of 31 checks passed

Forgind deleted the search-version-with-component branch January 9, 2025 21:25

		public WorkloadManifest(string id) : this(id, new FXVersion(7, 3, 5), null, string.Empty, [], [], []) { }

	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.

	var packageNamesAndVersions = workloadVersions.Select(version =>
	var manifestIdsAndVersions = workloadVersions.Select(version =>

Support package@version syntax for dotnet workload search/update/install version Fixes #42367 #45156

Support package@version syntax for dotnet workload search/update/install version Fixes #42367 #45156

Conversation

Forgind commented Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 commented Dec 11, 2024

Forgind commented Dec 12, 2024

Forgind commented Dec 12, 2024

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo commented Dec 16, 2024

Forgind commented Dec 17, 2024

Forgind commented Dec 17, 2024

Forgind commented Dec 20, 2024

dsplaisted commented Jan 6, 2025

Choose a reason for hiding this comment

Forgind commented Nov 26, 2024 •

edited

Loading