First-run workload experience #34322

MiYanni · 2023-07-29T02:43:58Z

Summary

There are certain situations where the user of the CLI might have updated the SDK version which brings in updated workload manifests within the same feature band. The user had workloads installed previously. However, they won't have the updated packages for the workloads that apply to the new manifests.

This PR makes it so that when the first-run experience for an SDK version activates, it (conditionally) runs the process for checking the state of the installed workloads, similar to the process done for the workload update command. For the purposes of this feature, I've called it "workload integrity check" since (conceptually) makes sure you're in a 'good' state with the workloads desired.

Key Aspects

The workload integrity check only runs when the .dotnetFirstUseSentinel for the proper SDK version is not present on-disk.
The .dotnetFirstUseSentinel is created prior to running the workload integrity check. This means that if, for any reason, the workload check doesn't work or run, it won't try to do it again.
The workload integrity check is wrapped in a try/catch so any failures will not hinder the CLI command from executing.
- Update: Added a log message on failure to advise the user to run dotnet workload update if they want the actual error details.
The workload integrity check only runs if there were workloads previously installed for the feature band.
The workload integrity check will not output the NuGet Package Manager logging information (it is quite extensive and unnecessary in most cases).
There is a new environment variable called DOTNET_SKIP_WORKLOAD_INTEGRITY_CHECK. ~~This is set to false by default. If set to true, the workload integrity check will be skipped.~~
- Update: If the environment variable is not present, the default value is set to true (meaning, skip the check) if running in CI. Otherwise, false is the default.
~~If the first-run command is a workload command, the workload integrity check is skipped. This avoids an open file conflict with the workload log file.~~
- Update: Fixed this by updating the workload log filename to include milliseconds to avoid the "file being open" conflict.
If the first-run command is ran in a CI environment, the workload integrity check is skipped. The CI detection for this is the same as what used for telemetry.
Currently, user cancellation is not implemented. A new issue will be opened to outline cancellation implementation.

Testing Notes

Tested this both with MSI and file-based installs.
Tested certain aspects by deleting installed workload packs and/or overriding workload manifests with previous versions.
Tested that workload integrity check failures (via improper manifest version dependencies) are properly caught and ignored by the try/catch.
Tested that the first-run command being a workload command is properly skipping the workload integrity check.
Tested that if a CI-based environment variable is set, the workload integrity check is skipped.

Example Output

Other

PR contains some minor unrelated changes such as whitespace or comments in other files. These were done during development and investigation, and do not include functional changes.

…stallWorkloads method during first run. This may be temporary. Cleaned up unused usings in various files. Cleaned up various stray spaces and extra carriage returns. Other various minor edits.

…leanup PR.

…CI can set it to true to no longer run the workload update.

…running in CI and if the first command called is a workload command. Added start/end text for running the workload command within first-run. Minor cleanup.

# Conflicts: # src/Cli/dotnet/commands/dotnet-workload/install/WorkloadResolverFactory.cs # src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs

…ommand. Removed unused usings.

MiYanni · 2023-07-29T02:44:50Z

src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs

@@ -15,6 +15,7 @@

 namespace Microsoft.DotNet.Cli.NuGetPackageDownloader
 {
+    // TODO: Never name a class the same name as the namespace. Update either for easier type resolution.


Added this TODO for (foreshadowing) future cleanup purposes.

MiYanni · 2023-07-29T02:45:22Z

src/Cli/dotnet/commands/dotnet-workload/WorkloadCommandBase.cs

+        private static bool ShouldVerifySignatures(ParseResult parseResult) =>
+            ShouldVerifySignatures(parseResult.GetValue(WorkloadInstallCommandParser.SkipSignCheckOption));
+
+        public static bool ShouldVerifySignatures(bool skipSignCheck = false)


Method needed to be public for workload integrity check.

src/Cli/dotnet/commands/dotnet-workload/WorkloadCommandBase.cs

dsplaisted

Is it currently not possible to cancel the integrity check at all? Or if you try to cancel, does it cancel the whole command instead of just the integrity check (which seems OK to me)?

src/Cli/dotnet/Program.cs

dsplaisted · 2023-07-31T16:01:36Z

src/Cli/dotnet/commands/dotnet-workload/WorkloadIntegrityChecker.cs

+            if (installedWorkloads.Any())
+            {
+                reporter.WriteLine(LocalizableStrings.WorkloadIntegrityCheck);
+                CliTransaction.RunNew(context => installer.InstallWorkloads(installedWorkloads, sdkFeatureBand, context));


I think it may be better to first check if there are any missing workload packs, and only if that's the case try to install them. The difference would be that we could print a message saying that workload packs are missing and that they are being installed. This may be important for msi-based installs, as when it tries to install the workload packs it should pop up an elevation prompt, and a clearer message might help people understand what it's for.

I tried to keep the changes simplistic since I wasn't sure if we'd be changing anything else related to workload installs. So, install workloads currently does this kind of check:

The check is within the transaction context. Not sure if that is necessary?? Like, we could just pre-filter the packs before creating the context. Looks like it is only done for message purposes. I didn't want to change this stuff too much as testing the first-run scenario is really time consuming.

If we want to change the messaging, should there really be different messaging between this new process and that of workload update/workload install? Testing the CLI command is fairly straight-forward compared to what this PR is fixing.

I think maybe doing a separate endeavor (different PR) to clean up the workload logging would be appropriate. I've already pointed out in one of the workload meeting chats about how when workloads are already up-to-date, we output 3 lines per pack, which seems a bit noisy.

I'll make a separate issue for this work.

Created issue to handle this work: #34417

Reducing the spew for updates would be great, but for the first run check, I think it's best if we print out nothing if everything is up to date, not even a "Checking the state of installed workloads..." message. Then if there are updates needed, we could print something like "Installing updated workload packs".

To do so, we could add a HasMissingWorkloadPacks method to IInstaller, and do a bit of refactoring to the installer implementations so that they share code between the new method and the InstallWorkloads method.

I believe that effort would go into the #34417 issue since it requires extracting out the logic for handling workload installation/messaging as to not be within the command logic directly. Right now, everything is tightly coupled to the concept of running as a CLI command.

That kind of 'tailored' experience could be unit tested when it is refactored, but I'm not planning on refactoring workload installation logic to simply update the text displayed for the initial implementation of this feature. Upon refactoring, we could add some tests within GivenADotnetFirstTimeUseConfigurer.cs and GivenADotnetFirstTimeUseConfigurerWithStateSetup.cs to account for the appropriate messaging.

I'd be ok with a separate cleanup PR since it will likely need to touch code that's unrelated to the first run experience and possibly have some test fallout as well.

MiYanni · 2023-07-31T22:27:30Z

Is it currently not possible to cancel the integrity check at all? Or if you try to cancel, does it cancel the whole command instead of just the integrity check (which seems OK to me)?

Cancelling cancels the entire command (including the workload integrity check). It just immediately stops doing anything. Here, I cancel as soon as it starts trying to install the workload pack.

I'll be creating an issue for supporting cancellation in our use of S.CL in the codebase. For this PR however, as soon as the command is cancelled, it will not reattempt to run the workload integrity check again since the first-run sentinel file exists, which is the desired behavior.

Edit: Created this issue for cancellation support.

… fails, call 'dotnet workload update' to get the proper error message.

…orks around the issue of the log file being locked for the integrity check.

…ment provider for the FlagsCombinationAndAction test.

dsplaisted

I think it's probably feasible to write some automated tests for this. It may not be the simplest thing, but could be better than having to test everything manually.

In the automated tests we could probably test WorkloadIntegrityChecker directly, instead of also worrying about the first use sentinel. It would need to support passing in mocked versions of the various classes it uses. Other workload commands such as WorkloadUpdateCommand and the corresponding GivenDotnetWorkloadUpdate tests show how this can be done.

src/Cli/dotnet/Program.cs

dsplaisted · 2023-08-09T22:43:35Z

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

+    <value>Checking the state of installed workloads...</value>
+  </data>
+  <data name="WorkloadIntegrityCheckError" xml:space="preserve">
+    <value>An issue was encountered when verifying workloads. For more information, run "dotnet workload update".</value>


I'm not sure dotnet workload update is the right guidance here to get the error message. For example, if a download failed during the workload integrity check due to network issues, then when you ran dotnet workload update you might not get the same error.

I'm not sure exactly what we should say here though. Any ideas @baronfel?

Certainly there will be scenarios in which dotnet workload update won't show the exact same issue encountered in first-run. However, the point of the first-run check is to validate that the workloads are in a good state. If it fails, it shouldn't impede a user's command. If they see this message in the first command they run and the first command is, say, dotnet build and their build fails, it gives them another avenue to investigate. dotnet workload update could fix their scenario, potentially. This first-run check is just a ~~band-aid~~ knee-pad to help avoid potential pain for the user.

One idea is to have error codes for the CLI that go to articles about potential reasons for the errors and possible workarounds. Not sure if we have that many CLI-specific problems were we need that kind of error system, though.

Should we remove the "when" or replace with "while". The string reads odd to me

And can you lock the dotnet workload update string? You can inlcude a comment in the resx, like {Locked="dotnet workload update"}

I've removed the when and added the formatted {Locked} comment to the resource.

dsplaisted · 2023-08-09T23:15:31Z

src/Cli/dotnet/commands/dotnet-workload/WorkloadIntegrityChecker.cs

+            if (installedWorkloads.Any())
+            {
+                reporter.WriteLine(LocalizableStrings.WorkloadIntegrityCheck);
+                CliTransaction.RunNew(context => installer.InstallWorkloads(installedWorkloads, sdkFeatureBand, context));


Reducing the spew for updates would be great, but for the first run check, I think it's best if we print out nothing if everything is up to date, not even a "Checking the state of installed workloads..." message. Then if there are updates needed, we could print something like "Installing updated workload packs".

To do so, we could add a HasMissingWorkloadPacks method to IInstaller, and do a bit of refactoring to the installer implementations so that they share code between the new method and the InstallWorkloads method.

…running in CI or not. Then, if the environment variable is provided, it is honored prior to falling back to the default.

MiYanni · 2023-08-11T00:42:22Z

I think it's probably feasible to write some automated tests for this. It may not be the simplest thing, but could be better than having to test everything manually.

In the automated tests we could probably test WorkloadIntegrityChecker directly, instead of also worrying about the first use sentinel. It would need to support passing in mocked versions of the various classes it uses. Other workload commands such as WorkloadUpdateCommand and the corresponding GivenDotnetWorkloadUpdate tests show how this can be done.

I talked with Marc and one approach that seems reasonable is to update the test scenarios we provide to our vendor for workloads to include a test for this first-run functionality. When I was manually testing, one of the main scenarios was for MSI-based installs which we don't have unit tests for currently.

I looked into both GivenDotnetWorkloadUpdate.cs and GivenFileBasedWorkloadInstall.cs. These tests cover the constituent parts of what the RunFirstUseCheck in WorkloadIntegrityChecker does. Meaning, I've not created 'new' functionality that needs tested separately. In those tests, they construct a real FileBasedInstaller but provide mocks of certain parts, such as INuGetPackageDownloader and IWorkloadManifestProvider. I've underlined the real components in green (#00FF00) and the mocked components in orange (#FFA500):

The intent of the RunFirstUseCheck was to just make the minimum amount of code to call InstallWorkloads which defaults as many of those components that make sense. So, all of those parts should have tests already. The tests above the code in the screenshot just call InstallWorkloads with a canned set of values.

The other call that is made in RunFirstUseCheck is GetInstalledWorkloads which also has a series of tests for it across a few different parts, such as WorkloadsInfoProviderTests and the dotnet workload list tests.

src/Cli/dotnet/Program.cs

joeloff

LGTM, question though, do we need to retarget against release/8.0.1xx?

…rly indicate the locked text for localization.

…ntVariableNames as static strings.

MiYanni · 2023-08-16T00:37:37Z

LGTM, question though, do we need to retarget against release/8.0.1xx?

Talked to Marc and he said to also put it into 8.0.1xx. We're trying to see if we can get that backport action working.

MiYanni · 2023-08-16T21:09:05Z

/backport to release/8.0.1xx

github-actions · 2023-08-16T21:09:19Z

Started backporting to release/8.0.1xx: https://github.com/dotnet/sdk/actions/runs/5883877917

github-actions · 2023-08-16T21:09:43Z

@MiYanni backporting to release/8.0.1xx failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Added WorkloadIntegrityChecker to use as a sidecar for running the InstallWorkloads method during first run. This may be temporary. Cleaned up unused usings in various files. Cleaned up various stray spaces and extra carriage returns. Other various minor edits.
Using index info to reconstruct a base tree...
M	src/Cli/Microsoft.DotNet.Configurer/DotnetFirstTimeUseConfigurer.cs
M	src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
M	src/Cli/dotnet/Program.cs
M	src/Cli/dotnet/commands/InstallingWorkloadCommand.cs
M	src/Cli/dotnet/commands/dotnet-workload/WorkloadCommandBase.cs
M	src/Cli/dotnet/commands/dotnet-workload/clean/WorkloadCleanCommand.cs
M	src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs
M	src/Cli/dotnet/commands/dotnet-workload/install/IInstaller.cs
M	src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs
M	src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallerFactory.cs
M	src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs
M	src/Cli/dotnet/commands/dotnet-workload/install/WorkloadResolverFactory.cs
M	src/Cli/dotnet/commands/dotnet-workload/repair/WorkloadRepairCommand.cs
M	src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchCommand.cs
M	src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs
M	src/Tests/Microsoft.DotNet.Configurer.UnitTests/GivenADotnetFirstTimeUseConfigurerWIthStateSetup.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Tests/Microsoft.DotNet.Configurer.UnitTests/GivenADotnetFirstTimeUseConfigurerWIthStateSetup.cs
CONFLICT (content): Merge conflict in src/Tests/Microsoft.DotNet.Configurer.UnitTests/GivenADotnetFirstTimeUseConfigurerWIthStateSetup.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/search/WorkloadSearchCommand.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/repair/WorkloadRepairCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-workload/repair/WorkloadRepairCommand.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/WorkloadResolverFactory.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-workload/install/WorkloadResolverFactory.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallerFactory.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/IInstaller.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/clean/WorkloadCleanCommand.cs
Auto-merging src/Cli/dotnet/commands/dotnet-workload/WorkloadCommandBase.cs
Auto-merging src/Cli/dotnet/commands/InstallingWorkloadCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/InstallingWorkloadCommand.cs
Auto-merging src/Cli/dotnet/Program.cs
Auto-merging src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs
Auto-merging src/Cli/Microsoft.DotNet.Configurer/DotnetFirstTimeUseConfigurer.cs
CONFLICT (content): Merge conflict in src/Cli/Microsoft.DotNet.Configurer/DotnetFirstTimeUseConfigurer.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Added WorkloadIntegrityChecker to use as a sidecar for running the InstallWorkloads method during first run. This may be temporary. Cleaned up unused usings in various files. Cleaned up various stray spaces and extra carriage returns. Other various minor edits.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-08-16T21:09:44Z

@MiYanni an error occurred while backporting to release/8.0.1xx, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

…#34694)

MiYanni added 10 commits July 13, 2023 18:24

Added WorkloadIntegrityChecker to use as a sidecar for running the In…

ca96fae

…stallWorkloads method during first run. This may be temporary. Cleaned up unused usings in various files. Cleaned up various stray spaces and extra carriage returns. Other various minor edits.

Merge branch 'main' into FirstRunWorkloadExperience

4396ec8

Removed using cleanup only changes. Just going to put this in a big c…

fea7f40

…leanup PR.

Testing using minimal for workload install output.

a46baba

Merge branch 'main' into FirstRunWorkloadExperience

8e67377

Added SkipWorkloadIntegrityCheck via an environment variable so that …

dd3eeff

…CI can set it to true to no longer run the workload update.

Fix broken tests from configuration change.

2c23616

Added 2 failsafe states where the integrity check will be skipped if …

ae65a33

…running in CI and if the first command called is a workload command. Added start/end text for running the workload command within first-run. Minor cleanup.

Merge branch 'main' into FirstRunWorkloadExperience

cec578a

# Conflicts: # src/Cli/dotnet/commands/dotnet-workload/install/WorkloadResolverFactory.cs # src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs

Added try/catch so the integrity check doesn't hinder executing the c…

4a3aadd

…ommand. Removed unused usings.

ghost added Area-Workloads untriaged Request triage from a team member labels Jul 29, 2023

MiYanni commented Jul 29, 2023

View reviewed changes

src/Cli/dotnet/commands/dotnet-workload/WorkloadCommandBase.cs Show resolved Hide resolved

MiYanni requested a review from a team July 29, 2023 02:46

dsplaisted reviewed Jul 31, 2023

View reviewed changes

MiYanni added 3 commits August 1, 2023 15:26

Added a message that tells the user when the workload integrity check…

de216a6

… fails, call 'dotnet workload update' to get the proper error message.

Added milliseconds to the workload log name for MSI-based installs. W…

8884a8d

…orks around the issue of the log file being locked for the integrity check.

Forgot to add the DOTNET_SKIP_WORKLOAD_INTEGRITY_CHECK to the environ…

bd0ea1a

…ment provider for the FlagsCombinationAndAction test.

MiYanni mentioned this pull request Aug 3, 2023

dotnet CLI: Consolidate dotnet workload command messages #34417

Open

4 tasks

marcpopMSFT requested a review from dsplaisted August 8, 2023 20:16

dsplaisted reviewed Aug 9, 2023

View reviewed changes

Set the default value for the workload integrity check skip based on …

8208350

…running in CI or not. Then, if the environment variable is provided, it is honored prior to falling back to the default.

joeloff reviewed Aug 11, 2023

View reviewed changes

src/Cli/dotnet/Program.cs Outdated Show resolved Hide resolved

joeloff approved these changes Aug 15, 2023

View reviewed changes

MiYanni added 2 commits August 15, 2023 17:06

Updated WorkloadIntegrityCheckError to have clearer wording and prope…

38ec517

…rly indicate the locked text for localization.

Added environment variables used via EnvironmentProvider to Environme…

6fc3eaa

…ntVariableNames as static strings.

MiYanni merged commit ccb0069 into dotnet:main Aug 16, 2023

MiYanni mentioned this pull request Aug 16, 2023

Backport of 'First-run workload experience #34322' to release/8.0.1xx #34694

Merged

MiYanni added a commit that referenced this pull request Aug 18, 2023

Backport of 'First-run workload experience #34322' to release/8.0.1xx (…

2a3188f

…#34694)

Forgind mentioned this pull request Jan 30, 2024

'dotnet workload list' CLI command should not download and install workloads without a user permission #38026

Closed

MiYanni mentioned this pull request Feb 12, 2024

[WebToolsE2E][Aspire] After updating .NET SDK to newer band, the Aspire workload does not appear to be installed anymore dotnet/aspire#2172

Closed

MiYanni mentioned this pull request Mar 4, 2025

An issue was encountered verifying workloads. For more information, run "dotnet workload update". #46187

Closed

First-run workload experience #34322

First-run workload experience #34322

Uh oh!

Conversation

MiYanni commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Aspects

Testing Notes

Example Output

Other

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiYanni commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiYanni commented Aug 11, 2023

Uh oh!

Uh oh!

joeloff left a comment

Choose a reason for hiding this comment

Uh oh!

MiYanni commented Aug 16, 2023

Uh oh!

MiYanni commented Aug 16, 2023

Uh oh!

github-actions bot commented Aug 16, 2023

Uh oh!

github-actions bot commented Aug 16, 2023

Uh oh!

github-actions bot commented Aug 16, 2023

Uh oh!

Uh oh!

MiYanni commented Jul 29, 2023 •

edited

Loading

MiYanni commented Jul 31, 2023 •

edited

Loading