[CMake] Explicitly link Testing to Foundation #693

rintaro · 2024-09-16T17:53:25Z

Explicitly link to Foundation in CMake builds

If Foundation_DIR is passed, find_package finds the Foundation target and target_link_libraries adds required -I and -l/-L. If not, find_package fails and target_link_libraries just assumes it's a system library name and just adds -lFoundation.

grynspan · 2024-09-16T19:24:19Z

Sources/Testing/CMakeLists.txt

@@ -96,6 +96,8 @@ add_library(Testing
  Traits/Trait.swift)
 target_link_libraries(Testing PRIVATE
  _TestingInternals)
+target_link_libraries(Testing PUBLIC


Will this fail if for some reason we can't find Foundation?

I think it should fail with linker errors.

etcwilde · 2024-09-16T21:08:59Z

CMakeLists.txt

@@ -15,6 +15,11 @@ endif()
 project(SwiftTesting
  LANGUAGES CXX Swift)

+if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)


Suggested change

if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)

if(NOT APPLE)

etcwilde · 2024-09-16T21:09:56Z

Sources/Testing/CMakeLists.txt

@@ -96,6 +96,11 @@ add_library(Testing
  Traits/Trait.swift)
 target_link_libraries(Testing PRIVATE
  _TestingInternals)
+if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)


Suggested change

if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)

if(NOT APPLE)

etcwilde · 2024-09-16T21:12:02Z

CMakeLists.txt

@@ -15,6 +15,11 @@ endif()
 project(SwiftTesting
  LANGUAGES CXX Swift)

+if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
+  find_package(dispatch)


If we go this route instead of using the build-directory configurations, we'll need to include a Finddispatch.cmake and FindFoundation.cmake in our modules directory and move these down below the list(APPEND CMAKE_MODUKE_PATH ... on line 25.

I think find_package() tries Module mode first then falls backs to Config mode IIUC.

When the basic signature is used, the command searches in Module mode first. If the package is not found, the search falls back to Config mode.

Sure, but I think we still would want the Find<PackageName>.cmake modules or I don't really see much point in not just using the CONFIG form.

Let's just use CONFIG then.

kateinoigakukun · 2024-09-17T03:55:54Z

Sources/Testing/CMakeLists.txt

@@ -96,6 +96,11 @@ add_library(Testing
  Traits/Trait.swift)
 target_link_libraries(Testing PRIVATE
  _TestingInternals)
+if(NOT APPLE)
+  target_link_libraries(Testing PUBLIC
+    dispatch


Can we optionalize this dispatch link as WASI doesn't support libdispatch? You can ensure the change doesn't break WASI build by @swift-ci test WebAssembly in cross repo testing.

What would be the condition? if(NOT WASI)?

I think it would be if(NOT CMAKE_SYSTEM_NAME STREQUAL WASI).

👍 Since WASI is equivalent to CMAKE_SYSTEM_NAME STREQUAL WASI, I went with WASI

WASI is available only in the bleeding edge CMakes (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9659), so can we stick on CMAKE_SYSTEM_NAME STREQUAL WASI for now?

WASI doesn't support `libdispatch`

CMakeLists.txt

Sources/Testing/CMakeLists.txt

stmontgomery · 2024-09-20T17:30:56Z

Sources/Testing/CMakeLists.txt

@@ -96,6 +96,14 @@ add_library(Testing
  Traits/Trait.swift)
 target_link_libraries(Testing PRIVATE
  _TestingInternals)
+if(NOT APPLE)


Curious why we need the if(NOT APPLE) guard for these target_link_libraries? I get why it's needed for find_package, but don't we want to link Foundation even on Darwin?

In Darwin, Foundation is a framework, so -lFoundation doesn't work.
Yes it links Foundation, but that's done via auto-linking. I.e. import Foundation automatically sets required linker flags.

stmontgomery

Had one question but LGTM regardless

@stmontgomery

Cherry-pick #693 into `release/6.0` * **Explanation**: Previously in CMake builds, when `Foundation` was not in the regular search directory (e.g. resource directory or SDK search paths) `#if canImport(Foundation)` used to fail, and the functionalities are not included. This patch provides a way to provide `Foundation_DIR` for `find_packgage(Foundation CONFIG)`, so that clients can correctly link Testing to Foundation * **Scope**: CMake builds * **Risk**: Low. No actual code changes. * **Testing**: Passes current test suite Also manually tested the build toolchain * **Issues**: N/A * **Reviewer**: @stmontgomery

@stmontgomery

Cherry-pick #693 into `release/6.0.2` * **Explanation**: Previously in CMake builds, when `Foundation` was not in the regular search directory (e.g. resource directory or SDK search paths) `#if canImport(Foundation)` used to fail, and the functionalities are not included. This patch provides a way to provide `Foundation_DIR` for `find_packgage(Foundation CONFIG)`, so that clients can correctly link Testing to Foundation * **Scope**: CMake builds * **Risk**: Low. No actual code changes. * **Testing**: Passes current test suite Also manually tested the build toolchain * **Issues**: N/A * **Reviewer**: @stmontgomery --------- Co-authored-by: Jonathan Grynspan <jgrynspan@apple.com>

rintaro requested review from stmontgomery, grynspan, dennisweissmann, briancroom, SeanROlszewski and suzannaratcliff as code owners September 16, 2024 17:53

rintaro mentioned this pull request Sep 16, 2024

[Windows] Teach Build-Testing how to find Foundation swiftlang/swift#76489

Merged

rintaro self-assigned this Sep 16, 2024

grynspan reviewed Sep 16, 2024

View reviewed changes

grynspan added bug Something isn't working build Affects the project's build configuration or process labels Sep 16, 2024

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

rintaro force-pushed the cmake-link-foundation branch 2 times, most recently from 8072ea4 to 8aa2643 Compare September 16, 2024 19:42

etcwilde reviewed Sep 16, 2024

View reviewed changes

[CMake] Explicitly link Testing to Foundation

79a443a

rintaro force-pushed the cmake-link-foundation branch from 8aa2643 to 79a443a Compare September 16, 2024 22:49

kateinoigakukun reviewed Sep 17, 2024

View reviewed changes

Don't link to dispatch in WASI

c2f8ccc

WASI doesn't support `libdispatch`

rintaro force-pushed the cmake-link-foundation branch from fa43fb6 to c2f8ccc Compare September 17, 2024 15:33

compnerd reviewed Sep 17, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

Sources/Testing/CMakeLists.txt Show resolved Hide resolved

stmontgomery reviewed Sep 20, 2024

View reviewed changes

stmontgomery approved these changes Sep 20, 2024

View reviewed changes

rintaro merged commit e20774a into swiftlang:main Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMake] Explicitly link Testing to Foundation #693

[CMake] Explicitly link Testing to Foundation #693

rintaro commented Sep 16, 2024

grynspan Sep 16, 2024

rintaro Sep 16, 2024

etcwilde Sep 16, 2024

etcwilde Sep 16, 2024

etcwilde Sep 16, 2024

rintaro Sep 16, 2024

etcwilde Sep 16, 2024

rintaro Sep 16, 2024

kateinoigakukun Sep 17, 2024

rintaro Sep 17, 2024

kateinoigakukun Sep 17, 2024

rintaro Sep 17, 2024 •

edited

Loading

kateinoigakukun Sep 17, 2024

stmontgomery Sep 20, 2024

rintaro Sep 20, 2024

stmontgomery left a comment

[CMake] Explicitly link Testing to Foundation #693

[CMake] Explicitly link Testing to Foundation #693

Conversation

rintaro commented Sep 16, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro Sep 17, 2024 • edited Loading

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

stmontgomery left a comment

Choose a reason for hiding this comment

rintaro Sep 17, 2024 •

edited

Loading