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

Update list of Swift runtime libraries to be de-duplicated, so the autolink-extract tool doesn't repeat them over and over again #76224

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

finagolfin
Copy link
Member

Followup to #76019 and #75262, cleaning up this list a bit more.

@grynspan, need a CI run here.

…tolink-extract tool doesn't repeat them over and over again
@jmschonfeld
Copy link
Contributor

@swift-ci please smoke test

// XCTest runtime libs (must be first due to http://github.com/apple/swift-corelibs-xctest/issues/432)
"-lXCTest",
// ICU Swift runtime libs
"-licui18nswift",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note - I did not remove ICU from the swift 6 toolchain, so if we do end up cherry picking this back to swift 6 we'll likely want to keep these in

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should go the other way and cherry-pick your #75262 also with these pulls for Swift 6.0. 😄

Those libraries are now entirely unused, with nothing linking to them:

> ag --search-binary libicu swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9
Binary file swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicui18nswift.so.69.1 matches.

Binary file swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicudataswift.so.69.1 matches.

Binary file swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicuucswift.so.69.1 matches.

and we can shave 35 MB, about 1%, off the size of the Swift toolchain by cleaning that out:

> du -sk swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicu*1
28004   swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicudataswift.so.69.1
4228    swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicui18nswift.so.69.1
2452    swift-6.0-DEVELOPMENT-SNAPSHOT-2024-08-22-a-ubi9/usr/lib/swift/linux/libicuucswift.so.69.1

@grynspan
Copy link
Contributor

grynspan commented Sep 3, 2024

@finagolfin Real question: does @swift-ci please test not do anything when you comment with it? If not, we can reach out to our colleagues and add you to whatever permission list allows for it.

@finagolfin
Copy link
Member Author

@grynspan, I have not applied to be a committer yet, 😄 plan to do so soon.

@finagolfin
Copy link
Member Author

Passed CI, ready for merge.

@jmschonfeld, I suggest that you bundle #76019, this pull, and #75262 together, after removing any changes to update-checkout-config.json in that last pull (as those ended up reverted), and submit to 6.0 and 6.0.0 next.

@finagolfin
Copy link
Member Author

@grynspan, mind merging? We should get this into Swift 6.0 next.

@jmschonfeld
Copy link
Contributor

Passed CI, ready for merge.

@jmschonfeld, I suggest that you bundle #76019, this pull, and #75262 together, after removing any changes to update-checkout-config.json in that last pull (as those ended up reverted), and submit to 6.0 and 6.0.0 next.

Merging seems fine, but we should probably get approval from @etcwilde or @MaxDesiatov as code owners first. As for cherry-picking back, we can cherry pick back to swift 6 but as removing ICU from the build is a large infrastructural change we determined at the time that it was only important to land it in main for the next release and not Swift 6, so I don't plan to cherry-pick that change back. In that case, you may need to adjust this PR while cherry-picking it back to keep the ICU libraries in on Swift 6

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.

Looks fine to me

@grynspan
Copy link
Contributor

grynspan commented Sep 9, 2024

I'll have to let my colleagues merge this PR. :)

@finagolfin
Copy link
Member Author

removing ICU from the build is a large infrastructural change we determined at the time that it was only important to land it in main for the next release and not Swift 6

The big change architecturally was merging the swift-foundation rewrite to Swift 6.0 so quickly after getting it into trunk 6.1, I was surprised when Tony told me that was the plan.

Your libicu pull is a minor cleanup by comparison and will not break anything now that swift-foundation-icu was added. I definitely think we should merge all three pulls I listed above to Swift 6.0.

What do you think, @etcwilde?

@etcwilde
Copy link
Contributor

etcwilde commented Sep 9, 2024

For 6.0.x, I agree that we should keep the existing ICU ordering and de-duplication. For the link itself, it means that if someone is pulling in those libraries, they avoid having it repeated, eating memory and link time. For folks who aren't, it's three extra string-compares, but no other impact on their link. So, I think that tradeoff is worth it for a version, or at least until we are no longer shipping the library as part of our SDK.

@finagolfin
Copy link
Member Author

at least until we are no longer shipping the library as part of our SDK.

@etcwilde, not sure if you saw my comment above, but I'm proposing removing libicu from Swift 6.0, as it has already been removed from trunk 6.1 and is no longer used anywhere by Swift 6.0, which has already switched to _FoundationICU instead.

@finagolfin
Copy link
Member Author

@jmschonfeld, any reason this hasn't been merged yet?

Also, will you submit these three pulls to the 6.0 branches or would you like me to? You wrote two out of the three pulls, so up to you.

@jmschonfeld jmschonfeld merged commit 8fe0fd1 into swiftlang:main Sep 16, 2024
3 checks passed
@jmschonfeld
Copy link
Contributor

Also, will you submit these three pulls to the 6.0 branches or would you like me to?

I can cherry-pick back my change to add Foundation's libraries to this list, but I think the ICU changes should be separate since removing ICU from the toolchain is a much larger change and I think its merits/risks should be considered separately. I'm not certain the benefits outweigh the risks currently as I haven't done due diligence to make sure that removing it in release/6.0 doesn't break anything, but if you have the time and would like to validate that and nominate it for release/6.0 you're welcome to post that cherry-pick and nominate it for consideration. Ultimately it's up the branch managers as to whether a change like that would be taken.

@finagolfin finagolfin deleted the link branch September 17, 2024 03:54
@grynspan
Copy link
Contributor

release/6.0 is dead, long live release/6.0.1!

finagolfin added a commit to finagolfin/swift that referenced this pull request Sep 20, 2024
…tolink-extract tool doesn't repeat them over and over again (swiftlang#76224)
finagolfin added a commit to finagolfin/swift that referenced this pull request Sep 20, 2024
…tolink-extract tool doesn't repeat them over and over again (swiftlang#76224)
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.

4 participants