Skip to content

Serialization: avoid pushing static libraries into autolink #63814

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Serialization: avoid pushing static libraries into autolink #63814

wants to merge 1 commit into from

Conversation

compnerd
Copy link
Member

Remove the static libraries from autolinking. This should be safe to do with SPM builds which does not respect the static linking and instead treats all targets as object libraries for static linking. With CMake this should also not be a problem as it already requires dependencies be spelt out and then correctly forms the linker invocation. This prevents us from double linking a library (both into a potential intermediate shared library and any consumers of any public interfaces).

@compnerd
Serialization: avoid pushing static libraries into autolink
93a7555 
Remove the static libraries from autolinking.  This should be safe to do
with SPM builds which does not respect the static linking and instead
treats all targets as object libraries for static linking.  With CMake
this should also not be a problem as it already requires dependencies be
spelt out and then correctly forms the linker invocation.  This prevents
us from double linking a library (both into a potential intermediate
shared library and any consumers of any public interfaces).
@compnerd compnerd marked this pull request as ready for review February 21, 2023 23:17
@compnerd
Copy link
Member Author

CC: @bnbarham @etcwilde @neonichu

@compnerd compnerd marked this pull request as draft February 22, 2023 01:34
@compnerd
Copy link
Member Author

This exposes an interesting issue that I was afraid of. There are symbols that may be part of a public interface that cannot be fully internalised.

@compnerd
Copy link
Member Author

@swift-ci please build toolchain Windows platform

@compnerd
Copy link
Member Author

Please test with following PRs:
apple/swift-argument-parser#579

@swift-ci please build toolchain Windows platform

@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-package-manager#6815
apple/swift-argument-parser#579

@swift-ci please build toolchain Windows platform

@compnerd compnerd marked this pull request as ready for review August 15, 2023 16:13
@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/swift-package-manager#6815
apple/swift-argument-parser#579

@swift-ci please test

@compnerd
Copy link
Member Author

@swift-ci please test Linux platform

@compnerd
Copy link
Member Author

@swift-ci please test

artemcm
artemcm approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@artemcm artemcm left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

etcwilde
etcwilde approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@etcwilde etcwilde left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@compnerd
Copy link
Member Author

@swift-ci please test Windows platform

@compnerd
Copy link
Member Author

@swift-ci please test Linux platform

1 similar comment
@compnerd
Copy link
Member Author

@swift-ci please test Linux platform

@compnerd
Copy link
Member Author

compnerd commented Dec 7, 2023

@swift-ci please test

@compnerd
Copy link
Member Author

@swift-ci please test Linux platform

@compnerd compnerd mentioned this pull request Sep 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etcwilde etcwilde etcwilde approved these changes

@artemcm artemcm artemcm approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@compnerd @etcwilde @artemcm