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

Run illink before ApiCompat #66706

Merged
merged 3 commits into from
Apr 26, 2022
Merged

Run illink before ApiCompat #66706

merged 3 commits into from
Apr 26, 2022

Conversation

ViktorHofer
Copy link
Member

As observed in #66634 (comment), illink currently runs after APICompat. This happens because nuget imports the ApiCompat targets earlier than the illink.targets is imported.

This should flag issues where illink removes interfaces even though it shouldn't in library mode.

cc @bartonjs @ericstj

@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 16, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Infrastructure-libraries and removed linkable-framework Issues associated with delivering a linker friendly framework labels Mar 16, 2022
@ghost ghost assigned ViktorHofer Mar 16, 2022
@ghost
Copy link

ghost commented Mar 16, 2022

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

Issue Details

As observed in #66634 (comment), illink currently runs after APICompat. This happens because nuget imports the ApiCompat targets earlier than the illink.targets is imported.

This should flag issues where illink removes interfaces even though it shouldn't in library mode.

cc @bartonjs @ericstj

Author: ViktorHofer
Assignees: -
Labels:

linkable-framework

Milestone: -

@bartonjs
Copy link
Member

I think, in my head, the ideal model is that GenAPI would run from the untrimmed compiler output, and APICompat would run on the trimmer output. That way if GenAPI thinks something is relevant that the trimmer didn't, we catch it.

@ViktorHofer ViktorHofer changed the title Run illink before ApiCompat and GenAPI Run illink before ApiCompat Mar 16, 2022
eng/illink.targets Outdated Show resolved Hide resolved
@ViktorHofer ViktorHofer added the blocked Issue/PR is blocked on something - see comments label Mar 29, 2022
@vitek-karas
Copy link
Member

The trimmer problem dotnet/linker#2238 has been fixed. Once it flows into runtime it should not remove any of the interfaces (or their methods).

As observed in #66634 (comment), illink currently runs after APICompat. This happens because nuget imports the ApiCompat targets earlier than the illink.targets is imported.
@ViktorHofer ViktorHofer removed the blocked Issue/PR is blocked on something - see comments label Apr 22, 2022
@ViktorHofer ViktorHofer force-pushed the ViktorHofer-patch-2 branch from 11f1ccd to 03165bd Compare April 22, 2022 07:43
@ViktorHofer ViktorHofer marked this pull request as ready for review April 22, 2022 07:44
@ViktorHofer
Copy link
Member Author

The change is now ready. PTAL

@ViktorHofer
Copy link
Member Author

@ericstj @carlossanlop @dotnet/area-infrastructure-libraries can someone please approve?

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@ViktorHofer ViktorHofer merged commit 6387a2d into main Apr 26, 2022
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-2 branch April 26, 2022 06:18
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants