Fix issue garbage collecting cross-feature band workload set, add errors for combinations with skip manifest update

Author: dsplaisted

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

@@ -36,6 +36,7 @@ internal abstract class InstallingWorkloadCommand : WorkloadCommandBase
         protected readonly ReleaseVersion _targetSdkVersion;
         protected readonly string _fromRollbackDefinition;
         protected string _workloadSetVersionFromCommandLine;
+        protected string _globalJsonPath;
         protected string _workloadSetVersionFromGlobalJson;
         protected readonly PackageSourceLocation _packageSourceLocation;
         protected readonly IWorkloadResolverFactory _workloadResolverFactory;
@@ -99,12 +100,12 @@ public InstallingWorkloadCommand(
             _workloadInstallerFromConstructor = workloadInstaller;
             _workloadManifestUpdaterFromConstructor = workloadManifestUpdater;
 
-            var globaljsonPath = SdkDirectoryWorkloadManifestProvider.GetGlobalJsonPath(Environment.CurrentDirectory);
-            _workloadSetVersionFromGlobalJson = SdkDirectoryWorkloadManifestProvider.GlobalJsonReader.GetWorkloadVersionFromGlobalJson(globaljsonPath);
+            _globalJsonPath = SdkDirectoryWorkloadManifestProvider.GetGlobalJsonPath(Environment.CurrentDirectory);
+            _workloadSetVersionFromGlobalJson = SdkDirectoryWorkloadManifestProvider.GlobalJsonReader.GetWorkloadVersionFromGlobalJson(_globalJsonPath);
 
             if (SpecifiedWorkloadSetVersionInGlobalJson && (SpecifiedWorkloadSetVersionOnCommandLine || UseRollback))
             {
-                throw new GracefulException(string.Format(Strings.CannotSpecifyVersionOnCommandLineAndInGlobalJson, globaljsonPath), isUserError: true);
+                throw new GracefulException(string.Format(Strings.CannotSpecifyVersionOnCommandLineAndInGlobalJson, _globalJsonPath), isUserError: true);
             }
 
             if (SpecifiedWorkloadSetVersionOnCommandLine && UseRollback)

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

@@ -322,6 +322,13 @@
   <data name="CannotCombineSkipManifestAndRollback" xml:space="preserve">
     <value>Cannot use the {0} and {1} options together. If installing from a rollback file, remove {0}. Otherwise, remove {1}</value>
   </data>
+  <data name="CannotCombineSkipManifestAndVersion" xml:space="preserve">
+    <value>Cannot use the {0} and {1} options together.  Remove one of the options.</value>
+  </data>
+  <data name="CannotUseSkipManifestWithGlobalJsonWorkloadVersion" xml:space="preserve">
+    <value>Cannot use the {0} option when workload version is specified in global.json.  Remove the {0} option, or remove the 'workloadVersion' element from {1}.</value>
+    <comment>"workloadVersion" is a literal value and should not be translated.</comment>
+  </data>
   <data name="SkipSignCheckOptionDescription" xml:space="preserve">
     <value>Skip signature verification of workload packages and installers.</value>
   </data>

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

@@ -77,6 +77,7 @@ void GarbageCollectWorkloadSets()
             foreach (var set in installedWorkloadSets.Keys)
             {
                 WorkloadSetsToKeep.Add(set);
+                _verboseReporter.WriteLine($"GC: Keeping workload set version {set} because workload set GC isn't implemented yet.");
             }
 
             var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetDir), "default.json");

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

@@ -43,8 +43,6 @@ public WorkloadInstallCommand(
 
             _workloadManifestUpdater = _workloadManifestUpdaterFromConstructor ?? new WorkloadManifestUpdater(Reporter, _workloadResolver, PackageDownloader, _userProfileDir,
                 _workloadInstaller.GetWorkloadInstallationRecordRepository(), _workloadInstaller, _packageSourceLocation, displayManifestUpdates: Verbosity.IsDetailedOrDiagnostic());
-
-            
         }
 
         private void ValidateWorkloadIdsInput()
@@ -108,32 +106,28 @@ public override int Execute()
                     WorkloadInstallCommandParser.SkipManifestUpdateOption.Name, InstallingWorkloadCommandParser.FromRollbackFileOption.Name,
                     WorkloadInstallCommandParser.SkipManifestUpdateOption.Name, InstallingWorkloadCommandParser.FromRollbackFileOption.Name), isUserError: true);
             }
+            else if (_skipManifestUpdate && SpecifiedWorkloadSetVersionOnCommandLine)
+            {
+                throw new GracefulException(string.Format(LocalizableStrings.CannotCombineSkipManifestAndVersion,
+                    WorkloadInstallCommandParser.SkipManifestUpdateOption.Name, InstallingWorkloadCommandParser.VersionOption.Name), isUserError: true);
+            }
+            else if (_skipManifestUpdate && SpecifiedWorkloadSetVersionInGlobalJson)
+            {
+                throw new GracefulException(string.Format(LocalizableStrings.CannotUseSkipManifestWithGlobalJsonWorkloadVersion,
+                    WorkloadInstallCommandParser.SkipManifestUpdateOption.Name, _globalJsonPath), isUserError: true);
+            }
             else
             {
-                bool shouldUpdateWorkloads = !_skipManifestUpdate;
-
                 try
                 {
                     //  Normally we want to validate that the workload IDs specified were valid.  However, if there is a global.json file with a workload
-                    //  set version specified, and we might update the workload version, then we don't do that check here, because we might not have the right
+                    //  set version specified, and we might install that workload version, then we don't do that check here, because we might not have the right
                     //  workload set installed yet, and trying to list the available workloads would throw an error
-                    if (!shouldUpdateWorkloads || string.IsNullOrEmpty(_workloadSetVersionFromGlobalJson))
+                    if (_skipManifestUpdate || string.IsNullOrEmpty(_workloadSetVersionFromGlobalJson))
                     {
                         ValidateWorkloadIdsInput();
                     }
 
-                    if (string.IsNullOrWhiteSpace(_workloadSetVersionFromCommandLine) && string.IsNullOrWhiteSpace(_workloadSetVersionFromGlobalJson))
-                    {
-                        var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json");
-                        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;
-                        }
-                    }
-
                     Reporter.WriteLine();
 
                     DirectoryPath? offlineCache = string.IsNullOrWhiteSpace(_fromCacheOption) ? null : new DirectoryPath(_fromCacheOption);