-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Ensure that Trimming and AOT scenarios trigger downloading of necessary runtime packs #51765
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
base: main
Are you sure you want to change the base?
Ensure that Trimming and AOT scenarios trigger downloading of necessary runtime packs #51765
Conversation
…also bring in the runtime packs when targeting a single RID. This fixes a gap where 'dotnet restore' for a RID followed by 'dotnet publish --no-restore' for that RID in trimming/AOT scenarios wouldn't work.
| <ItemGroup> | ||
| <ProjectCapability Remove="TestContainer" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is a bugfix from the DevKit folks (thanks @peterwald!) that unblocks the DevKit test explorer experience.
Because this project references xunit (thanks Arcade!) the xunit package adds this capability. Because the project has this capability, DevKit tries to load it as a test. Because this project is a library and doesn't have all of the test dependencies, it fails to load and crashes the test-discovery process.
There was a problem hiding this 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 pull request fixes a gap in runtime pack resolution where projects using PublishTrimmed or PublishAot without SelfContained=true would fail during publish with --no-restore because required runtime packs weren't downloaded during restore.
Key Changes:
- Introduced
DeploymentModelRequiresRuntimeComponentsproperty that consolidates logic for determining when runtime packs are needed, now includingRequiresILLinkPack(set when trimming is enabled) alongsideSelfContained,ReadyToRunEnabled, andPublishAot - Refactored
KnownRuntimePackstruct to use C# 12 primary constructor and readonly properties for better maintainability - Added comprehensive unit tests covering various publish scenarios including trimming and AOT without self-contained deployment
- Created documentation specification detailing expected behavior of
ProcessFrameworkReferencestask across different deployment scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs | Core fix to runtime pack resolution logic; refactored struct to use modern C# features; introduced constants and helper properties for better code clarity |
| src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/ProcessFrameworkReferencesTests.cs | Added comprehensive test coverage for PublishTrimmed, PublishAot, and ReadyToRun scenarios with and without SelfContained; added support for ReadyToRunEnabled in test configuration |
| documentation/specs/ProcessFrameworkReferences-outputs.md | New specification document describing expected outputs for different deployment scenarios and documenting the fix for issue #51667 |
| test/Microsoft.NET.TestFramework/Microsoft.NET.TestFramework.csproj | Removed TestContainer project capability to prevent VS from treating the framework project as a test container |
| tasks.slnf | New solution filter for convenient development focused on build tasks projects |
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| /// <summary> | ||
| /// We have several deployment models that require the use of runtime assets of various kinds. | ||
| /// This member helps identify when any of those models are in use, because we've had bugs in the past | ||
| /// where we didn't properly account for all of them. | ||
| /// </summary> | ||
| private bool DeploymentModelRequiresRuntimeComponents => | ||
| SelfContained || ReadyToRunEnabled || PublishAot || RequiresILLinkPack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This member is the actual fix. Every other change is tests, infra, debugging utility, clarity, etc.
|
Nit: per #51766 (comment) |
| <SelfContained>true</SelfContained> | ||
| <RuntimeIdentifier>linux-x64</RuntimeIdentifier> | ||
| <ReadyToRunEnabled>true</ReadyToRunEnabled> | ||
| <ReadyToRunUseCrossgen2>true</ReadyToRunUseCrossgen2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a minimum runtime pack version that the current .NET SDK is compatible with? Does the current .NET SDK still support publishing for .NET 5 in all flavors?
UseCrossgen2 property was a thing in .NET 5 where crossgen2 was in preview and setting this property to true was used to opt-in into the preview. It has no purpose (it is always true) in .NET 6+.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK does still support publishing net5 apps - though you get a warning that that runtime is out of support. The property is mostly mentioned here for completeness given that it does play into the runtime pack selection in the processframeworkreferences task today - I didn't want a reader to read this and then see an entirely new concept in the implementation and wonder if we missed something critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK does still support publishing net5 apps
Do you have a deprecation schedule for when .NET SDK is going to drop support for runtimes that are out of support for a long time?
I do not think it is a good use of resources to carry this baggage around, and the support is likely broken in number of ways anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why currently have no timeline for this. The general approach has been to allow everything, but only explicitly test 2 LTSs back. So for example net 6 is the lower bound on a lot of our tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why currently have no timeline for this. The general approach has been to allow everything, but only explicitly test 2 LTSs back. So for example net 6 is the lower bound on a lot of our tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove testing, we have to assume that the functionality for that targeting will degrade from release to release and we're ok with that.
It's not supported today but we've held off taking any action as we didn't want to actively break customers.
What value does this provide? It builds false trust with customer that we will eventually break. In theory we could make a change and unintentionally break a "major" scenario for an EOL RID. This is saying we are ok with that. Let's be proactive in keeping the codebase clean and free of legacy baggage in support of quicker innovation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s worth spelling out what the alternative looks like in these cases: users will need to download old SDKs to publish for old frameworks. I think that’s a desirable state to be in. If a customer were to ask me how they should continue building for netcoreapp3.1, knowing it is long out of support, that is what I would recommend. In the case where no testing is run and nothing is being supported, I would prioritize no changes over all else. As a netcoreapp3.1 user, no changes being made for net10 would be worth the risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's the proposal. I gather this is what we're going to do. Retain the ability to acquire and build not test it.
Proposal for .NET 11 SDK:
net10.0+ -- supportednet8.0+ -- testednet1.0+ -- mechanical enabled (will download TP and should work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Andy is pointing out that he would not ever recommend to a customer to depend on net1.0+ mechanical enablement in .NET 11 SDK. I would do the same. It does not form a coherent story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I missed a key point in his first sentence. Yes, that sounds best. In any case, we agree on the proposed end point.
…n't explicitly set to true until ILLink targets are updated
| <!-- The ILLink Targets expect projects to set SelfContained specifically, but the real condition they're trying to detect is | ||
| 'does this publish scenario require runtime components'. SelfContained is just the 'legacy' signal for that. | ||
| See https://github.com/dotnet/sdk/pull/51765#issuecomment-3537080089 for more context. | ||
| To work around this current constraint, we can detect the scenarios where runtime components are required | ||
| and set SelfContained accordingly before ILLink runs. It's ok to set SelfContained this 'late' because | ||
| the ProcessFrameworkReferences Task (which is where runtime packs are resolved) has already run by this | ||
| point and it has been updated to use this 'expanded' definition of when runtime components are required | ||
| already (e.g. the ProcessFrameworkReferences.DeploymentModelRequiresRuntimeComponents member). | ||
| The _RequiresILLinkPack property is set based on this expanded definition, so we can reuse it as our signal here. | ||
| It's possible that this property is what the ILLink targets should be using directly instead of SelfContained, but | ||
| we may want to have more 'abstract', non-private signal. | ||
| --> | ||
| <Target Name="_AlignSelfContainedBeforeILLink" BeforeTargets="PrepareForPublish" Condition=""> | ||
| <PropertyGroup> | ||
| <SelfContained Condition="'$(SelfContained)' != 'true' and '$(_RequiresILLinkPack)' == 'true'">true</SelfContained> | ||
| </PropertyGroup> | ||
| </Target> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a target to the SDK that 'patches' the SelfContained status so that we can demonstrate end-to-ends - we should have a conversation on what the ILLink Targets should be conditioning on instead of SelfContained directly.
|
Is that dotnet/runtime issue from 2021 what you intended to link to? I do expect that the extra care you are providing them 4 years later is much appeciated. |
|
|
||
| <!-- The ILLink Targets expect projects to set SelfContained specifically, but the real condition they're trying to detect is | ||
| 'does this publish scenario require runtime components'. SelfContained is just the 'legacy' signal for that. | ||
| See https://github.com/dotnet/sdk/pull/51765#issuecomment-3537080089 for more context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update ILLink to use whatever is the non-legacy signal?
I think the comment isn't entirely accurate - the ILLink targets are trying to detect whether the publish output is self-contained since we don't support trimming individual libraries as an end-user scenario. Would PublishSelfContained be a better signal? (I'm aware that wouldn't work today because of how PublishSelfContained is implemented.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - totally agree here. This entire Target should just be looked at as a 'bridge' - to see if I can get the new tests to show the 'expected'/'desired' behavior. I think it's entirely reasonable now that I have tests that I like and are green to shift this PR over to the VMR so that the ILLink condition could be updated and this target could be removed - and the tests could be run in-situ. I just don't like the experience of working directly in the VMR so I tried to push that off as long as possible :)
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Show resolved
Hide resolved
|
Tests are green on this - next steps are to port this over to the VMR, delete the 'bridge' Target I added here, and update the ILLink conditions I mentioned so that the whole change is applied in one go. |
test/Microsoft.NET.TestFramework/Microsoft.NET.TestFramework.csproj
Outdated
Show resolved
Hide resolved
…sproj Co-authored-by: Michael Yanni <[email protected]>
|
FYI the fact that PublishSelfContained sets SelfContained is an undesirable behavior from the runtime perspective. Because SelfContained impacts both Publish and Build it causes the build to produce self-contained output when publishing for trimming/aot, which no one wants. |
agreed - I just have a much less firm on the blast radius of making that change. I could see trying to attack it as a follow-up in the VMR after this change lands (since this change 'just' fixes the runtime-acquisition behavior for PublishTrimmed/PublishAot). |
| not by the user. | ||
| --> | ||
| <Target Name="_AlignSelfContainedBeforeILLink" BeforeTargets="PrepareForPublish" Condition="'$(_SelfContainedWasSpecified)' != 'true' and '$(SelfContained)' == 'false' and '$(_RequiresILLinkPack)' == 'true'"> | ||
| <PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally use RequiresILLink in process framework reference but _RrequiresILLink here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, could SelfContained be empty at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe SelfContained can be empty at this point - the earlier logic in the RuntimeIdentifierI ference targets forces it to a Boolean as far as I can tell.
I can try to check about the naming thing - unsure as to my thought process and what the binlogs were showing at that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: the naming:
_RequiresILLinkPack is the 'private' MSBuild Property that is already computed in other Targets in the Publish targets - it feeds into ProcessFrameworkReferences and is fed into the RequiresILLinkPack Task parameter - I'm not sure I've ever seen an MSBuild Task with a prefix-leading name - Task parameters are always thought of as public surface area so the distinction in name makes sense to me here.
| [InlineData("PublishTrimmed", false)] | ||
| public void SomePublishPropertiesInferSelfContained(string property, bool useFrameworkDependentDefaultTargetFramework) | ||
| [InlineData("PublishReadyToRun", false)] // R2R doesn't imply self-contained in 8 and above | ||
| [InlineData("PublishSingleFile", true)] // single-file implies self-contained |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs updating - PublishSingleFile should not force the use of self-contained mechanisms. This was a mistake on my part. The doc in documentation/specs/ProcessFrameworkReferences-outputs.md needs to be updated, this test needs to add the PublishSingleFile/false case back in, and changes need to be applied to the ProcessFrameworkReferences Task to make it green.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I am indeed wrong, again! The defaults for PublishSingleFile are currently set to prefer self-contained - but a user can disable the selfcontained part if they want.

Fixes #51667, #51766, #50093 and likely many others.
There was a gap when restoring a project for a RID and then publishing it for that same RID with
--no-restorewhere if that project only set PublishTrimmed or PublishAOT, the runtime packs for that RID would not be restored. This would lead to the publish command failing saying that required runtime packs weren't available. There were a couple workarounds:Along the way I added some tests to cover the new scenarios, and extracted out a documentation spec to capture how we think ProcessFrameworkReferences should behave, modulo any bugs. In the future we should be able to reference this when answering questions about what the expected behavior is.
I also made a
tasks.slnfto make it easier to iterate on the SDK's MSBuild Tasks and their tests in isolation. This massively helps the VSCode user experiences, especially on the testing inner loop.