-
Notifications
You must be signed in to change notification settings - Fork 1.1k
First-run workload experience #34322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
ca96fae
4396ec8
fea7f40
a46baba
8e67377
dd3eeff
2c23616
ae65a33
cec578a
4a3aadd
de216a6
8884a8d
bd0ea1a
8208350
38ec517
6fc3eaa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,4 +153,11 @@ | |
<data name="WorkloadInfoDescription" xml:space="preserve"> | ||
<value>Display information about installed workloads.</value> | ||
</data> | ||
</root> | ||
<data name="WorkloadIntegrityCheck" xml:space="preserve"> | ||
<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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I'm not sure exactly what we should say here though. Any ideas @baronfel? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Certainly there will be scenarios in which 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove the "when" or replace with "while". The string reads odd to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And can you lock the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the |
||
<comment>Do not translate "dotnet workload update" as it is a CLI command.</comment> | ||
</data> | ||
</root> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,9 +101,9 @@ public WorkloadCommandBase( | |
|
||
TempDirectoryPath = !string.IsNullOrWhiteSpace(tempDirPath) | ||
? tempDirPath | ||
: !string.IsNullOrWhiteSpace(parseResult.GetValue(WorkloadInstallCommandParser.TempDirOption)) | ||
? parseResult.GetValue(WorkloadInstallCommandParser.TempDirOption) | ||
: PathUtilities.CreateTempSubdirectory(); | ||
: (!string.IsNullOrWhiteSpace(parseResult.GetValue(WorkloadInstallCommandParser.TempDirOption)) | ||
? parseResult.GetValue(WorkloadInstallCommandParser.TempDirOption) | ||
: PathUtilities.CreateTempSubdirectory()); | ||
|
||
TempPackagesDirectory = new DirectoryPath(Path.Combine(TempDirectoryPath, "dotnet-sdk-advertising-temp")); | ||
|
||
|
@@ -123,17 +123,18 @@ public WorkloadCommandBase( | |
/// <param name="parseResult">The results of parsing the command line.</param> | ||
/// <returns><see langword="true"/> if signatures of packages and installers should be verified.</returns> | ||
/// <exception cref="GracefulException" /> | ||
private static bool ShouldVerifySignatures(ParseResult parseResult) | ||
private static bool ShouldVerifySignatures(ParseResult parseResult) => | ||
ShouldVerifySignatures(parseResult.GetValue(WorkloadInstallCommandParser.SkipSignCheckOption)); | ||
|
||
public static bool ShouldVerifySignatures(bool skipSignCheck = false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method needed to be public for workload integrity check. |
||
{ | ||
if (!SignCheck.IsDotNetSigned()) | ||
{ | ||
// Can't enforce anything if we already allowed an unsigned dotnet to be installed. | ||
return false; | ||
} | ||
|
||
bool skipSignCheck = parseResult.GetValue(WorkloadInstallCommandParser.SkipSignCheckOption); | ||
MiYanni marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool policyEnabled = SignCheck.IsWorkloadSignVerificationPolicySet(); | ||
|
||
if (skipSignCheck && policyEnabled) | ||
{ | ||
// Can't override the global policy by using the skip option. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using Microsoft.DotNet.Cli; | ||
using Microsoft.DotNet.Cli.NuGetPackageDownloader; | ||
using Microsoft.DotNet.Cli.Utils; | ||
using Microsoft.DotNet.Workloads.Workload.Install; | ||
using Microsoft.Extensions.EnvironmentAbstractions; | ||
using Microsoft.NET.Sdk.WorkloadManifestReader; | ||
using NuGet.Common; | ||
|
||
namespace Microsoft.DotNet.Workloads.Workload | ||
{ | ||
internal static class WorkloadIntegrityChecker | ||
{ | ||
public static void RunFirstUseCheck(IReporter reporter) | ||
{ | ||
var creationParameters = new WorkloadResolverFactory.CreationParameters() | ||
{ | ||
CheckIfFeatureBandManifestExists = true, | ||
UseInstalledSdkVersionForResolver = true | ||
}; | ||
|
||
var creationResult = WorkloadResolverFactory.Create(creationParameters); | ||
var sdkFeatureBand = new SdkFeatureBand(creationResult.SdkVersion); | ||
var verifySignatures = WorkloadCommandBase.ShouldVerifySignatures(); | ||
var tempPackagesDirectory = new DirectoryPath(PathUtilities.CreateTempSubdirectory()); | ||
var packageDownloader = new NuGetPackageDownloader( | ||
tempPackagesDirectory, | ||
verboseLogger: new NullLogger(), | ||
verifySignatures: verifySignatures); | ||
|
||
var installer = WorkloadInstallerFactory.GetWorkloadInstaller( | ||
reporter, | ||
sdkFeatureBand, | ||
creationResult.WorkloadResolver, | ||
VerbosityOptions.normal, | ||
creationResult.UserProfileDir, | ||
verifySignatures, | ||
packageDownloader, | ||
creationResult.DotnetPath); | ||
var repository = installer.GetWorkloadInstallationRecordRepository(); | ||
var installedWorkloads = repository.GetInstalledWorkloads(sdkFeatureBand); | ||
|
||
if (installedWorkloads.Any()) | ||
{ | ||
reporter.WriteLine(LocalizableStrings.WorkloadIntegrityCheck); | ||
CliTransaction.RunNew(context => installer.InstallWorkloads(installedWorkloads, sdkFeatureBand, context)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created issue to handle this work: #34417 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
reporter.WriteLine("----------------"); | ||
} | ||
} | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this TODO for (foreshadowing) future cleanup purposes.