Skip to content

Fix duplicate runtime pack crash in dotnet test device deployment #55502

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.

Sign up for GitHub

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
jonathanpeppers merged 2 commits into from
Jul 29, 2026
Merged

Fix duplicate runtime pack crash in dotnet test device deployment #55502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
439823a
Fix duplicate runtime pack crash in dotnet test device deployment
jonathanpeppers Jul 28, 2026
15e421a
Clarify which build gets the fresh ProjectInstance
jonathanpeppers Jul 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/Cli/dotnet/Commands/Test/MTP/SolutionAndProjectUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,10 +619,16 @@ static RunProperties DeployAndGetRunProperties(
lock (s_buildLock)
{
var loggers = logger is null ? null : new[] { logger };
if (project.Targets.ContainsKey(Constants.DeployToDevice) &&
!project.Build([Constants.DeployToDevice], loggers))
if (project.Targets.ContainsKey(Constants.DeployToDevice))
{
throw new GracefulException(CliCommandStrings.RunCommandDeployFailed);
// Deploy on a fresh ProjectInstance to avoid accumulating state (existing item
// groups) that would leak into the ComputeRunArguments build below, which has to
// build the original instance since the run properties are read back from it.
// Same reason as dotnet run, see RunCommandSelector.OpenProjectIfNeeded.
if (!project.DeepCopy().Build([Constants.DeployToDevice], loggers))
{
throw new GracefulException(CliCommandStrings.RunCommandDeployFailed);
}
}

if (!project.Build(s_computeRunArgumentsTarget, loggers))
Comment thread
Evangelink marked this conversation as resolved.
Expand Down
8 changes: 7 additions & 1 deletion test/TestAssets/TestProjects/DotnetTestDevices/DotnetTestDevices.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
</ItemGroup>
</Target>

<Target Name="DeployToDevice">
<!-- ResolveFrameworkReferences mimics Android -->
<Target Name="DeployToDevice" DependsOnTargets="ResolveFrameworkReferences">
<Message Text="DeployToDevice: Deployed $(TargetFramework) to device $(Device) with RuntimeIdentifier $(RuntimeIdentifier)" />
<Message Text="DeployToDevice: RuntimeEnvironmentVariable=@(RuntimeEnvironmentVariable->'%(Identity)=%(Value)', ', ')" />
<Error Condition="'$(FailDeployToDevice)' == 'true'" Text="DeployToDevice failed as requested." />
Expand Down Expand Up @@ -72,5 +73,10 @@
Condition="'@(RuntimeEnvironmentVariable)' != ''">
<Message Text="ComputeRunArguments: RuntimeEnvironmentVariable=@(RuntimeEnvironmentVariable->'%(Identity)=%(Value)', ', ')" />
</Target>
<!-- Unlike dotnet run, dotnet test builds DeployToDevice first and ComputeRunArguments second,
so ResolveFrameworkReferences has to run in the second build too to mimic Android. -->
<Target Name="_ComputeRunArgumentsDependsOnResolveFrameworkReferences"
BeforeTargets="ComputeRunArguments"
DependsOnTargets="ResolveFrameworkReferences" />

@Evangelink Evangelink Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This hook is what carries the whole regression guard, and nothing asserts that it exists. If someone later tidies up this empty target, ResolveFrameworkReferences stops running in the ComputeRunArguments build, every device test still passes, and the protection is gone with no signal. The comment helps, but a comment isn't a failing test.

ItDeploysBeforeComputingRunArguments already reads msbuild-dotnet-test.binlog through the AssertTargetInBinlog helper, so pinning it is cheap:

AssertTargetInBinlog(binlogPath, "ResolveFrameworkReferences", targets =>
    targets.Should().HaveCount(2, "ResolveFrameworkReferences must run in both the DeployToDevice build and the ComputeRunArguments build"));

To be precise about what that buys, since it's easy to overclaim: it guards the asset (deleting this hook takes the count 2 → 1). It does not discriminate fixed CLI from unfixed — before the fix the count is also 2, the second one just fails. Reverting DeepCopy() is already caught by these tests crashing outright. Worth confirming the exact count on a real run before committing to the number.

@jonathanpeppers jonathanpeppers Jul 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If someone later tidies up this empty target ... every device test still passes

So, if someone deleted this target, tests would start failing. I think that is good, and the comment should help someone understand why it exists.

@Evangelink Evangelink Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I checked this and I think it's the other way around: deleting that target makes the tests pass, not fail.

The PR description already measured exactly that configuration:

Test asset CLI Result
DeployToDevice DependsOnTargets only, no ComputeRunArguments hook unfixed 29 passed — bug not caught
Both asset changes unfixed 13 failed
Both asset changes fixed 29 passed

Row 1 is the "someone deleted the target" state, and it's green even against the unfixed CLI. It's green against the fixed CLI too, so the deletion is invisible in both directions.

That follows from the mechanism described in the PR body: the SDK's ComputeRunArguments is empty with no DependsOnTargets. Remove the hook and the second build runs nothing, so nothing accumulates and the duplicate Microsoft.NETCore.App key never occurs. The hook is what creates the failure condition — it isn't something the failure condition depends on.

And nothing else pins it: GivenDotnetTestSelectsDevice.cs never mentions ResolveFrameworkReferences, the asset's ComputeRunArguments targets only touch @(RuntimeEnvironmentVariable), and ItDeploysBeforeComputingRunArguments only asserts DeployToDevice.EndTime <= ComputeRunArguments.StartTime.

So the comment does its job of explaining why the target is there, but there's no failing signal if it goes away — which is why I'd still like a cheap assertion on the target count.


</Project>
Loading