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

Move ILLink targets to the linker repo #25993

Merged
merged 4 commits into from
Jun 21, 2022
Merged

Conversation

agocke
Copy link
Member

@agocke agocke commented Jun 14, 2022

This will keep the ILLink task and targets in-sync.

This will keep the ILLink task and targets in-sync.
@agocke
Copy link
Member Author

agocke commented Jun 14, 2022

I think there's some potential simplification here by getting rid of the "SDK" stuff entirely. Just <Import Include="../../Microsoft.NET.Sdk.ILLink/Microsoft.NET.Sdk.ILLink.Tasks.props" etc

@agocke
Copy link
Member Author

agocke commented Jun 14, 2022

@dsplaisted Is there any benefit to the "Sdk" login in MSBuild for these built-in components? It seems like it's just a space/time saver for people typing project files manually.

@agocke agocke marked this pull request as draft June 15, 2022 21:26
@agocke agocke closed this Jun 15, 2022
@agocke agocke reopened this Jun 15, 2022
@agocke agocke marked this pull request as ready for review June 21, 2022 06:54
@agocke agocke requested a review from sbomer June 21, 2022 06:54
@agocke
Copy link
Member Author

agocke commented Jun 21, 2022

@MichalStrehovsky FYI, in case this conflicts with anything you're thinking. I was planning on extracting trimming pieces that are shared between NativeAOT and ILLink into the central publish targets (like the feature switches).

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky FYI, in case this conflicts with anything you're thinking. I was planning on extracting trimming pieces that are shared between NativeAOT and ILLink into the central publish targets (like the feature switches).

Looks good to me. We should do the ILLinkTargetPath for NativeAOT targets as well. (Not really a comment for this pull request, just an observation.)

@agocke agocke requested a review from LakshanF June 21, 2022 16:08
Copy link
Contributor

@LakshanF LakshanF left a comment

Choose a reason for hiding this comment

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

I like the effort to keep in sync as well streamline common actions between NativeAOT and Trimming. I'm assuming that the existing tests we have for both NativeAOT and Trimming in the SDK will guard against regression here.

@agocke
Copy link
Member Author

agocke commented Jun 21, 2022

I'm assuming that the existing tests we have for both NativeAOT and Trimming in the SDK will guard against regression here.

Yup, I'm aiming for no functional change here.

@agocke agocke merged commit 2993803 into dotnet:main Jun 21, 2022
@agocke agocke deleted the move-illink-files branch June 21, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants