Configure CMake build to install content into toolchain #581

rintaro · 2024-07-26T22:32:18Z

(Supersedes #531)

This includes several changes to the CMake rules to support installing built content into a toolchain.

Motivation:

This is necessary to support including swift-testing in Swift 6 toolchains.

Checklist:

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

Sources/CMakeLists.txt

Sources/Testing/CMakeLists.txt

Sources/Testing/ABI/EntryPoints/SwiftPMEntryPoint.swift

Sources/Testing/ExitTests/WaitFor.swift

…it's _not_ built as an executable

rintaro · 2024-08-03T03:13:24Z

@swift-ci Please test

rintaro · 2024-08-03T04:48:08Z

@swift-ci Please test

rintaro · 2024-08-03T04:53:30Z

Sources/Testing/CMakeLists.txt

@@ -97,16 +97,9 @@ target_link_libraries(Testing PRIVATE
 add_dependencies(Testing
  TestingMacros)
 target_compile_options(Testing PRIVATE
-  -enable-library-evolution)
+  -enable-library-evolution
+  -emit-module-interface -emit-module-interface-path $<TARGET_PROPERTY:Testing,Swift_MODULE_DIRECTORY>/Testing.swiftinterface)


We might want to stop emitting .swiftinterface unless it's macOS because .swiftinterface is not installed and useless.

(Should we stop -enable-library-evolution too?)

I think we need -enable-library-evolution to avoid folks directly linking our C++ module?

Sources/Testing/CMakeLists.txt

compnerd · 2024-08-06T02:51:06Z

Sources/CMakeLists.txt

+  # TestingMacros uses `ExternalProject` and we cannot directly query the
+  # properties of its targets here.
+  if(NOT SwiftTesting_BuildMacrosAsExecutables)
+    if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")


Should these not be checking CMAKE_HOST_SYSTEM_NAME?

compnerd · 2024-08-06T02:52:04Z

Sources/TestingMacros/CMakeLists.txt

+  target_link_options(TestingMacros PRIVATE "-no-toolchain-stdlib-rpath")
+  # Not setting RPATH means it requires all the dependencies are already loaded
+  # in the process, because 'plugin' directory wouldn't contain any dependencies.
+  set_property(TARGET TestingMacros PROPERTY INSTALL_RPATH)


You should use set_target_properties to avoid calling the function twice.

I don't know how to unset a property using set_target_properties.

Is it critical to change it? Or just an efficiency thing?

Efficiency and stylistic thing I think. I believe this also work,

set_target_properties(TestingMacros PROPERTIES INSTALL_RPATH "" BUILD_WITH_INSTALL_RPATH YES)

but I just wanted to clear the property. tbh, I'm not sure which is more correct. Explicit empty list ("") or clearing.

I believe you can do both with set_property

set_property(TARGET TestingMacros PROPERTY INSTALL_RPATH PROPERTY BUILD_WITH_INSTALL_RPATH YES)

Hmm, are you sure? I don't think this handles multiple PROPERTY pairs
https://github.com/Kitware/CMake/blob/5e734dd25a466d8f6e328bf6fc49bc44a4c4c480/Source/cmSetPropertyCommand.cxx#L387

rintaro · 2024-08-06T13:36:30Z

@swift-ci Please test

Sources/CMakeLists.txt

Add `SwiftTesting_MACRO` configuration option: * By default, the plugin is built using `SwiftTesting_MACRO_*` options * If it's explicitly "NO" (or other 'false' value), build Testing library without using the plugin * Otherwise, use the passed-in value as the plugin path Update macOS install location to 'usr/lib/swift/testing' for libraries, and 'usr/lib/host/plugins/testing' for plugin. Correct the install RPATH.

rintaro · 2024-08-06T22:37:55Z

@swift-ci Please test

rintaro · 2024-08-09T17:46:30Z

@swift-ci Please test

etcwilde · 2024-08-09T23:47:21Z

Sources/CMakeLists.txt

+    set(SwiftTesting_MACRO_Swift_COMPILER ${CMAKE_Swift_COMPILER})
+  endif()
+  if(NOT SwiftTesting_MACRO_Swift_FLAGS)
+    set(SwiftTesting_MACRO_Swift_FLAGS ${CMAKE_Swift_FLAGS})


What flags are you hoping to pull here?
This seems risky given that these flags might be for an entirely different machine than the one the macros will build for/run on.

This is actually a leftover from when I was trying around. Let me remove this as a followup.

etcwilde · 2024-08-09T23:58:38Z

Sources/TestingMacros/CMakeLists.txt

+  target_link_options(TestingMacros PRIVATE "-no-toolchain-stdlib-rpath")
+  # Not setting RPATH means it requires all the dependencies are already loaded
+  # in the process, because 'plugin' directory wouldn't contain any dependencies.
+  set_property(TARGET TestingMacros PROPERTY INSTALL_RPATH)


I believe you can do both with set_property

set_property(TARGET TestingMacros PROPERTY INSTALL_RPATH PROPERTY BUILD_WITH_INSTALL_RPATH YES)

grynspan reviewed Jul 27, 2024

View reviewed changes

Sources/CMakeLists.txt Outdated Show resolved Hide resolved

rintaro force-pushed the toolchain branch 2 times, most recently from 0d05234 to a0b2146 Compare July 30, 2024 20:12

rintaro mentioned this pull request Jul 30, 2024

Build and install swift-testing in toolchains swiftlang/swift#75515

Merged

rintaro force-pushed the toolchain branch 3 times, most recently from 35786c8 to 5e09d09 Compare August 1, 2024 21:37

rintaro commented Aug 1, 2024

View reviewed changes

Sources/Testing/CMakeLists.txt Outdated Show resolved Hide resolved

grynspan reviewed Aug 1, 2024

View reviewed changes

Sources/Testing/ABI/EntryPoints/SwiftPMEntryPoint.swift Outdated Show resolved Hide resolved

grynspan reviewed Aug 1, 2024

View reviewed changes

Sources/Testing/ExitTests/WaitFor.swift Outdated Show resolved Hide resolved

rintaro force-pushed the toolchain branch from 5e09d09 to 64dae81 Compare August 2, 2024 16:27

stmontgomery added 4 commits August 2, 2024 19:35

Configure CMake build to install content into toolchain

295838e

Fix negated conditional to ensure the macro plugin is installed when …

008ea62

…it's _not_ built as an executable

Fix the macro install locations

a5ffe52

Use the correct library name for the macro plugin on Linux

5eee2cf

rintaro force-pushed the toolchain branch from 64dae81 to 0658335 Compare August 3, 2024 03:11

rintaro mentioned this pull request Aug 3, 2024

[PackageModel] Support swift-testing installed in a toolchain swiftlang/swift-package-manager#7840

Merged

rintaro force-pushed the toolchain branch from 0658335 to d66a3f8 Compare August 3, 2024 04:38

rintaro commented Aug 3, 2024

View reviewed changes

Sources/Testing/CMakeLists.txt Outdated Show resolved Hide resolved

rintaro marked this pull request as ready for review August 3, 2024 14:12

rintaro requested review from stmontgomery, dennisweissmann, briancroom, SeanROlszewski and suzannaratcliff as code owners August 3, 2024 14:12

rintaro changed the title ~~[WIP] Configure CMake build to install content into toolchain~~ Configure CMake build to install content into toolchain Aug 3, 2024

briancroom approved these changes Aug 5, 2024

View reviewed changes

compnerd reviewed Aug 6, 2024

View reviewed changes

compnerd requested a review from etcwilde August 6, 2024 02:52

rintaro force-pushed the toolchain branch from d66a3f8 to 849f008 Compare August 6, 2024 13:35

edymtt approved these changes Aug 6, 2024

View reviewed changes

Sources/CMakeLists.txt Outdated Show resolved Hide resolved

rintaro force-pushed the toolchain branch from 849f008 to 2753f4d Compare August 6, 2024 22:37

grynspan assigned rintaro Aug 7, 2024

grynspan added enhancement New feature or request swiftpm-integration Swift Package Manager integration tools integration Integration of swift-testing into tools/IDEs swift-6 labels Aug 7, 2024

[CMake] Don't install macros from the main CMake invocation

91ffb07

rintaro removed their assignment Aug 9, 2024

bnbarham approved these changes Aug 9, 2024

View reviewed changes

etcwilde reviewed Aug 9, 2024

View reviewed changes

rintaro merged commit 264fbd9 into swiftlang:main Aug 10, 2024
3 checks passed

grynspan mentioned this pull request Aug 10, 2024

Configure CMake rules to install content into toolchain #531

Closed

2 tasks

ADKaster mentioned this pull request Aug 27, 2024

[CMake] Fix build with SwiftTesting_BuildMacrosAsExecutables option set #645

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure CMake build to install content into toolchain #581

Configure CMake build to install content into toolchain #581

rintaro commented Jul 26, 2024 •

edited

Loading

rintaro commented Aug 3, 2024

rintaro commented Aug 3, 2024

rintaro Aug 3, 2024 •

edited

Loading

grynspan Aug 6, 2024

compnerd Aug 6, 2024

compnerd Aug 6, 2024

rintaro Aug 6, 2024

grynspan Aug 7, 2024

rintaro Aug 9, 2024 •

edited

Loading

etcwilde Aug 9, 2024

rintaro Aug 10, 2024

rintaro commented Aug 6, 2024

rintaro commented Aug 6, 2024

rintaro commented Aug 9, 2024

etcwilde Aug 9, 2024

rintaro Aug 10, 2024

etcwilde Aug 9, 2024

Configure CMake build to install content into toolchain #581

Configure CMake build to install content into toolchain #581

Conversation

rintaro commented Jul 26, 2024 • edited Loading

Motivation:

Checklist:

rintaro commented Aug 3, 2024

rintaro commented Aug 3, 2024

rintaro Aug 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro commented Aug 6, 2024

rintaro commented Aug 6, 2024

rintaro commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rintaro commented Jul 26, 2024 •

edited

Loading

rintaro Aug 3, 2024 •

edited

Loading

rintaro Aug 9, 2024 •

edited

Loading