CMake: Install `_TestingInternals` in static library builds #651

kateinoigakukun · 2024-08-29T12:52:33Z

When building the Testing library as a static library (BUILD_SHARED_LIBS=FALSE), _TestingInternals is not included in the libTesting.a archive by default. (when building as a shared library, libTesting.so includes _TestingInternals objects). This causes the linker to complain about missing symbols, so we need to ship lib_TestingInternals.a and autolink it too.

This change is required to ship swift-testing as a part of Swift SDK for WebAssembly

Checklist:

Code and documentation should follow the style of the Style Guide.
If public symbols are renamed or modified, DocC references should be updated.

grynspan · 2024-08-29T12:54:33Z

@etcwilde @rintaro Please review.

rintaro · 2024-08-30T19:03:00Z

Sources/Testing/CMakeLists.txt

+  set_property(TARGET Testing PROPERTY STATIC_LIBRARY_OPTIONS
+    $<TARGET_OBJECTS:_TestingInternals>)


Is there a reasons not to use target_link_options(Testing $<TARGET_OBJECTS:_TestingInternals>)?

Note This command cannot be used to add options for static library targets, since they do not use a linker. To add archiver or MSVC librarian flags, see the STATIC_LIBRARY_OPTIONS target property.

https://cmake.org/cmake/help/latest/command/target_link_options.html

Yes, it's intentional. Options specified to target_link_options are passed only when linking but not archiving, so we need to use STATIC_LIBRARY_OPTIONS here.

etcwilde

I mean, this is kind of how static archives work, there should be a lib_TestingInternals.a that libTesting.a depends on.
You'll have to remove the dependency on _TestingInternals.a or you'll have duplicate symbols.
I think rather than going through the link options, you can actually list the objects as "sources" to the target, so

add_library(BlahBlah ...)

# ...

target_sources(BlahBlah PRIVATE $<TARGET_OBJECTS:_TestingInternals>)

which I think looks a bit cleaner than mucking with the properties directly.

Are you testing with older versions of CMake as well? I seem to recall the old build model having no idea how to deal with object files getting linked into a Swift target so it would just omit them.

@compnerd, do you have opinions here?

Edit:

Are you testing with older versions of CMake as well? I seem to recall the old build model having no idea how to deal with object files getting linked into a Swift target so it would just omit them.

Hmm, I suppose that's why you'd be passing them as link flags instead of sources.

grynspan · 2024-08-30T20:28:10Z

We definitely would prefer it not to be possible to directly link to _TestingInternals as it's not, as the name suggests, public. It's definitely not ABI- or API-stable and just houses our C++ gunk.

etcwilde · 2024-08-30T20:56:07Z

From the shared library perspective, that's fine, they have things like "visibility". But that's not really one of the facilities that static archives provide in general. Whether it's all in one archive or two, the symbols are still there for the poking. If people do poke at it, that's on them though.

TBH, we should probably update the minimum CMake version and take the sources that are currently going into lib_TestingInternals.a and shove them directly into libTesting.a. Then there is no _TestingInternals library to link against. The difference here is that we

grynspan · 2024-08-30T21:21:56Z

… is that we…?

etcwilde · 2024-08-30T22:21:23Z

That's likely me talking to future me. :)

compnerd · 2024-08-31T01:30:46Z

I think that I would prefer that we expose the internal library. The way that you build the library matters as that is encoded into the module and the static nature of the library (e.g. if _TestingInternals is static and Testing is dynamic, the modules would be isolated but then merged into a single library and _TestingInternals would be fully internalized).

kateinoigakukun · 2024-09-03T18:24:42Z

I like @compnerd's option as its simplicity and it does not require the new split build model in CMake.
Updated the change to do that.

etcwilde · 2024-09-06T20:36:32Z

cmake/modules/SwiftModuleInstallation.cmake

+#   type: The type of the library (STATIC_LIBRARY, SHARED_LIBRARY, or EXECUTABLE).
+#         Typically, the value of the TYPE target property.
+#   result_var_name: The name of the variable to set
+function(get_swift_install_lib_dir type result_var_name)


I kind of wonder if we should factor this out into its own module, sort of like GNUInstallDirs, and have it set a bunch of cached variables so that it's easier for distributors to override? Would be fine for a different PR though.

I'm not sure if we should bother. I'd really rather just sink this down into CMake itself and support install(TARGETS ...) properly for Swift targets.

Either way, the library installation directory of swift-testing is a bit different than the conventional one (not lib/swift/<os> but lib/swift/<os>/testing on darwin), so we need a project specific logic that cannot be shared across packages.

etcwilde · 2024-09-06T20:37:02Z

CC @compnerd

compnerd · 2024-09-06T21:02:33Z

cmake/modules/SwiftModuleInstallation.cmake

+#   type: The type of the library (STATIC_LIBRARY, SHARED_LIBRARY, or EXECUTABLE).
+#         Typically, the value of the TYPE target property.
+#   result_var_name: The name of the variable to set
+function(get_swift_install_lib_dir type result_var_name)


I'm not sure if we should bother. I'd really rather just sink this down into CMake itself and support install(TARGETS ...) properly for Swift targets.

cmake/modules/SwiftModuleInstallation.cmake

When building the Testing library as a static library (BUILD_SHARED_LIBS=FALSE), `_TestingInternals` was not being installed alongside the main library. This caused the missing symbol error when linking against the Testing library statically.

kateinoigakukun · 2024-09-08T04:23:18Z

@swift-ci test

@grynspan

- **Explanation**: Switch to the official [600.0.0 tag](https://github.com/swiftlang/swift-syntax/releases/tag/600.0.0) for swift-syntax, which was recently created. - **Original PR**: [#651 https://github.com/swiftlang/swift-testing/pull/672](https://github.com/swiftlang/swift-testing/pull/690) - **Risk**: Low - **Testing**: Tested in ci.swift.org - **Reviewer**: @grynspan @briancroom

@grynspan

- **Explanation**: Switch to the official [600.0.0 tag](https://github.com/swiftlang/swift-syntax/releases/tag/600.0.0) for swift-syntax, which was recently created. - **Original PR**: [#651 https://github.com/swiftlang/swift-testing/pull/672](https://github.com/swiftlang/swift-testing/pull/690) - **Risk**: Low - **Testing**: Tested in ci.swift.org - **Reviewer**: @grynspan @briancroom

@grynspan

- **Explanation**: When building the Testing library as a static library (BUILD_SHARED_LIBS=FALSE), `_TestingInternals` is not included in the `libTesting.a` archive by default. (when building as a shared library, `libTesting.so` includes `_TestingInternals` objects). This causes the linker to complain about missing symbols, so we need to ship `lib_TestingInternals.a` and autolink it too. - **Original PR**: #651 #672 - **Risk**: Low; NFC for non WASI platforms - **Testing**: Tested in ci.swift.org - **Reviewer**: @grynspan @briancroom

kateinoigakukun requested review from stmontgomery, grynspan, dennisweissmann, briancroom, SeanROlszewski and suzannaratcliff as code owners August 29, 2024 12:52

grynspan assigned kateinoigakukun Aug 29, 2024

grynspan added wasi/wasm build swift-6.1 labels Aug 29, 2024

This was referenced Aug 29, 2024

Include swift-testing in the Wasm Swift SDK swiftlang/swift#76147

Merged

Build swift-testing swiftwasm/swiftwasm-build#421

Merged

MaxDesiatov requested review from rintaro, etcwilde, xedin and bnbarham August 30, 2024 11:03

rintaro reviewed Aug 30, 2024

View reviewed changes

etcwilde reviewed Aug 30, 2024

View reviewed changes

kateinoigakukun force-pushed the yt/bundle-cpp-objs-static branch from a4a69d9 to fa7c0f3 Compare September 3, 2024 18:22

kateinoigakukun changed the title ~~CMake: Merge private dependencies into single static objects archive~~ CMake: Install _TestingInternals in static library builds Sep 3, 2024

MaxDesiatov requested review from etcwilde and rintaro September 6, 2024 20:16

etcwilde approved these changes Sep 6, 2024

View reviewed changes

compnerd approved these changes Sep 6, 2024

View reviewed changes

kateinoigakukun force-pushed the yt/bundle-cpp-objs-static branch from fa7c0f3 to ed97c9c Compare September 8, 2024 04:23

kateinoigakukun merged commit f078f7a into swiftlang:main Sep 8, 2024
3 checks passed

grynspan added this to the Swift 6.1 milestone Sep 10, 2024

grynspan removed the swift-6.1 label Sep 10, 2024

kateinoigakukun mentioned this pull request Sep 12, 2024

[6.0.2] Fix WASI build with CMake #687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: Install `_TestingInternals` in static library builds #651

CMake: Install `_TestingInternals` in static library builds #651

kateinoigakukun commented Aug 29, 2024 •

edited

Loading

grynspan commented Aug 29, 2024

rintaro Aug 30, 2024

kateinoigakukun Aug 30, 2024

etcwilde left a comment •

edited

Loading

grynspan commented Aug 30, 2024

etcwilde commented Aug 30, 2024

grynspan commented Aug 30, 2024

etcwilde commented Aug 30, 2024

compnerd commented Aug 31, 2024

kateinoigakukun commented Sep 3, 2024

etcwilde Sep 6, 2024

compnerd Sep 6, 2024

kateinoigakukun Sep 8, 2024

etcwilde commented Sep 6, 2024

compnerd Sep 6, 2024

kateinoigakukun commented Sep 8, 2024

		set_property(TARGET Testing PROPERTY STATIC_LIBRARY_OPTIONS
		$<TARGET_OBJECTS:_TestingInternals>)

CMake: Install _TestingInternals in static library builds #651

CMake: Install _TestingInternals in static library builds #651

Conversation

kateinoigakukun commented Aug 29, 2024 • edited Loading

Checklist:

grynspan commented Aug 29, 2024

rintaro Aug 30, 2024

Choose a reason for hiding this comment

kateinoigakukun Aug 30, 2024

Choose a reason for hiding this comment

etcwilde left a comment • edited Loading

Choose a reason for hiding this comment

grynspan commented Aug 30, 2024

etcwilde commented Aug 30, 2024

grynspan commented Aug 30, 2024

etcwilde commented Aug 30, 2024

compnerd commented Aug 31, 2024

kateinoigakukun commented Sep 3, 2024

etcwilde Sep 6, 2024

Choose a reason for hiding this comment

compnerd Sep 6, 2024

Choose a reason for hiding this comment

kateinoigakukun Sep 8, 2024

Choose a reason for hiding this comment

etcwilde commented Sep 6, 2024

compnerd Sep 6, 2024

Choose a reason for hiding this comment

kateinoigakukun commented Sep 8, 2024

CMake: Install `_TestingInternals` in static library builds #651

CMake: Install `_TestingInternals` in static library builds #651

kateinoigakukun commented Aug 29, 2024 •

edited

Loading

etcwilde left a comment •

edited

Loading