Skip to content

Fix Framework-Dependent, RuntimeIdentifier-specific tool execution #49521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2025

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Jun 22, 2025

Builds on top of #49501, merge that one first.

Fixes #49494

The new multi-RID tool support relies on using the 'runner' set in the tool manifest to properly discover how to launch the app in the tool package. For this to work, the 'runner' type ('dotnet'/'executable') needs to align with the name of the binary inside the tool package we're running ('toolname.dll'/'toolname[.exe]'). These two were mismatched because we were pivoting the detection of which execution mode to use for tools off of the wrong property - if an apphost exists for the tool then we need to use the executable strategy.

A bit of test refactoring made this much easier to verify.

@baronfel baronfel requested review from Copilot and a team June 22, 2025 04:30
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR corrects how the SDK chooses executable vs. framework‐dependent runners based on whether an apphost is used, and refactors tests to make RID‐specific and trimmed tool packaging easier to assert.

  • TestToolBuilder:
    • Propagate a RidSpecific flag from various properties and include it in the tool identifier.
    • Add a collectBinlogs option to pack commands and handle RuntimeIdentifiers centrally.
  • End-to-end tests:
    • Introduce new tests covering multiple RID‐specific and trimmed tool packaging.
    • Refactor XML helpers and add assertions for trimmed dependencies.
  • PackTool.targets:
    • Preserve publish-related properties (SelfContained, Publish*) during restore.
    • Switch ToolCommandRunner to use UseAppHost instead of just SelfContained.

Reviewed Changes

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

File Description
test/.../TestToolBuilder.cs Updated TestToolSettings auto-properties and CreateTestTool
test/.../EndToEndToolTests.cs Added RID-specific and trimmed packaging tests and helpers
src/.../Microsoft.NET.PackTool.targets Adjusted publish property conditions and ToolCommandRunner
Comments suppressed due to low confidence (7)

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

  • Auto-properties cannot reference field. You need an explicit backing field (e.g. _nativeAot) and use that in the getter/setter.
            public bool NativeAOT { get; set { field = value; this.RidSpecific = value; } } = false;

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

  • Same issue here—auto-properties don't expose field. Introduce a private backing field for SelfContained.
            public bool SelfContained { get; set { field = value; this.RidSpecific = value; } } = false;

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

  • This setter also uses field. Define an explicit backing field (e.g. _trimmed) before referencing it.
            public bool Trimmed { get; set { field = value; this.RidSpecific = value; } } = false;

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

  • The IncludeAnyRid setter still refers to field. You must back these auto-properties with a private field.
            public bool IncludeAnyRid { get; set { field = value; this.RidSpecific = value; } } = false;

test/Microsoft.DotNet.PackageInstall.Tests/EndToEndToolTests.cs:168

  • In this test you reference expectedRids later but never define it. Add var expectedRids = ToolsetInfo.LatestRuntimeIdentifiers.Split(';'); before using it.
        public void PackagesMultipleTrimmedToolsWithASingleInvocation()

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

  • MSBuild conditions must compare strings. Change the condition to Condition="'$(UseAppHost)' != 'true'" so it parses correctly.
    <ToolCommandRunner Condition="!$(UseAppHost)">dotnet</ToolCommandRunner>

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

  • This should read Condition="'$(UseAppHost)' == 'true'" to reliably detect when an apphost is used.
    <ToolCommandRunner Condition="$(UseAppHost)">executable</ToolCommandRunner>

@baronfel baronfel changed the title Fix fdd rid specific tool execution Fix Framework-Dependent, RuntimeIdentifier-specific tool execution Jun 22, 2025
@baronfel baronfel force-pushed the fix-fdd-rid-specific-tool-execution branch from b348571 to 2529cf7 Compare June 24, 2025 20:34
@baronfel baronfel enabled auto-merge (squash) June 24, 2025 20:34
@baronfel baronfel merged commit c0557ec into dotnet:main Jun 24, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multi-RID tools that are framework-dependent do not work
2 participants