Address / remove TODOs

Author: dsplaisted

src/Cli/dotnet/ToolPackage/LocalToolsResolverCache.cs

@@ -33,8 +33,6 @@ public void Save(
     {
         EnsureFileStorageExists();
 
-        //  TODO: Save resolved package info here?
-
         foreach (var distinctPackageIdAndRestoredCommandMap in restoredCommandMap.GroupBy(x => x.Key.PackageId))
         {
             PackageId distinctPackageId = distinctPackageIdAndRestoredCommandMap.Key;
@@ -189,6 +187,5 @@ private class CacheRow
         public string Name { get; set; }
         public string Runner { get; set; }
         public string PathToExecutable { get; set; }
-        //  TODO: Need resolved package info here
     }
 }

src/Cli/dotnet/ToolPackage/RestoredCommandIdentifier.cs

@@ -19,9 +19,6 @@ internal class RestoredCommandIdentifier(
     string runtimeIdentifier,
     ToolCommandName commandName) : IEquatable<RestoredCommandIdentifier>
 {
-
-    // TODO: What is this class and how is it different from CacheRow?  Does it need to have the resolved package information?
-
     public PackageId PackageId { get; } = packageId;
     public NuGetVersion Version { get; } = version ?? throw new ArgumentException(nameof(version));
     public NuGetFramework TargetFramework { get; } = targetFramework ?? throw new ArgumentException(nameof(targetFramework));

src/Cli/dotnet/ToolPackage/ToolConfigurationDeserializer.cs

@@ -57,7 +57,6 @@ public static ToolConfiguration Deserialize(string pathToXml, IFileSystem fileSy
             throw new ToolConfigurationException(CliStrings.ToolSettingsMoreThanOneCommand);
         }
 
-        //  TODO: Should be an error if runner is empty and there aren't any RID-specific packages
         var runner = dotNetCliTool.Commands[0].Runner;
         if (!string.IsNullOrEmpty(runner) && runner != "dotnet" && runner != "executable")
         {
@@ -71,6 +70,16 @@ public static ToolConfiguration Deserialize(string pathToXml, IFileSystem fileSy
         var ridSpecificPackages = dotNetCliTool.RuntimeIdentifierPackages?.ToDictionary(p => p.RuntimeIdentifier, p => new PackageIdentity(p.Id, new NuGet.Versioning.NuGetVersion(p.Version)))
             .AsReadOnly();
 
+        //  Also error out if there are no RID-specific packages and the runner is empty
+        if (string.IsNullOrEmpty(runner) && !ridSpecificPackages.Any())
+        {
+            throw new ToolConfigurationException(
+                string.Format(
+                    CliStrings.ToolSettingsUnsupportedRunner,
+                    dotNetCliTool.Commands[0].Name,
+                    dotNetCliTool.Commands[0].Runner));
+        }
+
         return new ToolConfiguration(
             dotNetCliTool.Commands[0].Name,
             dotNetCliTool.Commands[0].EntryPoint,

src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs

@@ -177,7 +177,8 @@ protected override void CreateAssetFile(
         managedCriteria.Add(standardCriteria);
 
         //  Create asset file
-        //  TODO: What about DotnetToolRidPackage type?
+        //  Note that we know that the package type for the lock file is DotnetTool because we just set it to that.
+        //  This if statement is still here because this mirrors code in NuGet for restore so maybe it will be easier to keep in sync if need be
         if (lockFileLib.PackageType.Contains(PackageType.DotnetTool))
         {
             AddToolsAssets(conventions, lockFileLib, collection, managedCriteria);

src/Cli/dotnet/ToolPackage/ToolPackageDownloaderBase.cs

@@ -28,12 +28,6 @@ internal abstract class ToolPackageDownloaderBase : IToolPackageDownloader
 {
     private readonly IToolPackageStore _toolPackageStore;
 
-    // The directory that the tool package is returned 
-    protected DirectoryPath _toolReturnPackageDirectory;
-
-    // The directory that the tool asset file is returned
-    protected DirectoryPath _toolReturnJsonParentDirectory;
-
     protected readonly IFileSystem _fileSystem;
 
     // The directory that global tools first downloaded
@@ -68,7 +62,7 @@ protected ToolPackageDownloaderBase(
         _localToolDownloadDir = new DirectoryPath(SettingsUtility.GetGlobalPackagesFolder(settings));
         _currentWorkingDirectory = currentWorkingDirectory;
 
-        _localToolAssetDir = new DirectoryPath(_fileSystem.Directory.CreateTemporarySubdirectory()); // PathUtilities.CreateTempSubdirectory());
+        _localToolAssetDir = new DirectoryPath(_fileSystem.Directory.CreateTemporarySubdirectory());
         _runtimeJsonPath = runtimeJsonPathForTests ?? Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "RuntimeIdentifierGraph.json");
     }
 

src/Cli/dotnet/ToolPackage/ToolPackageStoreAndQuery.cs

@@ -89,10 +89,6 @@ public IEnumerable<IToolPackage> EnumeratePackageVersions(PackageId packageId)
 
         foreach (var subdirectory in _fileSystem.Directory.EnumerateDirectories(packageRootDirectory.Value))
         {
-            // TODO: How to handle redirected RID-specific packages?
-            //  Probably we can restore both priamry and RID-specific packages under the same versioned folder,
-            //  for example .dotnet\tools\.store\microsoft.dotnet-interactive\1.0.415202
-            //  Probably the RID-specific package assets file can be stored under a different name, such as project.assets.<RID>.json
             yield return new ToolPackageInstance(id: packageId,
                 version: NuGetVersion.Parse(Path.GetFileName(subdirectory)),
                 packageDirectory: new DirectoryPath(subdirectory),

src/Tasks/Microsoft.NET.Build.Tasks/GenerateToolsSettingsFile.cs

@@ -9,10 +9,6 @@ namespace Microsoft.NET.Build.Tasks
 {
     public class GenerateToolsSettingsFile : TaskBase
     {
-        // bump whenever the format changes such that it will break old consumers
-        //  TODO: Make this version 2 when tool has RID-specific packages?
-        //private static readonly int _formatVersion = 1;
-
         [Required]
         public string EntryPointRelativePath { get; set; }
 
@@ -41,6 +37,7 @@ protected override void ExecuteCore()
         internal static XDocument GenerateDocument(string entryPointRelativePath, string commandName, string commandRunner, string runtimeIdentifier,
             string toolPackageId, string toolPackageVersion, ITaskItem[] toolPackageRuntimeIdentifiers)
         {
+            // Format version should bump whenever the format changes such that it will break old consumers
             int formatVersion = 1;
 
             if (string.IsNullOrEmpty(commandRunner))

src/Tasks/Microsoft.NET.Build.Tasks/RemoveTargetFromList.cs

@@ -21,6 +21,7 @@ public class RemoveTargetFromList : TaskBase
         [Required]
         public string TargetToRemove { get; set; }
 
+        //  Output needs to be an array so MSBuild won't escape semicolonns
         [Output]
         public string[] UpdatedTargetList { get; private set; }
 

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets

@@ -100,8 +100,6 @@ Copyright (c) .NET Foundation. All rights reserved.
       <PackageId>$(PackageId).$(RuntimeIdentifier)</PackageId>
     </PropertyGroup>
 
-    <!-- TODO: Should we read Version from the corresponding ToolPackageRuntimeIdentifier item, if it's set? -->
-
   </Target>
 
   <Target Name="PackToolImplementation" Condition="'$(_ToolPackageShouldIncludeImplementation)' == 'true'">