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

Add new Foundation libraries to SwiftRuntimeLibsOrdered #76019

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

jmschonfeld
Copy link
Contributor

Now that Foundation has added a handful of new libraries to the build that are linked from Foundation itself, we're seeing a handful of build logs that contain contents like the following:

... -lFoundationInternationalization -lFoundationEssentials -lFoundationInternationalization -lFoundationEssentials -lstdc++ -lswiftCxx -lswiftCxxStdlib -lstdc++ -lswiftCxx -lswiftCxxStdlib -lstdc++ -lswiftCxx -lswiftCxxStdlib -lstdc++ -lswiftCxx -lswiftCxxStdlib -lFoundationInternationalization -lFoundationEssentials -lFoundationInternationalization -lFoundationEssentials -lFoundationInternationalization -lFoundationEssentials -lswift_Differentiation -lFoundationInternationalization -lFoundationEssentials -lFoundationInternationalization -lFoundationEssentials -lFoundationInternationalization -lFoundationEssentials -lFoundationInternationalization -lFoundationEssentials ...

Per swiftlang/swift-foundation#872, this list seems to be used to deduplicate linked libraries for common libraries. This change adds the new Foundation dynamic libraries (FoundationEssentials, FoundationInternationalization, and _FoundationICU) along with swiftSynchronization (which Foundation also links now) to this list for deduplication to help minimize the issues caused by these repeated link flags.

(I'm not extremely familiar with this list, but it seems that since Foundation is already there we should add the new Foundation libraries, however feel free to let me know if we shouldn't do this or if there's a different change that should be made)

@jmschonfeld
Copy link
Contributor Author

@swift-ci please smoke test

@finagolfin
Copy link
Member

I think you can remove the icu libs now: those are no longer built or shipped separately.

@@ -231,15 +231,19 @@ int autolink_extract_main(ArrayRef<const char *> Args, const char *Argv0,
"-lswift_RegexBuilder",
Copy link
Member

Choose a reason for hiding this comment

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

This is now swiftRegexBuilder.

@finagolfin
Copy link
Member

finagolfin commented Aug 21, 2024

As long as we're here, might as well update this list for 6.1. Some more candidates for addition: swift_Builtin_float, swift_Differentiation, swiftDistributed, swiftObservation, swift_Volatile, Testing.

@stephentyrone, @ktoso, @grynspan, @al45tair, wdyt, should we add any of those?

@grynspan
Copy link
Contributor

I honestly don't know. :)

@finagolfin
Copy link
Member

@grynspan, try this: find a large project with many Testing test files, then run swift build --build-tests -v on linux. Does the last link command building the test runner have one -lTesting or many?

@grynspan
Copy link
Contributor

Ah, yes, one per file. So sounds like we should add Testing as you've suggested.

@finagolfin
Copy link
Member

Pinging @gwynne and @compnerd too, in case you have any input.

@finagolfin
Copy link
Member

finagolfin commented Sep 3, 2024

@al45tair and @FranzBusch, mind pushing this change through to completion and into Swift 6? We are going to see a regression with a ton of repeated linker flags on linux and other Unix platforms otherwise.

It looks like everybody went on summer vacation in the last couple weeks when this was proposed, but with the Swift 6 release likely coming in the next month, we should get this in.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Ouch, this is a great catch! Definitely something to consider pulling into 6.0.

CC: @parkera

@finagolfin
Copy link
Member

Thanks, @compnerd, do you have an opinion on which other of the new runtime libraries listed above should also be de-duplicated?

@compnerd
Copy link
Member

compnerd commented Sep 3, 2024

I'd say that Testing is a good idea to add to the list, the other ones don't seem as important.

@finagolfin
Copy link
Member

Alright, if someone would go ahead and merge, @jmschonfeld doesn't seem to be active lately, so I went ahead and submitted #76224 with my remaining suggestions for this pull, so we can get those in separately.

@jmschonfeld
Copy link
Contributor Author

Sorry for the delay @finagolfin! I'll go ahead and merge this one, thanks for the followup PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't add -lFoundationEssentials -lFoundationInternationalization repeatedly when linking many object files
6 participants