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

Clean up dead code in illink targets #85741

Merged
merged 7 commits into from
May 16, 2023
Merged

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented May 3, 2023

We reference ILLink as a packagereference that versions with the runtime, so the 8.0 linker is only supported when targeting net8.0, and we can remove the TFM checks. I am assuming that we don't want to support someone adding a PackageReference to an 8.0 linker, while targeting an older TFM, in an attempt to get newer linker bits but with settings that were closer to what we shipped with that TFM.

@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label May 3, 2023
@sbomer sbomer requested a review from vitek-karas May 3, 2023 23:41
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 3, 2023
@ghost ghost assigned sbomer May 3, 2023
@ghost
Copy link

ghost commented May 3, 2023

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

We reference ILLink as a packagereference that versions with the runtime, so the 8.0 linker is only supported when targeting net8.0, and we can remove the TFM checks. I am assuming that we don't want to support someone adding a PackageReference to an 8.0 linker, while targeting an older TFM, in an attempt to get newer linker bits but with settings that were closer to what we shipped with that TFM.

Author: sbomer
Assignees: -
Labels:

linkable-framework

Milestone: -

@eerhardt
Copy link
Member

eerhardt commented May 4, 2023

I am assuming that we don't want to support someone adding a PackageReference to an 8.0 linker, while targeting an older TFM,

Maybe we should guard against that? If it isn't supported.

What if I have an app that is multi-targeted? <TargetFrameworks>net6.0;net8.0</TargetFrameworks>. How will that be supported?

@runfoapp runfoapp bot mentioned this pull request May 4, 2023
@sbomer
Copy link
Member Author

sbomer commented May 4, 2023

I added a warning. Some of the test failures look suspect, so maybe this will help us ensure our tests are also using the right TFM.

For multi-targeting, publish requires a single TFM, and will restore the ILLink package matching that TFM unless the user has added a PackageReference already.

@sbomer sbomer force-pushed the illinkTargetsCleanup branch from e1410d2 to 0c73186 Compare May 4, 2023 17:06
@sbomer
Copy link
Member Author

sbomer commented May 4, 2023

Failures are known (#85769 and #80619).

This matches the behavior of nativeaot
@sbomer
Copy link
Member Author

sbomer commented May 15, 2023

@vitek-karas @agocke PTAL

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Nice!

@sbomer sbomer merged commit 0cfc81d into dotnet:main May 16, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2023
@sbomer sbomer deleted the illinkTargetsCleanup branch November 3, 2023 18:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants