Skip to content

[Frontend] Add -public-autolink-library option #35936

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 15, 2021
Merged

[Frontend] Add -public-autolink-library option #35936

merged 1 commit into from
Jun 15, 2021

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Feb 12, 2021 •
edited by MaxDesiatov
Loading

Foundation imports CoreFoundation with @_implementationOnly,
so CoreFoundation's modulemap won't be read, and the dependent libraries
of CoreFoundation will not be automatically linked when using static
linking.

For example, CoreFoundation depends on libicui18n and it's modulemap has
link "icui18n" statement. If Foundation imports CoreFoundation with
@_implementationOnly as a private dependency, the toolchain doesn't have
CoreFoundation's modulemap and Foundation's swiftmodule doesn't import
CoreFoundation. So the swiftc can't know that libicui18n is required.

This new option will add LINK_LIBRARY entry in swiftmodule to
specify dependent libraries (in the example case, Foundation's
swiftmodule should have LINK_LIBRARY entry of libicui18n)

See also: Autolinking behavior of @_implementationOnly with static linking

CC: @compnerd

@kateinoigakukun kateinoigakukun force-pushed the katei/public-autolink-library branch from 5439d2b to bf8780b Compare February 13, 2021 03:35
@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov requested a review from compnerd February 15, 2021 15:48
@kateinoigakukun kateinoigakukun force-pushed the katei/public-autolink-library branch from bf8780b to 1a92574 Compare February 17, 2021 06:02
@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@finagolfin
Copy link
Member

I've had the same issue on Android for the last couple months.

@finagolfin
Copy link
Member

To be clear, the issue for me is that once CoreFoundation went @_implementationOnly on Android too, the CoreFoundation and uuid static libraries are no longer linked against from the module map, breaking linking against Foundation statically. Since the plan is to do the same on linux one day, there's going to need to be a solution for static linking against libCoreFoundation.a on all non-Darwin platforms eventually.

@drexin or @spevans, could one of you provide direction on this?

@kateinoigakukun kateinoigakukun mentioned this pull request Apr 27, 2021
Merged
@kateinoigakukun kateinoigakukun force-pushed the katei/public-autolink-library branch from 1a92574 to 14266fa Compare April 27, 2021 14:42
kateinoigakukun added a commit to kateinoigakukun/swift-corelibs-foundation that referenced this pull request Apr 27, 2021
@kateinoigakukun
Merge private dependencies into single static library
fc0c1c8 
`Foundation` imports `CoreFoundation` with `@_implementationOnly` as a
private dependency, and it doesn't emit `IMPORTED_MODULE` in
swiftmodule.
Users of Foundation can link `libCoreFoundation.so` automatically when
using dynamic linking because it's already linked into
`libFoundation.so`.
However, this automatic linking doesn't work when static linking, so
users need to link CoreFoundation and dependent libraries of
CoreFoundation explicitly by adding `-lCoreFoundation -lBlocksRuntime
-licui18n`.

To avoid forcing users to link private dependencies explicitly, this
patch changed to merge the dependencies into libFoundation.a. And also
to avoid forcing to link dependent libs of CoreFoundation like `curl` or
`icui18n`, add `LINK_LIBRARY` entry in Foundation.swiftmodule using
`-public-autolink-library` which is introduced by
swiftlang/swift#35936
kateinoigakukun added a commit to kateinoigakukun/swift-corelibs-foundation that referenced this pull request Apr 29, 2021
@kateinoigakukun
Merge private dependencies into single static library
f409d1f 
`Foundation` imports `CoreFoundation` with `@_implementationOnly` as a
private dependency, and it doesn't emit `IMPORTED_MODULE` in
swiftmodule.
Users of Foundation can link `libCoreFoundation.so` automatically when
using dynamic linking because it's already linked into
`libFoundation.so`.
However, this automatic linking doesn't work when static linking, so
users need to link CoreFoundation and dependent libraries of
CoreFoundation explicitly by adding `-lCoreFoundation -lBlocksRuntime
-licui18n`.

To avoid forcing users to link private dependencies explicitly, this
patch changed to merge the dependencies into libFoundation.a. And also
to avoid forcing to link dependent libs of CoreFoundation like `curl` or
`icui18n`, add `LINK_LIBRARY` entry in Foundation.swiftmodule using
`-public-autolink-library` which is introduced by
swiftlang/swift#35936
@kateinoigakukun kateinoigakukun mentioned this pull request Apr 29, 2021
Merged
@kateinoigakukun
Copy link
Member Author

@compnerd Could you take a look at this change? This feature is required for swiftlang/swift-corelibs-foundation#2996

@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@spevans
Copy link
Contributor

spevans commented Apr 29, 2021

Please test with following PR:
swiftlang/swift-corelibs-foundation#2996
swiftlang/swift-integration-tests#87

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 14266fa9ae9198d882961687bc879d291b37dd5d

@kateinoigakukun
Copy link
Member Author

Could you re-run the CI again? 🙏

@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@kateinoigakukun
Copy link
Member Author

@MaxDesiatov Thanks:) Could you re-trigger with PRs links in please test comment to run cross-repo test?

@MaxDesiatov

This comment has been minimized.

Sign in to view
1 similar comment
@MaxDesiatov

This comment has been minimized.

Sign in to view
@MaxDesiatov

This comment has been minimized.

Sign in to view
@MaxDesiatov

This comment has been minimized.

Sign in to view
@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented May 1, 2021 •
edited
Loading

Hmm, ci bot is something wrong ? the GitHub PR status has not been updated 🤔

@MaxDesiatov
Copy link
Contributor

Please test with following pull request:
swiftlang/swift-corelibs-foundation#2996

@swift-ci Please test

@kateinoigakukun
Copy link
Member Author

@compnerd Gentle ping :)

And CI status is not updated, but they have been succeeded.

@MaxDesiatov

This comment has been minimized.

Sign in to view
1 similar comment
@MaxDesiatov
Copy link
Contributor

@swift-ci please test Windows platform

@kateinoigakukun
Copy link
Member Author

Hi, @compnerd :) Could you take a look?

@MaxDesiatov MaxDesiatov requested a review from CodaFi June 2, 2021 08:02
@kateinoigakukun kateinoigakukun force-pushed the katei/public-autolink-library branch from 14266fa to 8ce7eea Compare June 2, 2021 09:49
@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

CodaFi
CodaFi approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

This is an acceptable solution to an extremely tricky autolinking problem that I imagine we'll encounter in the future in even more exotic configurations than just the core libs.

@kateinoigakukun @MaxDesiatov
[Frontend] Add -public-autolink-library option
831facd 
Foundation imports CoreFoundation with @_implementationOnly,
so CoreFoundation's modulemap won't be read, and the dependent libraries
of CoreFoundation will not be automatically linked when using static
linking.

For example, CoreFoundation depends on libicui18n and it's modulemap has
`link "icui18n"` statement. If Foundation imports CoreFoundation with
@_implementationOnly as a private dependency, the toolchain doesn't have
CoreFoundation's modulemap and Foundation's swiftmodule  doesn't import
CoreFoundation. So the swiftc can't know that libicui18n is required.

This new option will add LINK_LIBRARY entry in swiftmodule to
specify dependent libraries (in the example case, Foundation's
swiftmodule should have LINK_LIBRARY entry of libicui18n)
@MaxDesiatov MaxDesiatov force-pushed the katei/public-autolink-library branch from 8ce7eea to 831facd Compare June 14, 2021 20:34
@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov merged commit 6362a98 into swiftlang:main Jun 15, 2021
@MaxDesiatov MaxDesiatov mentioned this pull request Aug 8, 2021
Closed
MaxDesiatov pushed a commit to swiftlang/swift-integration-tests that referenced this pull request Oct 2, 2023
@kateinoigakukun
Add integration tests for static-linking of Foundation (#87)
bc6ce67 
Implementation: swiftlang/swift-corelibs-foundation#2996 and swiftlang/swift#35936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith keith keith left review comments

@beccadax beccadax beccadax left review comments

@CodaFi CodaFi CodaFi approved these changes

@compnerd compnerd Awaiting requested review from compnerd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kateinoigakukun @MaxDesiatov @finagolfin @spevans @swift-ci @keith @beccadax @CodaFi