Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Split off system libraries into a separate ItemGroup #89916

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

MichalStrehovsky
Copy link
Member

Should help with #88294 (comment).

We could try to put more arguments on the ExtraLinkerArg plan but I wanted to keep the diff short and reviewable for now.

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented Aug 3, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Should help with #88294 (comment).

We could try to put more arguments on the ExtraLinkerArg plan but I wanted to keep the diff short and reviewable for now.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@rolfbjarne rolfbjarne requested a review from filipnavara August 3, 2023 09:19
Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines 141 to 143
<LinkerArg Include="-L/usr/lib/swift" Condition="'$(_targetOS)' == 'osx'" />
<LinkerArg Include="@(StaticICULibs)" Condition="'$(StaticICULinking)' == 'true'" />
<LinkerArg Include="@(StaticSslLibs)" Condition="'$(StaticOpenSslLinking)' == 'true'" />
Copy link
Member

Choose a reason for hiding this comment

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

We can move these three as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The first one is capital L so it doesn't feel like it fits either of the new ItemGroups. Would need a NativeSystemLibraryPath

Breaking up the static ICU/SSL weirdness would be more diffs so it's the same reasoning as "We could try to put more arguments on the ExtraLinkerArg plan but I wanted to keep the diff short and reviewable for now."

@MichalStrehovsky MichalStrehovsky merged commit bd471dc into dotnet:main Aug 3, 2023
@MichalStrehovsky MichalStrehovsky deleted the split branch August 3, 2023 21:19
@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants