Skip to content
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

Don't add AOT package in restore for < net7 #31201

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Mar 14, 2023

Fixes #30814.

In order to avoid errors for invalid RIDs for AOT during restore, we need to avoid trying to restore a package for ILC when TFM < 7. Then, we need to warn during publish when there are no ILC packages found during restore to make sure we don't still go through with publish with PublishAOT=true and TFM<7.

@jtschuster jtschuster added Area-NativeAOT Native AOT compilation and removed Area-ReadyToRun labels Mar 14, 2023
@jtschuster jtschuster marked this pull request as ready for review March 14, 2023 22:55
@jtschuster jtschuster requested review from AntonLapounov and a team as code owners March 14, 2023 22:55
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Could you check if the other publish scenarios have the same problem? I think you could get it with PublishReadyToRun (maybe you need to opt into crossgen2) on downlevel TFMs too.

@jtschuster jtschuster requested a review from sbomer June 9, 2023 22:22
@jtschuster
Copy link
Member Author

I forgot to follow up on the PR feedback and let this get stale. The issue was still present in preview 4, and this fix is still necessary. @sbomer @agocke Can you take another look?

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this!

Looking at this a second time, I don't think adding a TFM condition to the task is a good idea. The source of truth for which TFMs are supported by the various scenarios (not just AOT) lives in https://github.com/dotnet/installer/blob/main/src/redist/targets/GenerateBundledVersions.targets, and ProcessFrameworkReferences is the current choke point that produces errors for all of these scenarios. I've been leaning on this to improve the error messages (see #32943 and #33041).

The current restore behavior (restoring for all TFMs) is by design per #30814 (comment), so if we're unhappy with this behavior I think we should be looking at a broader solution that changes the design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NativeAOT Native AOT compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PublishAOT property causes build failure when used with earlier TFMS in TargetFrameworks property
3 participants