Apply code review feedback

Author: dsplaisted

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

@@ -46,7 +46,7 @@ internal abstract class InstallingWorkloadCommand : WorkloadCommandBase
         protected IWorkloadManifestUpdater _workloadManifestUpdater;
 
         protected bool UseRollback => !string.IsNullOrWhiteSpace(_fromRollbackDefinition);
-        protected bool SpecifiedWorkloadSetVersionOnCommandLine => !string.IsNullOrEmpty(_workloadSetVersionFromCommandLine);
+        protected bool SpecifiedWorkloadSetVersionOnCommandLine => !string.IsNullOrWhiteSpace(_workloadSetVersionFromCommandLine);
         protected bool SpecifiedWorkloadSetVersionInGlobalJson => !string.IsNullOrWhiteSpace(_workloadSetVersionFromGlobalJson);
 
         public InstallingWorkloadCommand(
@@ -115,7 +115,6 @@ public InstallingWorkloadCommand(
             }
 
             //  At this point, at most one of SpecifiedWorkloadSetVersionOnCommandLine, UseRollback, and SpecifiedWorkloadSetVersionInGlobalJson is true
-
         }
 
         protected static Dictionary<string, string> GetInstallStateContents(IEnumerable<ManifestVersionUpdate> manifestVersionUpdates) =>
@@ -131,8 +130,7 @@ InstallStateContents GetCurrentInstallState()
         static InstallStateContents GetCurrentInstallState(SdkFeatureBand sdkFeatureBand, string dotnetDir)
         {
             string path = Path.Combine(WorkloadInstallType.GetInstallStateFolder(sdkFeatureBand, dotnetDir), "default.json");
-            var installStateContents = File.Exists(path) ? InstallStateContents.FromString(File.ReadAllText(path)) : new InstallStateContents();
-            return installStateContents;
+            return InstallStateContents.FromPath(path);
         }
 
         public static bool ShouldUseWorkloadSetMode(SdkFeatureBand sdkFeatureBand, string dotnetDir)
@@ -142,18 +140,18 @@ public static bool ShouldUseWorkloadSetMode(SdkFeatureBand sdkFeatureBand, strin
 
         protected void UpdateWorkloadManifests(ITransactionContext context, DirectoryPath? offlineCache)
         {
-            var updateUsingWorkloadSets = ShouldUseWorkloadSetMode(_sdkFeatureBand, _dotnetPath);
-            if (UseRollback && updateUsingWorkloadSets)
+            var updateToLatestWorkloadSet = ShouldUseWorkloadSetMode(_sdkFeatureBand, _dotnetPath);
+            if (UseRollback && updateToLatestWorkloadSet)
             {
                 // Rollback files are only for loose manifests. Update the mode to be loose manifests.
                 Reporter.WriteLine(Update.LocalizableStrings.UpdateFromRollbackSwitchesModeToLooseManifests);
                 _workloadInstaller.UpdateInstallMode(_sdkFeatureBand, false);
-                updateUsingWorkloadSets = false;
+                updateToLatestWorkloadSet = false;
             }
 
             if (SpecifiedWorkloadSetVersionOnCommandLine)
             {
-                updateUsingWorkloadSets = false;
+                updateToLatestWorkloadSet = false;
 
                 //  If a workload set version is specified, then switch to workload set update mode
                 //  Check to make sure the value needs to be changed, as updating triggers a UAC prompt
@@ -167,14 +165,14 @@ protected void UpdateWorkloadManifests(ITransactionContext context, DirectoryPat
             string resolvedWorkloadSetVersion = _workloadSetVersionFromGlobalJson ??_workloadSetVersionFromCommandLine;
             if (string.IsNullOrWhiteSpace(resolvedWorkloadSetVersion) && !UseRollback)
             {
-                _workloadManifestUpdater.UpdateAdvertisingManifestsAsync(_includePreviews, updateUsingWorkloadSets, offlineCache).Wait();
-                if (updateUsingWorkloadSets)
+                _workloadManifestUpdater.UpdateAdvertisingManifestsAsync(_includePreviews, updateToLatestWorkloadSet, offlineCache).Wait();
+                if (updateToLatestWorkloadSet)
                 {
                     resolvedWorkloadSetVersion = _workloadManifestUpdater.GetAdvertisedWorkloadSetVersion();
                 }
             }
 
-            if (updateUsingWorkloadSets && resolvedWorkloadSetVersion == null)
+            if (updateToLatestWorkloadSet && resolvedWorkloadSetVersion == null)
             {
                 Reporter.WriteLine(Update.LocalizableStrings.NoWorkloadUpdateFound);
                 return;

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

@@ -267,13 +267,13 @@ public WorkloadSet InstallWorkloadSet(ITransactionContext context, string worklo
             context.Run(
                 action: () =>
                 {
-                    Elevate();
-
                     DetectState state = DetectPackage(msi.ProductCode, out Version installedVersion);
                     InstallAction plannedAction = PlanPackage(msi, state, InstallAction.Install, installedVersion);
 
                     if (plannedAction == InstallAction.Install)
                     {
+                        Elevate();
+
                         ExecutePackage(msi, plannedAction, msiPackageId);
 
                         // Update the reference count against the MSI.
@@ -316,7 +316,7 @@ public WorkloadSet InstallWorkloadSet(ITransactionContext context, string worklo
             //  Unwrap AggregateException caused by switch from async to sync
             catch (Exception ex) when (ex is NuGetPackageNotFoundException || ex.InnerException is NuGetPackageNotFoundException)
             {
-                throw new GracefulException(string.Format(Update.LocalizableStrings.WorkloadVersionRequestedNotFound, workloadSetVersion), ex);
+                throw new GracefulException(string.Format(Update.LocalizableStrings.WorkloadVersionRequestedNotFound, workloadSetVersion), ex is NuGetPackageNotFoundException ? ex : ex.InnerException);
             }
             VerifyPackage(msi);
 

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

@@ -125,15 +125,12 @@ public override int Execute()
                     if (string.IsNullOrWhiteSpace(_workloadSetVersionFromCommandLine) && string.IsNullOrWhiteSpace(_workloadSetVersionFromGlobalJson))
                     {
                         var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json");
-                        if (File.Exists(installStateFilePath))
+                        var installStateContents = InstallStateContents.FromPath(installStateFilePath);
+                        //  If install state has pinned workload set or manifest versions, then don't update workloads
+                        if (!string.IsNullOrEmpty(installStateContents.WorkloadVersion) || installStateContents.Manifests != null)
                         {
-                            var installStateContents = InstallStateContents.FromPath(installStateFilePath);
-                            //  If install state has pinned workload set or manifest versions, then don't update workloads
-                            if (!string.IsNullOrEmpty(installStateContents.WorkloadVersion) || installStateContents.Manifests != null)
-                            {
-                                //  TODO: respect shouldUpdateWorkloads, or figure out update / install manifest difference in InstallingWorkloadCommand
-                                shouldUpdateWorkloads = false;
-                            }
+                            //  TODO: respect shouldUpdateWorkloads, or figure out update / install manifest difference in InstallingWorkloadCommand
+                            shouldUpdateWorkloads = false;
                         }
                     }
 

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

@@ -485,8 +485,6 @@ private async Task<bool> NewerManifestPackageExists(ManifestId manifest)
 
         public IEnumerable<ManifestVersionUpdate> CalculateManifestUpdatesForWorkloadSet(WorkloadSet workloadSet)
         {
-            //  TODO: Don't look up previous manifest versions (since we may be in the mode where there's a global.json with a workload set
-            //  that's not installed, and trying to get the current manifests will throw an exception)
             return CalculateManifestRollbacks(workloadSet.ManifestVersions.Select(kvp => (kvp.Key, new ManifestVersionWithBand(kvp.Value.Version, kvp.Value.FeatureBand))));
         }
 

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

@@ -501,7 +501,6 @@ public Dictionary<string, WorkloadSet> GetAvailableWorkloadSets(SdkFeatureBand w
 
             foreach (var manifestRoot in _manifestRoots.Reverse())
             {
-                //  We don't automatically fall back to a previous band
                 var workloadSetsRoot = Path.Combine(manifestRoot, workloadSetFeatureBand.ToString(), WorkloadSetsFolderName);
                 if (Directory.Exists(workloadSetsRoot))
                 {

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

@@ -169,7 +169,9 @@ public static string WorkloadSetVersionToWorkloadSetPackageVersion(string setVer
 
             if (preReleaseOrBuild != null)
             {
-                packageVersion += '-' + preReleaseOrBuild;
+                //  Figure out if we split on a '-' or '+'
+                char separator = setVersion[sections[0].Length];
+                packageVersion += separator + preReleaseOrBuild;
             }
 
             return packageVersion;