[cmake] add Synchronization library to Supplemental build #81310
Conversation
justice-adams-apple
requested review from
etcwilde,
compnerd and
edymtt
as code owners
|
|
||
| set(CMAKE_POSITION_INDEPENDENT_CODE YES) | ||
|
|
||
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/../cmake/modules") |
There was a problem hiding this comment.
These can come after the project call.
| if(NOT PROJECT_IS_TOP_LEVEL) | ||
| message(SEND_ERROR "Swift Synchronization must build as a standalone project") | ||
| endif() |
There was a problem hiding this comment.
We should likely hoist this to right after the project call
| option(${PROJECT_NAME}_ARCH_SUBDIR "Architecture used for setting the architecture subdirectory" | ||
| ${SwiftCore_ARCH_SUBDIR}) | ||
|
|
||
| option(${PROJECT_NAME}_INSTALL_NESTED_SUBDIR "Install libraries under a platform and architecture subdirectory" | ||
| ${SwiftCore_INSTALL_NESTED_SUBDIR}) | ||
|
|
||
| option(${PROJECT_NAME}_PLATFORM_SUBDIR "Platform name used for installed swift{doc,module,interface} files" | ||
| ${SwiftCore_PLATFORM_SUBDIR}) | ||
|
|
||
| option(${PROJECT_NAME}_COMPILER_VARIANT_TARGET "Compiler variant target to use when emiting modules" | ||
| ${SwiftCore_COMPILER_VARIANT_TARGET}) |
There was a problem hiding this comment.
@etcwilde - whats your take on these defaults? I'm kinda somewhat torn. I don't like introducing more option values, but these feel like it might make sense to separate.
There was a problem hiding this comment.
I think inheriting the value for library evolution and prespecialization is probably fine. They're defaults so if someone really doesn't want them to match, passing the option directly, using a cmake cache, or modifying the value with ccmake later will all work. Whether or not to install into the nested subdirectory is also probably fine to default to match.
The arch and platform subdirs are computed by the compiler for the platform and they aren't boolean options, so they shouldn't be options. We don't need to inherit them, just use the value coming out of PlatformInfo. Maybe later, we could look to see if these values have all been set and avoid invoking the compiler to get this info since process launches are expensive on Windows, but I don't think it's worth worrying about now.
Lets drop the defaults for:
- platform subdirectory
- arch subdirectory
- compiler variant target
| $<$<PLATFORM_ID:Darwin>:Mutex/DarwinImpl.swift> | ||
| $<$<PLATFORM_ID:Linux>:Mutex/LinuxImpl.swift> | ||
| $<$<PLATFORM_ID:WASI>:Mutex/SpinLoopHint.swift> | ||
| $<$<PLATFORM_ID:WINDOWS>:Mutex/WindowsImpl.swift>) |
There was a problem hiding this comment.
Should we not keep the Sources directory structure?
There was a problem hiding this comment.
The synchronization library isn't laid out this way. StringProcessing is since it's also a SwiftPM package in the separate repo. For now, let's keep the layout of whatever the library was using. We can certainly revisit the filesystem layout once we've finished the migration and are no-longer syncing the sources. Maybe worth making a note or file an issue to make things consistent though.
| option(${PROJECT_NAME}_ARCH_SUBDIR "Architecture used for setting the architecture subdirectory" | ||
| ${SwiftCore_ARCH_SUBDIR}) | ||
|
|
||
| option(${PROJECT_NAME}_INSTALL_NESTED_SUBDIR "Install libraries under a platform and architecture subdirectory" | ||
| ${SwiftCore_INSTALL_NESTED_SUBDIR}) | ||
|
|
||
| option(${PROJECT_NAME}_PLATFORM_SUBDIR "Platform name used for installed swift{doc,module,interface} files" | ||
| ${SwiftCore_PLATFORM_SUBDIR}) | ||
|
|
||
| option(${PROJECT_NAME}_COMPILER_VARIANT_TARGET "Compiler variant target to use when emiting modules" | ||
| ${SwiftCore_COMPILER_VARIANT_TARGET}) |
There was a problem hiding this comment.
I think inheriting the value for library evolution and prespecialization is probably fine. They're defaults so if someone really doesn't want them to match, passing the option directly, using a cmake cache, or modifying the value with ccmake later will all work. Whether or not to install into the nested subdirectory is also probably fine to default to match.
The arch and platform subdirs are computed by the compiler for the platform and they aren't boolean options, so they shouldn't be options. We don't need to inherit them, just use the value coming out of PlatformInfo. Maybe later, we could look to see if these values have all been set and avoid invoking the compiler to get this info since process launches are expensive on Windows, but I don't think it's worth worrying about now.
Lets drop the defaults for:
- platform subdirectory
- arch subdirectory
- compiler variant target
| $<$<PLATFORM_ID:Darwin>:Mutex/DarwinImpl.swift> | ||
| $<$<PLATFORM_ID:Linux>:Mutex/LinuxImpl.swift> | ||
| $<$<PLATFORM_ID:WASI>:Mutex/SpinLoopHint.swift> | ||
| $<$<PLATFORM_ID:WINDOWS>:Mutex/WindowsImpl.swift>) |
There was a problem hiding this comment.
The synchronization library isn't laid out this way. StringProcessing is since it's also a SwiftPM package in the separate repo. For now, let's keep the layout of whatever the library was using. We can certainly revisit the filesystem layout once we've finished the migration and are no-longer syncing the sources. Maybe worth making a note or file an issue to make things consistent though.
|
|
||
| set_target_properties(swiftSynchronization PROPERTIES | ||
| Swift_MODULE_NAME Synchronization | ||
| Swift_COMPILATION_MODE wholemodule) |
There was a problem hiding this comment.
Is WMO required to build the synchronization library? It should be fine to set CMAKE_Swift_COMPILATION_MODE in a cache or as a flag. Incremental builds make iterating on the library faster.
| Swift_COMPILATION_MODE wholemodule) | ||
|
|
||
| if(APPLE AND BUILD_SHARED_LIBS) | ||
| target_link_options(swiftSynchronization PRIVATE "SHELL:-Xlinker -headerpad_max_install_names") |
There was a problem hiding this comment.
I think this is an Apple-ism that isn't needed externally. We can move it into the vendor files.
There was a problem hiding this comment.
I think that leaving this in an APPLE clause is fine. This is mostly a MachO thing (which has a similar issue on ELF). Changing the encoded values post-facto is difficult unless padding is included when linking.
Addresses rdar://150300769
justice-adams-apple
force-pushed
the
add-synchronization
branch
from
a1c8166 to
ed8ed44
Compare
Addresses rdar://150300769
Add Synchronization library to new build system
Land #81215 first