Ensure that tool packages can still publish normally with `dotnet publish`. #50445

baronfel · 2025-08-22T22:21:38Z

The PackTool.targets were modifying critical properties of the tool package project that impacted the way the tool would be published, not just packed.

To fix this, we needed to make sure pathways that are called during 'normal publishing' and 'packaging' could distinguish between the two. This meant

making sure that RID-defaulting (UseCurrentRuntimeIdentifier) didn't happen for packaging, only for publishing
making sure that some RID specific validation doesn't fire for packaging
and in the pack tool targets themselves, making sure that we only override the properties that influence RID-package selection (Publish*, etc) when we're both actively packaging and we know the package is actually not-RID-specific

Almost all of this is super gross, and I believe that the more correct way to structure all of this would be to push more of this overriding into Targets that set Properties less-conditionally and more clearly, but that pathway led to a lot of follow-on changes that were even harder to reason about, and I want to make sure we can get this in for RC2 so folks aren't blocked.

Copilot

Pull Request Overview

This PR fixes an issue where PackTool.targets were incorrectly modifying properties that affected tool publishing behavior, not just packaging. The fix restructures the targets to ensure tool-specific modifications only activate during pack operations.

Key changes:

Moves tool-specific property modifications from global scope to pack-specific targets
Adds _IsPacking property to PackCommand and related test framework classes
Updates file-based program support to include dotnet pack functionality

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets	Restructures targets to move property modifications into pack-specific scope
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets	Removes PackAsTool condition from runtime identifier inference
src/Cli/dotnet/Commands/Pack/PackCommand.cs	Adds file-based program support and refactors command creation
test/Microsoft.NET.TestFramework/Commands/*.cs	Adds `_IsPacking` and `_IsPublishing` properties to test commands
test/dotnet.Tests/CommandTests/Run/RunFileTests.cs	Adds comprehensive tests for pack functionality with file-based programs
test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAToolProject.cs	Adds regression test for normal publish behavior
eng/Version.Details.xml	Updates Microsoft.Testing.Platform and MSTest package versions

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

test/Microsoft.NET.TestFramework/Commands/PackCommand.cs

baronfel · 2025-08-29T03:08:35Z

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

+    <_PackToolPublishDependency Condition=" $(_ToolPackageShouldIncludeImplementation) and '$(GeneratePackageOnBuild)' != 'true' and $(IsPublishable) == 'true' ">Publish</_PackToolPublishDependency>
    <!-- BUT builds that GeneratePackageOnBuild can't directly rely on Publish since Publish would trigger Build, causing an infinite loop. -->
    <!-- To get around this, we try a bit of a workaround: since we can't set NoBuild=true and call Publish that way, we instead trigger all of the
         dependencies of the no-build Publish Target, _PublishNoBuildAlternative -->
-    <_PackToolPublishDependency Condition=" '$(_ToolPackageShouldIncludeImplementation)' != '' and '$(GeneratePackageOnBuild)' == 'true' and $(IsPublishable) == 'true' ">$(_PublishNoBuildAlternativeDependsOn)</_PackToolPublishDependency>
+    <_PackToolPublishDependency Condition=" $(_ToolPackageShouldIncludeImplementation) and '$(GeneratePackageOnBuild)' == 'true' and $(IsPublishable) == 'true' ">$(_PublishNoBuildAlternativeDependsOn)</_PackToolPublishDependency>
+    <!-- No implementation required? Then don't call either target. -->
+    <_PackToolPublishDependency Condition="!$(_ToolPackageShouldIncludeImplementation)"></_PackToolPublishDependency>


these three sets were actually a big part of the problem - they were incorrectly checking _ToolPackageShouldIncludeImplementation in a way that caused even top-level non-implementation packages to call Publish - which then causes the negative impacts observed by properties like PublishAot, etc. The final clause here explicitly makes sure that we end up calling no Publish-related Target if we don't need a tool implementation.

baronfel · 2025-08-29T03:11:02Z

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

    <PreferNativeArm64 Condition="'$(PreferNativeArm64)'==''">false</PreferNativeArm64>
    <SignAssembly Condition="'$(SignAssembly)'==''">false</SignAssembly>
    <DelaySign Condition="'$(DelaySign)'==''">false</DelaySign>
+    <!-- TODO: should we set _IsPacking=true on the NuGet Pack targets when they hook up the 'generate package on build' stuff -->


@dsplaisted / @zivkan I think this is potentially a thing that we should dig into.

Here's the relevant part of the Pack Targets:

<Target Name="_PackAsBuildAfterTarget" AfterTargets="Build" Condition="'$(GeneratePackageOnBuild)' == 'true' AND '$(IsInnerBuild)' != 'true'" DependsOnTargets="Pack"> </Target> <Target Name="_CleanPackageFiles" DependsOnTargets="_GetOutputItemsFromPack" AfterTargets="Clean" Condition="'$(GeneratePackageOnBuild)' == 'true' AND '$(IsInnerBuild)' != 'true'"> <ItemGroup> <_PackageFilesToDelete Include="@(_OutputPackItems)"/> </ItemGroup> <Delete Files="@(_PackageFilesToDelete)"/> </Target>

It sucks to keep propagating _IsPacking, but I think we might need to. Ideally sometime after the build runs but before the 'hook' NuGet _PackAsBuildAfterTarget ties into the Pack DependsOnTargets?

I don't think we can successfully set _IsPacking in these targets, as there are evaluation-time properties in Microsoft.NET.PackTool.targets that depend on the _IsPacking value.

… due to release schedules

…operty-set Condition check instead of simply not importing the targets at all when not packaging

…ckages to call Publish unecessarily

…nly-on-pack

baronfel · 2025-09-05T18:04:41Z

per @dsplaisted the three remaining test failures are actually related to PrunePackageReferences and the way we construct some SDK layout stuff, not this PR. I'd be ok with merging this as a result.

dsplaisted · 2025-09-05T19:34:41Z

The test errors should be fixed by #50661.

…nly-on-pack

baronfel · 2025-09-06T20:13:35Z

@dsplaisted it's finally ready!

dsplaisted

Looks good, and not as complicated as I had feared.

I think it would be good to get rid of all the HELIX_WORKITEM_UPLOAD_ROOT ibnlog-gathering code from the tests.

dsplaisted · 2025-09-16T17:55:24Z

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

+    <PublishSelfContained Condition="!$(_IsRidSpecific) and !$(_IsImplicitRestore) and $(_IsPacking)">false</PublishSelfContained>
+    <SelfContained Condition="!$(_IsRidSpecific) and !$(_IsImplicitRestore) and $(_IsPacking)">false</SelfContained>
+    <PublishTrimmed Condition="!$(_IsRidSpecific) and !$(_IsImplicitRestore) and $(_IsPacking)">false</PublishTrimmed>
+    <PublishReadyToRun Condition="!$(_IsRidSpecific) and !$(_IsImplicitRestore) and $(_IsPacking)">false</PublishReadyToRun>
+    <PublishSingleFile Condition="!$(_IsRidSpecific) and !$(_IsImplicitRestore) and $(_IsPacking)">false</PublishSingleFile>
+    <PublishAot Condition="!$(_IsRidSpecific) and !$(_IsImplicitRestore) and $(_IsPacking)">false</PublishAot>


The same condition is repeated here for a bunch of properties. Would it make sense to put these in a separate PropertyGroup with the condition just once on the PropertyGroup?

And if we do that it might make sense to avoid creating our own _IsImplicitRestore property or forcing _IsPacking to have a value if it's not set.

dsplaisted · 2025-09-16T21:25:55Z

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


    <!-- inner builds and non-RID-specific outer builds need publish content-->
-    <_PackToolPublishDependency Condition=" '$(_ToolPackageShouldIncludeImplementation)' != '' and '$(GeneratePackageOnBuild)' != 'true' and $(IsPublishable) == 'true' ">Publish</_PackToolPublishDependency>
+    <_PackToolPublishDependency Condition=" $(_ToolPackageShouldIncludeImplementation) and '$(GeneratePackageOnBuild)' != 'true' and $(IsPublishable) == 'true' ">Publish</_PackToolPublishDependency>


I can't comment the exact line since you didn't change it here, but would it make sense to change this:

<_ToolPackageShouldIncludeImplementation Condition=" '$(_ToolPackageShouldIncludeImplementation)' == '' And '$(PackAsTool)' == 'true' And ('$(_UserSpecifiedToolPackageRids)' == '' or '$(RuntimeIdentifier)' != '')">true</_ToolPackageShouldIncludeImplementation>

to this?

<_ToolPackageShouldIncludeImplementation Condition=" '$(_ToolPackageShouldIncludeImplementation)' == '' And '$(PackAsTool)' == 'true' And ('$(_HasRIDSpecificTools)' != 'true' or '$(RuntimeIdentifier)' != '')">true</_ToolPackageShouldIncludeImplementation>

Basically use the derived _HasRIDSpecificTools property. I think the result would be the same and this would be a bit easier to understand.

dsplaisted · 2025-09-16T22:21:41Z

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

    <PreferNativeArm64 Condition="'$(PreferNativeArm64)'==''">false</PreferNativeArm64>
    <SignAssembly Condition="'$(SignAssembly)'==''">false</SignAssembly>
    <DelaySign Condition="'$(DelaySign)'==''">false</DelaySign>
+    <!-- TODO: should we set _IsPacking=true on the NuGet Pack targets when they hook up the 'generate package on build' stuff -->


I don't think we can successfully set _IsPacking in these targets, as there are evaluation-time properties in Microsoft.NET.PackTool.targets that depend on the _IsPacking value.

dsplaisted · 2025-09-16T22:23:04Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildAnAppWithoutTransitiveProjectRefs.cs

+            var binlogDestPath = Environment.GetEnvironmentVariable("HELIX_WORKITEM_UPLOAD_ROOT") is { } ciOutputRoot && Environment.GetEnvironmentVariable("HELIX_WORKITEM_ID") is { } helixGuid ?
+                Path.Combine(ciOutputRoot, "binlog", helixGuid, $"{nameof(It_cleans_the_project_successfully_with_static_graph_and_isolation)}.binlog") :
+                "./msbuild.binlog";


I think this can be removed in favor of the logic that's now built in to TestCommand.

Copilot AI review requested due to automatic review settings August 22, 2025 22:21

baronfel requested a review from a team as a code owner August 22, 2025 22:21

baronfel changed the base branch from main to release/10.0.1xx August 22, 2025 22:21

dotnet deleted a comment from github-actions bot Aug 22, 2025

Copilot AI reviewed Aug 22, 2025

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets Show resolved Hide resolved

baronfel marked this pull request as draft August 25, 2025 20:44

baronfel force-pushed the make-packtool-targets-light-up-only-on-pack branch from 8644cd8 to dedfaf5 Compare August 26, 2025 02:48

This was referenced Aug 28, 2025

error : HttpRequestException: The SSL connection could not be established, see inner exception. dotnet/dnceng#5015

Open

arcade/src/Microsoft.DotNet.Arcade.Sdk/src/DownloadFile.cs:line 184 TimeoutException: The operation was canceled. dotnet/dnceng#6074

Open

zivkan reviewed Aug 28, 2025

View reviewed changes

test/Microsoft.NET.TestFramework/Commands/PackCommand.cs Show resolved Hide resolved

baronfel commented Aug 29, 2025

View reviewed changes

baronfel marked this pull request as ready for review August 29, 2025 03:11

baronfel force-pushed the make-packtool-targets-light-up-only-on-pack branch from 8695a37 to 630278d Compare August 29, 2025 03:11

build-analysis bot mentioned this pull request Aug 29, 2025

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

baronfel and others added 9 commits September 3, 2025 17:49

Don't modify the ambient state for non-pack workflows

11c7c53

drop a binlog

cc95d96

another binlog

af3e647

revert previous approach, go with simpler-but-less-good-long-term fix…

0a668c8

… due to release schedules

PackTool targets are needed for some side-effect, so move to a per-pr…

82cbee2

…operty-set Condition check instead of simply not importing the targets at all when not packaging

revert URC condition change

d802647

Don't modify behavior when PackAsTool is set unless actually packing

8797234

Fix really silly condition inversion

a162f89

Fix a really dumb defaulting error that was causing top-level tool pa…

ad63e78

…ckages to call Publish unecessarily

baronfel force-pushed the make-packtool-targets-light-up-only-on-pack branch from 630278d to ad63e78 Compare September 3, 2025 22:49

fix test expectation that only really applies on older TFMs

66d30f9

baronfel requested a review from a team as a code owner September 4, 2025 02:10

build-analysis bot mentioned this pull request Sep 4, 2025

Sometimes the helix SDK uses GetWorkItemsAsync when workitems aren't done processing. dotnet/dnceng#6011

Open

3 tasks

don't try to be smart because some of our tools can't handle it

1f9e31a

build-analysis bot mentioned this pull request Sep 4, 2025

Nuget: Central Directory corrupt dotnet/dnceng#3099

Closed

3 tasks

baronfel and others added 4 commits September 4, 2025 15:00

binlogs for remaining failing tests

3422187

scope binlogs by workitem id

33bc7e9

Merge branch 'release/10.0.1xx' into make-packtool-targets-light-up-o…

6f3fce1

…nly-on-pack

even more binlogs

0c3ed3c

Merge branch 'release/10.0.1xx' into make-packtool-targets-light-up-o…

2b12e3c

…nly-on-pack

build-analysis bot mentioned this pull request Sep 6, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

baronfel requested a review from a team September 6, 2025 20:13

baronfel enabled auto-merge (squash) September 6, 2025 20:13

baronfel changed the title ~~Make packtool targets light up only on pack~~ Ensure that tool packages can still publish normally with dotnet publish. Sep 9, 2025

dsplaisted approved these changes Sep 16, 2025

View reviewed changes

baronfel merged commit 5b0a505 into dotnet:release/10.0.1xx Sep 16, 2025
27 checks passed

baronfel deleted the make-packtool-targets-light-up-only-on-pack branch September 16, 2025 23:17

baronfel mentioned this pull request Sep 16, 2025

address Daniel's feedback #50876

Open

dsplaisted mentioned this pull request Sep 18, 2025

AOT publish doesn't work if PackAsTool is set (a regression) #50435

Closed

Ensure that tool packages can still publish normally with dotnet publish. #50445

Ensure that tool packages can still publish normally with dotnet publish. #50445

Uh oh!

Conversation

baronfel commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

baronfel Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baronfel Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

dsplaisted Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

baronfel commented Sep 5, 2025

Uh oh!

dsplaisted commented Sep 5, 2025

Uh oh!

baronfel commented Sep 6, 2025

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

dsplaisted Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

dsplaisted Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

dsplaisted Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

dsplaisted Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ensure that tool packages can still publish normally with `dotnet publish`. #50445

Ensure that tool packages can still publish normally with `dotnet publish`. #50445

baronfel commented Aug 22, 2025 •

edited

Loading

baronfel Aug 29, 2025 •

edited

Loading