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.

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

Get Swift-Foundation building for Musl/Static SDK #838

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Get Swift-Foundation building for Musl/Static SDK #838

merged 2 commits into from
Aug 16, 2024

Conversation

etcwilde
Copy link
Contributor

@etcwilde etcwilde commented Aug 8, 2024

This adds the missing imports to get Swift-Foundation building against Musl so we can get the static SDK going again.

compnerd
compnerd reviewed Aug 8, 2024
View reviewed changes
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.

I'd really rather that we just merge #714 rather than do the workaround that you are doing for the macros.

@etcwilde
Copy link
Contributor Author

etcwilde commented Aug 8, 2024

We can do that, but since they aren't used in the build, we don't need to build them at all.

@jmschonfeld
Copy link
Contributor

We can do that, but since they aren't used in the build, we don't need to build them at all.

*yet - eventually the macro will be needed to build Foundation itself, so this isn't an option that we can expect to be long-lived if we did add it

@etcwilde
Copy link
Contributor Author

etcwilde commented Aug 8, 2024

The install/deployment story for macros is still very hazy and will likely need to change, but this is for 6.0.
Without changing SPM and the compiler, the only way to get them to work for 6.0 is for the compiler to pull the macro from the toolchain side of things rather than in the SDK itself. This is safe enough because the static SDK only works with an aligned toolchain, so the macro content in the toolchain and the SDK content in the static SDK can't be out-of-sync and still be a viable configuration.

Unless you're planning on making them required to build Swift-Foundation in the 6.0 release? In which case, yes, we should take the bits of #714 for ensuring that the macros are compiled for the machine doing the build rather than the machine that we're building for. At the same time, in that case, we should build the macro independently as a separate project and expose it through some SwiftFoundationMacroConfig.cmake file so that we can find_package it and re-use the same one across builds instead of rebuilding it for each architecture.

@jmschonfeld
Copy link
Contributor

but this is for 6.0.

Ah I missed that this is for 6.0 - normally for 6.0 work we merge to main and then cherry-pick back to 6.0 filling out the nomination template, could we follow that pattern here to be consistent? That will help us on the Foundation team out a lot with the tooling that we use to manage the repo. But yes we don't plan to require the macro to build Foundation itself in the swift 6 timeframe, so if we need to exclude the macro build from the static SDK due the the incompatibilities at the moment, that seems reasonable. Is that what the stdlib macros do today?

@etcwilde
Copy link
Contributor Author

etcwilde commented Aug 8, 2024

swiftlang/swift-corelibs-foundation#5060
@swift-ci please test

@etcwilde etcwilde mentioned this pull request Aug 8, 2024
Merged
iCharlesHu
iCharlesHu approved these changes Aug 9, 2024
View reviewed changes
Sources/CMakeLists.txt Outdated
if(NOT CMAKE_SYSTEM_NAME STREQUAL Windows)
add_subdirectory(FoundationMacros)
# Macros are only useful for development, but not at runtime.
option(SwiftFoundation_BUILD_MACROS "Include SwiftFoundation Macro Targets" ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this flag set to false by the build script on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, is now: swiftlang/swift#75814

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these changes will likely "conflict" with the changes the @compnerd has landed on main (and the ongoing work that I have a PR for). It'd be helpful to see the main PR for this work as well while reviewing this to see what our plan is for beyond just this release, where Macros are now built separately

@iCharlesHu
Copy link
Contributor

This change will need to be cherry-picked to release/6.0. Please tag @itingliu for review since she's the release manager for 6.0

@etcwilde etcwilde requested a review from itingliu August 9, 2024 02:41
@etcwilde etcwilde mentioned this pull request Aug 9, 2024
Closed
jmschonfeld
jmschonfeld reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@jmschonfeld jmschonfeld left a comment

Choose a reason for hiding this comment

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

One comment about handling macros on the main branch where we've redesigned how they're built, but otherwise I don't have any additional comments on this

Sources/CMakeLists.txt Outdated
if(NOT CMAKE_SYSTEM_NAME STREQUAL Windows)
add_subdirectory(FoundationMacros)
# Macros are only useful for development, but not at runtime.
option(SwiftFoundation_BUILD_MACROS "Include SwiftFoundation Macro Targets" ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these changes will likely "conflict" with the changes the @compnerd has landed on main (and the ongoing work that I have a PR for). It'd be helpful to see the main PR for this work as well while reviewing this to see what our plan is for beyond just this release, where Macros are now built separately

@etcwilde etcwilde mentioned this pull request Aug 14, 2024
Merged
itingliu
itingliu approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@itingliu itingliu left a comment

Choose a reason for hiding this comment

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

6.0 change looks fine to me

@etcwilde
Get FoundationEssentials building
1334166 
Adding the missing musl imports to get FoundationEssentials building for
the Swift static SDKs again.

Also providing an option to disable building the macros. The macros
aren't necessary for building the library and will not be run as part of
the static SDK. No need to bloat the SDK or build times further. For
Swift 6, the macros should be provided by the toolchain since the
toolchain and SDK are current revlocked due to swiftmodules.
@etcwilde
Get FoundationInternationalization building
062c415 
Adding the missing Musl imports to get FoundationInternationalization
building for the static SDK.
@etcwilde etcwilde force-pushed the ewilde/static-sdk-support branch from 118fa4d to 062c415 Compare August 15, 2024 22:37
@etcwilde
Copy link
Contributor Author

@swift-ci please test

@etcwilde etcwilde merged commit 07b7a5d into swiftlang:release/6.0 Aug 16, 2024
3 checks passed
@etcwilde etcwilde deleted the ewilde/static-sdk-support branch August 16, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd left review comments

@jmschonfeld jmschonfeld jmschonfeld left review comments

@al45tair al45tair al45tair approved these changes

@iCharlesHu iCharlesHu iCharlesHu approved these changes

@itingliu itingliu itingliu approved these changes

@shahmishal shahmishal Awaiting requested review from shahmishal

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

Successfully merging this pull request may close these issues.
6 participants
@etcwilde @jmschonfeld @iCharlesHu @compnerd @al45tair @itingliu