-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support PublishReadyToRun for framework-dependent deployment #3326
Conversation
cc @dsplaisted @fadimounir @peterhuene This is still a draft, but I'm interested in feedback on the approach. It feels a little clunky that you now cannot assume invariant of RuntimePackAsset non-empty == self-contained, and that I had to run a separate conflict resolution pass in the crossgen + fdd. So I'm interested in other ideas. I don't think this is terrible, though, so if you think this isn't too bad, I'll probably stick with it. Working on tests and fixing the error message still. We had been lumping #3109 and #3110 together in discussions, but they're actually quite separate. This only fixes #3110. I will move on the #3109 next once this is out of draft. |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.TargetingPackResolution.targets
Show resolved
Hide resolved
/cc @sbomer |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Show resolved
Hide resolved
Can we now delete this:
|
No. We need #3109 for that. It appears the comment is referencing the wrong bug. I'll fix that. |
Went ahead and marked this ready for review. If we decide to take a fundamentally different approach, I'll close and start fresh on that. |
@dsplaisted @fadimounir @dotnet/dotnet-cli Ping for review. |
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunCrossgen.cs
Outdated
Show resolved
Hide resolved
This comment can be deleted, and replace Refers to: src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets:321 in 275920f. [](commit_id = 275920f, deletion_comment = False) |
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 _CreateR2RSymbols
target needs to be fixed to use the _ReadyToRunImplementationAssemblies
list
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.
LGTM
…026.1 (dotnet#3326) - Microsoft.NET.Sdk.Web - 3.1.100-preview2.19526.1
Fix #3110