Introduce CMake support for SwiftFoundationICU #22

iCharlesHu · 2024-04-24T15:53:43Z

Add Cmake support for SwiftFoundationICU which includes:

CMakeLists.txt in major directories
module.modulemap for each include directory (these files will be automatically generated during export)

Unfortunately we also had to merge all ICU modules into one as part of this change.

iCharlesHu · 2024-04-30T21:34:17Z

@swift-ci build toolchain

etcwilde

Alright, that should be enough of an initial review.

The layout of the include directories are confusing me a bit and I'm not sure what the goal is. They're labelled as not propagating to things that depend on the target, but then you turn around and target_include_directories across the directories to get to them again, so they're still treated as PUBLIC, but weirdly. If you're trying to prevent them from being exposed if someone installed the library, we can certainly do that.

These two are probably what you're looking for?

$<BUILD_INTERFACE: --directory-- >
$<INSTALL_INTERFACE: --directory-- >

So for ICUCommon, I think it might look something like this:

target_include_directories(ICUCommon
  PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
    $<BUILD_INTERFACE:unicode>
    include/)

CMakeLists.txt

icuSources/i18n/include/unicode/module.modulemap

icuSources/io/CMakeLists.txt

icuSources/stubdata/CMakeLists.txt

etcwilde · 2024-05-03T05:30:29Z

I see one of your questions on the email notification, but not in the GitHub UI, so I'll answer it here.

These are the shared settings across different targets. Should I repeat the same settings in each target?

CMake scoping rules are kind of strange to get used to at first. The directory structure defines scopes. Things set add_compile_definition, add_compile_options, etc... are applied to all targets defined in CMake files under add_subdirectory calls made after the add_compile_definitions/add_compile_options call. It's a scripting language too though, so the order of the add_compile_definitions and add_subdirectory calls does matter.

etcwilde

This is looking much better. Thank you for making those changes. Still a few small comments, but this should work.

I think we can wait on the install story for now and add one later if necessary.

etcwilde · 2024-05-03T05:11:31Z

CMakeLists.txt

+##
+##===----------------------------------------------------------------------===##
+
+cmake_minimum_required(VERSION 3.24)


Is this for CI? It might be worth trying this build across a few versions of CMake and doing something like this

cmake_minimum_required(VERSION 3.24...3.29)

That way we pick up CMP0156 and CMP0157. We definitely want these two if folks have a new enough CMake since it will fix the linker warning about duplicate libraries and will result in faster incremental builds due to the better Swift build graphs in CMake 3.29.

At the very least, between the cmake_minimum_required and project invocations, can you enable those policies? Something like this:

cmake_minimum_required(VERSION 3.24) if(POLICY CMP0156) # Deduplicate linked libraries where appropriate cmake_policy(SET CMP0156 NEW) endif() if(POLICY CMP0157) # New Swift build model: improved incremental build performance and LSP support cmake_policy(SET CMP0157 NEW) endif() project(SwiftFoundationICU LANGUAGES CXX Swift) ...

etcwilde · 2024-05-03T05:13:26Z

CMakeLists.txt

+set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
+set(CMAKE_Swift_MODULE_DIRECTORY ${CMAKE_BINARY_DIR}/swift)
+
+option(BUILD_SHARED_LIBS ON)


Don't need this bit. CMake knows about BUILD_SHARED_LIBS already and will implicitly set any add_library that doesn't specify SHARED or STATIC with the selection there.

https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

I think that this is trying to build as shared libraries by default.

Okay, then it's fine to leave this line. Don't need 30 - 34 though. Those are handled implicitly by add_library if the library type isn't specified.

etcwilde · 2024-05-03T05:14:25Z

swift/CMakeLists.txt

+##===----------------------------------------------------------------------===##
+
+file(GLOB_RECURSE _FoundationICUSources "FoundationICU/*.swift")
+add_library(FoundationICU ${LIB_TYPE} ${_FoundationICUSources})


Don't need ${LIB_TYPE} here. BUILD_SHARED_LIBS will do that for you.

etcwilde · 2024-05-03T05:22:08Z

cmake/modules/CMakeLists.txt

+    ${CMAKE_CURRENT_BINARY_DIR}/SwiftFoundationICUConfig.cmake)
+
+get_property(SWIFT_FOUNDATION_ICU_EXPORTS GLOBAL PROPERTY SWIFT_FOUNDATION_ICU_EXPORTS)
+export(TARGETS ${SWIFT_FOUNDATION_ICU_EXPORTS}


Okay, I think we can leave the install story up in the air for the time being. If we need to figure it out, we can come up with how that should work later.

etcwilde · 2024-05-03T05:23:52Z

CMakeLists.txt

+
+add_subdirectory(icuSources)
+add_subdirectory(swift)
+add_subdirectory(cmake/modules)


Eh, we can leave it for now.

etcwilde

Ah, missed a few things. Just based on the comment made above that we only want to expose FoundationICU, we should explicitly specify that the other libraries are STATIC so that BUILD_SHARED_LIBS doesn't turn them into dynamic libraries. That said, if BUILD_SHARED_LIBS is FALSE, we'll still need the static archives available for linking, but like I say, we can defer on the install story for now.

etcwilde · 2024-05-03T05:43:27Z

icuSources/common/CMakeLists.txt

+##===----------------------------------------------------------------------===##
+
+file(GLOB_RECURSE _ICUCommonSources "*.cpp")
+add_library(ICUCommon ${_ICUCommonSources})


Make this STATIC

etcwilde · 2024-05-03T05:43:45Z

icuSources/i18n/CMakeLists.txt

+##===----------------------------------------------------------------------===##
+
+file(GLOB_RECURSE _ICUI18NSources "*.cpp")
+add_library(ICUI18N ${_ICUI18NSources})


make this STATIC

etcwilde · 2024-05-03T05:43:59Z

icuSources/io/CMakeLists.txt

+##===----------------------------------------------------------------------===##
+
+file(GLOB_RECURSE _ICUIOSources "*.cpp")
+add_library(ICUIO ${_ICUIOSources})


Make this STATIC

etcwilde · 2024-05-03T05:44:07Z

icuSources/stubdata/CMakeLists.txt

+##===----------------------------------------------------------------------===##
+
+file(GLOB_RECURSE _ICUStubDataSources "*.cpp")
+add_library(ICUStubData ${_ICUStubDataSources})


Make this STATIC

etcwilde · 2024-05-04T20:50:46Z

icuSources/common/CMakeLists.txt

+        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/unicode/>
+        include/)
+
+target_link_libraries(ICUCommon PRIVATE stdc++)


This shouldn't be necessary. From chatting with @compnerd, we might want to enable C++ interop so that the linker-driver is clang++ instead of just clang so that it implicitly links the C++ standard library.

Yes, if we enable the c++ interop, we should get the c++ driver as the linker driver, ensuring that we get the c++ runtime linked properly. Additionally, users can just specify -Xclang-linker -stdlib=... to get the desired c++ runtime.

No, Swift doesn't use clang++ as the linker driver when C++-interop is enabled, just clang (except in Swift 5.9):
https://github.com/apple/swift-driver/blob/b0300a86653daf63466a47841983539b47bf7cde/Sources/SwiftDriver/Jobs/GenericUnixToolchain%2BLinkerSupport.swift#L62-L97

icuSources/i18n/CMakeLists.txt

jmschonfeld

This LGTM now but since I helped a lot with this I'll leave it to the others for final approval 🙂

icuSources/CMakeLists.txt

etcwilde · 2024-06-07T23:30:58Z

icuSources/CMakeLists.txt

+    PUBLIC
+        include/)
+
+target_link_libraries(_FoundationICU PRIVATE stdc++)


This is an odd one since we have to guess what flavor of C++ folks are using since the Swift driver decided to not help us out and tell us what it's using (actually, I think part of the problem is that it doesn't know what flavor of C++ we're using either leaving it as an exercise to the user to pass all the right flags).
I know this isn't going to work for Android (uses libcxx) nor for Windows (which can use the Windows C++ runtime, but I think libcxx might also work there too so it could go either way). I think it should be customizable at distribution time since the person putting together the SDK will likely have the best chance of knowing which C++ runtime they're building the SDK with. At least until the Swift compiler learns how to specify what C++ runtime you're working with.

In theory, we could do something like this to provide reasonable defaults:

if(Linux) set(SwiftFoundation_CXX_RUNTIME_default stdc++) elseif(Android) set(SwiftFoundation_CXX_RUNTIME_default c++) elseif(WIN32) set(SwiftFoundation_CXX_RUNTIME_default WhateverWindowsUses) endif() set(SwiftFoundation_CXX_RUNTIME "${SwiftFoundation_CXX_RUNTIME_default}" CACHE STRING "C++ runtime that SwiftFoundation is built against")

@compnerd, do you have a better idea?

Why not leave it to the user? If we set the linker language to C++, the C++ driver will be used. The user can add in -stdlib=libc++ or -stdlib=libstdc++ if they are using clang. If they are using cl, that will default to the MS STL, as would clang-cl.

This is related to using c++ interop at link time. The one thing that we should be careful of is if that gets serialized into the module.

It turns out we don't actually need it... I deleted it.

CMakeLists.txt

iCharlesHu · 2024-06-10T23:04:57Z

Addressed @etcwilde 's comments.

etcwilde · 2024-06-11T15:28:01Z

icuSources/CMakeLists.txt

+add_library(_FoundationICU)
+
+target_include_directories(_FoundationICU
+    PRIVATE


Since we're adding each subdirectory as an include directory with target_include_directories(_FoundationICU PRIVATE .), we shouldn't need any of these PRIVATE include directories here either.

etcwilde

Alright, I think this looks good enough structurally.
One thing I noticed is that it looks like stubdata is not part of any target.
Is that intentional?

jmschonfeld · 2024-06-13T23:16:02Z

One thing I noticed is that it looks like stubdata is not part of any target.
Is that intentional?

Yes that's intentional - the stub data is part of the icu project and part of the sources that get copied over into this repo, but it is not used because we provide data that is built directly into the binary. The stub data is only necessary to build a version of ICU without any data files. We can probably remove that in a future PR

iCharlesHu force-pushed the charles/cmake-support branch from 2643f35 to d91484a Compare April 24, 2024 17:29

iCharlesHu marked this pull request as ready for review April 24, 2024 17:33

iCharlesHu force-pushed the charles/cmake-support branch from 104b73f to 847a8ad Compare April 30, 2024 21:33

etcwilde requested changes May 2, 2024

View reviewed changes

jmschonfeld mentioned this pull request May 3, 2024

[WIP] Add swift-foundation to toolchain build swiftlang/swift#73403

Closed

etcwilde reviewed May 3, 2024

View reviewed changes

etcwilde reviewed May 4, 2024

View reviewed changes

iCharlesHu commented May 5, 2024

View reviewed changes

icuSources/i18n/CMakeLists.txt Outdated Show resolved Hide resolved

iCharlesHu force-pushed the charles/cmake-support branch 2 times, most recently from 745992d to 9852007 Compare May 31, 2024 21:01

iCharlesHu force-pushed the charles/cmake-support branch 2 times, most recently from 1f40e48 to 67312f3 Compare June 7, 2024 22:51

jmschonfeld requested a review from etcwilde June 7, 2024 22:55

jmschonfeld approved these changes Jun 7, 2024

View reviewed changes

etcwilde reviewed Jun 7, 2024

View reviewed changes

etcwilde reviewed Jun 11, 2024

View reviewed changes

iCharlesHu force-pushed the charles/cmake-support branch from b0600af to 2dea4ba Compare June 11, 2024 23:46

jmschonfeld mentioned this pull request Jun 13, 2024

[DNM] Test Toolchain Build swiftlang/swift#74344

Closed

etcwilde approved these changes Jun 13, 2024

View reviewed changes

Introduce CMake support for SwiftFoundationICU

3122441

iCharlesHu force-pushed the charles/cmake-support branch from 2dea4ba to 3122441 Compare June 14, 2024 16:51

iCharlesHu merged commit 2ba3887 into swiftlang:main Jun 14, 2024

iCharlesHu deleted the charles/cmake-support branch June 14, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CMake support for SwiftFoundationICU #22

Introduce CMake support for SwiftFoundationICU #22

iCharlesHu commented Apr 24, 2024 •

edited

Loading

iCharlesHu commented Apr 30, 2024

etcwilde left a comment

etcwilde commented May 3, 2024

etcwilde left a comment

etcwilde May 3, 2024

etcwilde May 3, 2024

compnerd May 3, 2024

etcwilde May 3, 2024

etcwilde May 3, 2024

etcwilde May 3, 2024

etcwilde May 3, 2024

etcwilde left a comment

etcwilde May 3, 2024

etcwilde May 3, 2024

etcwilde May 3, 2024

etcwilde May 3, 2024

etcwilde May 4, 2024

compnerd Jun 8, 2024

etcwilde Jun 9, 2024 •

edited

Loading

jmschonfeld left a comment

etcwilde Jun 7, 2024

compnerd Jun 8, 2024

compnerd Jun 8, 2024

iCharlesHu Jun 10, 2024 •

edited

Loading

iCharlesHu commented Jun 10, 2024

etcwilde Jun 11, 2024

etcwilde left a comment

jmschonfeld commented Jun 13, 2024

Introduce CMake support for SwiftFoundationICU #22

Introduce CMake support for SwiftFoundationICU #22

Conversation

iCharlesHu commented Apr 24, 2024 • edited Loading

iCharlesHu commented Apr 30, 2024

etcwilde left a comment

Choose a reason for hiding this comment

etcwilde commented May 3, 2024

etcwilde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etcwilde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etcwilde Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

jmschonfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iCharlesHu Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

iCharlesHu commented Jun 10, 2024

Choose a reason for hiding this comment

etcwilde left a comment

Choose a reason for hiding this comment

jmschonfeld commented Jun 13, 2024

iCharlesHu commented Apr 24, 2024 •

edited

Loading

etcwilde Jun 9, 2024 •

edited

Loading

iCharlesHu Jun 10, 2024 •

edited

Loading