Multi-RID tool packages usability tweaks #49288

baronfel · 2025-06-07T01:00:20Z

This PR:

Migrates from an Item-based usage mode (<ToolPackageRuntimeIdentifier Include="linux-x64" Version="A.B.C" />) to a property-based usage mode (<ToolPackageRuntimeIdentifier>linux-x64</ToolPackageRuntimeIdentifier>).
- This implies the removal of the Version from the RID-specific tool package configuration entries (the versions must always be aligned with the 'parent' RID-less package)
Simplifies some of the packaging of the Tool packages
Makes it so packaging the 'parent' package via dotnet pack triggers the packaging of the 'inner' per-RID packages as well
- Caveat - this won't ever work if we're in AOT mode, so we don't do that for PublishAOT packages.

Copilot

Pull Request Overview

This PR adjusts the packaging mechanism for RID-specific tool packages by migrating from an item‐based to a property‐based usage mode, streamlining package creation for both parent and RID‐specific packages.

Migrates from using an Item-based RID specification with a Version attribute to a property-based configuration without a version.
Updates various build targets to correctly trigger and define inner and outer builds, including adjustments to packaging tasks and property conditions.
Removes legacy code (such as the version handling in tool package resolution and associated unit tests) to simplify the overall tool package design.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets	Updates PackTool import condition to respect PackAsTool usage.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Common.targets	Adjusts condition to reference the new property ToolPackageRuntimeIdentifiers.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets	Refactors packaging targets to differentiate inner and outer builds and update package paths.
src/Tasks/Microsoft.NET.Build.Tasks/GenerateToolsSettingsFile.cs	Removes version attribute assignment in RID package nodes.
src/Cli/dotnet/ToolPackage/ToolPackageDownloaderBase.cs	Updates package version sourcing for installation and asset file creation.
src/Cli/dotnet/ToolPackage/ToolConfigurationDeserializer.cs	Eliminates version information when creating PackageIdentity from RID-specific data.
src/Cli/dotnet/ToolPackage/ToolConfigurationDeserialization/DotNetCliToolRuntimeIdentifierPackage.cs	Removes the Version attribute in favor of property-based configuration.
(Other files)	Removal of legacy test and resolution files in-line with the new design.

Comments suppressed due to low confidence (3)

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Common.targets:57

Verify that the new property 'ToolPackageRuntimeIdentifiers' is correctly defined and consistently propagated across the targets to avoid mismatches with legacy item names.

<_ToolPackageType Condition="'$(RuntimeIdentifier)' != '' And '$(ToolPackageRuntimeIdentifiers)' != ''">DotnetToolRidPackage</_ToolPackageType>

src/Cli/dotnet/ToolPackage/ToolConfigurationDeserializer.cs:70

Ensure that all consumers of PackageIdentity are updated to handle a null version, as the version is now inherited from the parent package rather than being stored per RID-specific package.

var ridSpecificPackages = dotNetCliTool.RuntimeIdentifierPackages?.ToDictionary(p => p.RuntimeIdentifier, p => new PackageIdentity(p.Id, null))

src/Cli/dotnet/ToolPackage/ToolPackageDownloaderBase.cs:291

Confirm that the 'packageVersion' variable is correctly assigned before this check, as replacing resolvedPackage.Version could lead to unintended behavior if packageVersion is not properly derived.

if (!IsPackageInstalled(new PackageId(resolvedPackage.Id), packageVersion, packageDownloadDir.Value))

baronfel · 2025-06-07T15:09:47Z

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

@@ -12,6 +12,5 @@ Copyright (c) .NET Foundation. All rights reserved.
 <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

  <PropertyGroup>
-    <TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificContentInPackage);PackTool</TargetsForTfmSpecificContentInPackage>


Since I added a PackAsTool condition to the PackTool.targets imports in Sdk.Common.targets, we can't unconditionally add this anymore. We don't know if PackAsTool has been set at this point (because we're pre-project-file-contents in the MSBuild evaluation ordering), so this should get pushed to the PackTool.targets instead. It doesn't effect ordering in anyway, so this is safe.

baronfel · 2025-06-07T15:10:31Z

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

+    <PublishSelfContained Condition="'$(RuntimeIdentifier)' == ''">false</PublishSelfContained>
+    <PublishTrimmed Condition="'$(RuntimeIdentifier)' == ''">false</PublishTrimmed>
+    <PublishReadyToRun Condition="'$(RuntimeIdentifier)' == ''">false</PublishReadyToRun>
+    <PublishAot Condition="'$(RuntimeIdentifier)' == ''">false</PublishAot>


Setting these means that if Build is called accidentally for the RID-less build then things can still mostly work.

baronfel · 2025-06-07T15:11:29Z

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

-  <Target Name="PackAsToolSwitchToNoBuild"
-          BeforeTargets="_IntermediatePack"
-          Condition=" '$(PackAsTool)' == 'true' And
-                      '@(ToolPackageRuntimeIdentifier)' != '' And
-                      '$(RuntimeIdentifier)' == ''">
-
-    <!-- If we are building the primary package for a tool with RID-specific packages, we don't include any of the build output in the package, so we don't need to run Build.
-         We might not be able to build without a RuntimeIdentifier if PublishAOT or SelfContained is set to true, so we skip the build so it doesn't fail.
-
-          We use a task to remove the Build target from GenerateNuspecDependsOn. This is effectively what happens if NoBuild is set, but we can't use the same logic because
-          we need to look at items (ToolPackageRuntimeIdentifier) to determine if we are building the primary package for a tool with RID-specific packages.  So we have to fix
-          up the dependency list afterwards.
-
-          We also can't use MSBuild property functions for string replacement to remove Build from the list, because MSBuild will escape the semicolons as %3B, and there
-          doesn't seem to be a way of avoiding that without using a task.
-     -->
-    <RemoveTargetFromList TargetList="$(GenerateNuspecDependsOn)"
-                          TargetToRemove="Build">
-      <Output TaskParameter="UpdatedTargetList" PropertyName="GenerateNuspecDependsOn" />
-    </RemoveTargetFromList>
-
-  </Target>


This isn't required anymore because we're preventing the errors that calling Build caused in the Property-sets above.

baronfel · 2025-06-07T15:13:47Z

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

  <Target Name="PackTool" DependsOnTargets="SetPackToolProperties;GenerateToolsSettingsFileFromBuildProperty;PackToPublishDependencyIndirection;_PackToolValidation;PackToolImplementation" Condition=" '$(PackAsTool)' == 'true' ">
    <ItemGroup>
      <_GeneratedFiles Include="$(_ToolsSettingsFilePath)"/>
+      <_PublishFilesRaw Condition="'$(RuntimeIdentifier)' != ''" Include="$(PublishDir)/**/*" />


We can't rely on ResolvedFilesToPublish - it's not comprehensive enough. When working with container publish incrementality I found that there are tons of folks that just copy things to the PublishDir and expect that to work. So we have to glob across the publish dir and use that as the source of truth for the package content. The good news is that it makes deriving the Relative and Package paths trivial, so we can removee the entire Task invocation that used to do that.

baronfel · 2025-06-07T15:14:16Z

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

    <ItemGroup>
      <TfmSpecificPackageFile Include="@(_GeneratedFiles)">
-        <PackagePath>tools/$(_ToolPackShortTargetFrameworkName)/any/%(_GeneratedFiles.RecursiveDir)%(_GeneratedFiles.Filename)%(_GeneratedFiles.Extension)</PackagePath>
+        <PackagePath>tools/$(_ToolPackShortTargetFrameworkName)/$(_ToolRidPath)/%(_GeneratedFiles.RecursiveDir)%(_GeneratedFiles.Filename)%(_GeneratedFiles.Extension)</PackagePath>


Request from NuGet here that we maintain the RID in this path if we're doing RID-specific things

baronfel · 2025-06-07T15:15:04Z

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

@@ -1431,7 +1431,7 @@ Copyright (c) .NET Foundation. All rights reserved.
  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.ObsoleteReferences.targets" />
  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Publish.targets" />
  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.PackStubs.targets" Condition="('$(Language)' == 'C++' and '$(_EnablePackageReferencesInVCProjects)' != 'true')"/>
-  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.PackTool.targets" />
+  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.PackTool.targets" Condition="'$(PackAsTool)' == 'true'" />


Adding this conditional means that we can modify things to our heart's content inside the PackTool.targets and not worry about corrupting non-Tool projects' state.

baronfel · 2025-06-08T13:34:41Z

All of the failing tests are around packaging the shims. After my changes the shims are getting the NuGetRecursiveDir metadata added onto them, which is causing them to be put into additional nested subfolders due to logic in the Pack targets:

NuGet.Build.Tasks.Pack.targets:

  <Target Name="_GetTfmSpecificContentForPackage"
          DependsOnTargets="$(TargetsForTfmSpecificContentInPackage)"
          Returns="@(TfmSpecificPackageFileWithRecursiveDir)">
    <!-- The below workaround needs to be done due to msbuild bug https://github.com/Microsoft/msbuild/issues/3121 -->
    <ItemGroup>
      <TfmSpecificPackageFileWithRecursiveDir Include="@(TfmSpecificPackageFile)">
        <NuGetRecursiveDir>%(TfmSpecificPackageFile.RecursiveDir)</NuGetRecursiveDir>
        <BuildAction>%(TfmSpecificPackageFile.BuildAction)</BuildAction>
      </TfmSpecificPackageFileWithRecursiveDir>
    </ItemGroup>
  </Target>

Bad item:

Leads to bad content:

baronfel · 2025-06-08T13:44:52Z

I think I've got it - since NuGet forces the use of RecursiveDir if it's present, we just lean into that and don't try to be quite as precise with our PackagePath setting. I've pushed up a commit with comments and adjustments for that.

baronfel · 2025-06-09T15:53:53Z

Alright - this is super close so is probably ok for review now. The only tests failing don't fail locally, and cover a pretty specific scenario: It_uses_customized_PackagedShimOutputRootDirectory.

MiYanni · 2025-06-10T23:25:57Z

test/Microsoft.NET.ToolPack.Tests/GivenThatWeWantToPackAToolProjectWithPackagedShim.cs

@@ -157,7 +157,7 @@ public void It_uses_customized_PackagedShimOutputRootDirectory(bool multiTarget,

            var packCommand = new PackCommand(Log, helloWorldAsset.TestRoot);

-            packCommand.Execute().Should().Pass();
+            packCommand.Execute("-bl:{}").Should().Pass();


Is this trying to create a binlog during test execution? As mentioned in our triage, even if it works, you cannot get at the file directly in Helix. We need a proper way to do this.

You can hard code the binlong path in the -bl command, and then dump the binary content into the test output, copy paste it into a file to read it. I like the idea of properly dumping the log from helix, but I don't want to block this PR to do so

I think I've figured it out, but now Helix is busted. In either case I'll be removing this before we merge this PR.

baronfel · 2025-06-11T22:07:36Z

Ok, I have added a test to cover the new multi-packaging feature, and have verified via binlogs locally that the shim output path stuff works. I want helix to try one more time to give me a binlog so I can see what's going on in the CI system, but then I want to skip those tests so we can get the good syntax in for p6.

baronfel · 2025-06-12T01:43:55Z

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

+    <_InnerToolsPublishAot>false</_InnerToolsPublishAot>
+    <_InnerToolsPublishAot Condition="'$(RuntimeIdentifier)' == '' and '$(PublishAot)' == 'true'">true</_InnerToolsPublishAot>


We need to know if the inner tools are publishing AOT to know if we're allowed to dispatch to the per-RID inner packs below.

baronfel · 2025-06-12T01:45:01Z

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

+    <_PackToolPublishDependency Condition=" '$(_ToolPackageShouldIncludeImplementation)' != '' and '$(GeneratePackageOnBuild)' == 'true' and $(IsPublishable) == 'true' ">$(_PublishNoBuildAlternativeDependsOn)</_PackToolPublishDependency>
+
+    <!-- Trigger RID-specific inner builds if RID-specific mode is enabled-->
+    <GenerateNuspecDependsOn>$(GenerateNuspecDependsOn);SetDotnetToolPackageType;_CreateRIDSpecificToolPackages</GenerateNuspecDependsOn>


note that these targets must be called from two different kinds of GenerateNuspec:

multi-tfm GenerateNuspec (from the crosstargeting targets)

single-tfm GenerateNuSpec (from the 'normal', single-TFM targets)

baronfel · 2025-06-12T01:47:20Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.CrossTargeting.targets

@@ -13,6 +13,7 @@ Copyright (c) .NET Foundation. All rights reserved.

  <Import Project="Microsoft.NET.Sdk.Common.targets"/>
  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.SourceLink.targets" Condition="'$(SuppressImplicitGitSourceLink)' != 'true'"/>
+  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.PackTool.targets" Condition="'$(PackAsTool)' == 'true'" />


Big change here - we need to import the tool targets at the multi-tfm level so that we can ensure that the correct package types get set.

baronfel · 2025-06-12T03:37:06Z

Ok we're good to go now! The test failures are watch and container timeouts, everything else is good.

… should come from the parent package's verison only

…tead for ease of use

…nges inside them to prevent unintentional modifications

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

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

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

test/Microsoft.NET.ToolPack.Tests/GivenThatWeWantToPackAToolProjectWithPackagedShim.cs

test/Microsoft.NET.TestFramework/ToolsetInfo.cs

test/Microsoft.DotNet.PackageInstall.Tests/TestToolBuilder.cs

src/Cli/dotnet/ToolPackage/ToolPackageDownloaderBase.cs

Directory.Build.props

nagilson

Another PR I wish I had more time to look over but I did what I could with the time we have :) I think using properties vs items is a great improvement and overall the change looks good. Kudos for the clever engineering here...

baronfel · 2025-06-12T22:13:32Z

I've addressed @nagilson's feedback (thanks!) and this should be good to merge now.

nagilson · 2025-06-12T22:21:10Z

mmmmm yes

…t actually exists

baronfel · 2025-06-13T16:27:59Z

The only remaining failing test is the completions known issue. Let's merge!

marcpopMSFT · 2025-06-13T17:31:22Z

Known completion test issue.

Copilot AI review requested due to automatic review settings June 7, 2025 01:00

Copilot AI reviewed Jun 7, 2025

View reviewed changes

This was referenced Jun 7, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

dotnet-watch tests failing #40006

Open

baronfel commented Jun 7, 2025

View reviewed changes

baronfel requested a review from a team June 7, 2025 15:15

baronfel requested a review from a team as a code owner June 7, 2025 15:28

build-analysis bot mentioned this pull request Jun 7, 2025

Throttling of Docker Downloads dotnet/arcade#15659

Open

3 tasks

baronfel force-pushed the multi-rid-packages-tweaks branch from 918efb0 to a17a199 Compare June 8, 2025 13:58

build-analysis bot mentioned this pull request Jun 8, 2025

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

MiYanni approved these changes Jun 10, 2025

View reviewed changes

This was referenced Jun 11, 2025

[dotnet watch] Cannot open project Lib.fsproj because extension fsproj is not associated with a language #46378

Open

RestApiException`1: The response contained an invalid status code 500 Internal Server Error dotnet/dnceng#2298

Open

baronfel commented Jun 12, 2025

View reviewed changes

baronfel force-pushed the multi-rid-packages-tweaks branch from 2c15ee9 to 7b701ca Compare June 12, 2025 02:30

build-analysis bot mentioned this pull request Jun 12, 2025

Microsoft.DotNet.Watch.UnitTests.FileWatcherTests.DeleteSubfolder has flaky test assertion #49233

Open

baronfel force-pushed the multi-rid-packages-tweaks branch from 035ba44 to bbe108a Compare June 12, 2025 13:53

baronfel mentioned this pull request Jun 12, 2025

[New article]: dotnet tools support RID-specific and AOT packages dotnet/docs#46801

Open

baronfel added 5 commits June 12, 2025 13:36

remove version from rid-specific tool package metadata - all versions…

06fa327

… should come from the parent package's verison only

make tool packages publish instead of build

c70bf75

use more precise apphost-detection for command entrypoint

d87a4af

don't use Items for ToolPackageRuntimeIdentifier - use a property ins…

11ae747

…tead for ease of use

condition the PackTool targets so that only tool packages get the cha…

0a37671

…nges inside them to prevent unintentional modifications

baronfel added 10 commits June 12, 2025 13:36

update AOT tool test scenario and fix test breaks

39efc99

temp: add binlogs

dc4dca6

try to write binlogs to helix upload root

af4ddb4

fix lookup path for binlogs

92fb63b

Add test for the multi-package tool creation

2444c06

Fix tool shim output path calculation

578eba8

Remove now-unused Task

55e5c4d

Ensure that RID-specific packages set the right package type

e9f8347

Fix packaging tools that are multi-tfm

f37177c

Add more documentation comments to Targets files

75b4ee6

baronfel force-pushed the multi-rid-packages-tweaks branch from bbe108a to 75b4ee6 Compare June 12, 2025 18:36