Skip to content

[embedded] Add a embedded-libraries CMake target to simplify the test dependencies #73870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2024
Merged

[embedded] Add a embedded-libraries CMake target to simplify the test dependencies #73870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions stdlib/public/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ elseif(BOOTSTRAPPING_MODE STREQUAL "OFF")
set(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB FALSE)
endif()

if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
add_custom_target(embedded-libraries ALL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALL should not be needed for a helper target for the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies for the very late reply :)

I think I need an "ALL" somewhere, either here on the "umbrella" target (embedded-libraries) or on the individual libraries. I think I'd like to think about embedded-libraries not just as a helper for tests, but rather as a top-level target for building embedded libraries in general. In which case, it should be "ALL" so that it gets built even if you are not running tests. Would it make sense to keep "ALL" here (and remove on the individual libraries)? See the new diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALL is, in my opinion, a code smell in CMake, since it allows you to avoid setting the right dependencies. Only "top level" targets (targets that nobody depends on) should be added to ALL (again, in my opinion).

With ALL in targets, when somebody does not use cmake --build all and use a reduced set of installation components, many dependencies are missing because the default CI systems always build all and people forget to set up the dependencies properly because it works for free in CI.

This is mostly a "nit". I was pointing out that it is not necessary because the dependencies are well setup at the moment. Still unnecessary with the newer version of the PR, if I am not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! I guess the concrete reason why I think I still need "ALL" here is that if I drop it, running ninja in the build directory will not build the embedded libraries. Same with running build-script without a -t. But we do build the regular stdlib in those cases.

endif()

set(EMBEDDED_STDLIB_TARGET_TRIPLES)

if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB_CROSS_COMPILING)
Expand Down
3 changes: 2 additions & 1 deletion stdlib/public/Concurrency/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ elseif(BOOTSTRAPPING_MODE STREQUAL "OFF")
set(SWIFT_SHOULD_BUILD_EMBEDDED_CONCURRENCY FALSE)
endif()
if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB AND SWIFT_SHOULD_BUILD_EMBEDDED_CONCURRENCY)
add_custom_target(embedded-concurrency ALL)
add_custom_target(embedded-concurrency)
add_dependencies(embedded-libraries embedded-concurrency)

set(SWIFT_ENABLE_REFLECTION OFF)
set(SWIFT_STDLIB_SUPPORT_BACK_DEPLOYMENT OFF)
Expand Down
3 changes: 2 additions & 1 deletion stdlib/public/Platform/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ add_swift_target_library(swiftDarwin ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES}
if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
set(SWIFT_ENABLE_REFLECTION OFF)

add_custom_target(embedded-darwin ALL)
add_custom_target(embedded-darwin)
add_dependencies(embedded-libraries embedded-darwin)
foreach(entry ${EMBEDDED_STDLIB_TARGET_TRIPLES})
string(REGEX REPLACE "[ \t]+" ";" list "${entry}")
list(GET list 0 arch)
Expand Down
3 changes: 2 additions & 1 deletion stdlib/public/Synchronization/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ add_swift_target_library(swiftSynchronization ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES
# i.e. there is no .o or .a file produced (no binary code is actually produced)
# and only users of a library are going to actually compile any needed code.
if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
add_custom_target(embedded-synchronization ALL)
add_custom_target(embedded-synchronization)
add_dependencies(embedded-libraries embedded-synchronization)

foreach(entry ${EMBEDDED_STDLIB_TARGET_TRIPLES})
string(REGEX REPLACE "[ \t]+" ";" list "${entry}")
Expand Down
3 changes: 2 additions & 1 deletion stdlib/public/Volatile/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ add_swift_target_library(swift_Volatile ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} IS_S
)

if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
add_custom_target(embedded-volatile ALL)
add_custom_target(embedded-volatile)
add_dependencies(embedded-libraries embedded-volatile)
foreach(entry ${EMBEDDED_STDLIB_TARGET_TRIPLES})
string(REGEX REPLACE "[ \t]+" ";" list "${entry}")
list(GET list 0 arch)
Expand Down
3 changes: 2 additions & 1 deletion stdlib/public/core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,8 @@ add_swift_target_library(swiftCore
# i.e. there is no .o or .a file produced (no binary code is actually produced)
# and only users of a library are going to actually compile any needed code.
if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
add_custom_target(embedded-stdlib ALL)
add_custom_target(embedded-stdlib)
add_dependencies(embedded-libraries embedded-stdlib)

set(SWIFT_ENABLE_REFLECTION OFF)
set(SWIFT_STDLIB_SUPPORT_BACK_DEPLOYMENT OFF)
Expand Down
3 changes: 2 additions & 1 deletion stdlib/public/stubs/Unicode/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@

# Embedded Swift Unicode library
if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
add_custom_target(embedded-unicode ALL)
add_custom_target(embedded-unicode)
add_dependencies(embedded-libraries embedded-unicode)

foreach(entry ${EMBEDDED_STDLIB_TARGET_TRIPLES})
string(REGEX REPLACE "[ \t]+" ";" list "${entry}")
Expand Down
18 changes: 2 additions & 16 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,22 +320,8 @@ foreach(SDK ${SWIFT_SDKS})
endif()
endif()

if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB)
if(TARGET "embedded-stdlib")
list(APPEND test_dependencies "embedded-stdlib")
endif()
if(TARGET "embedded-darwin")
list(APPEND test_dependencies "embedded-darwin")
endif()
if(TARGET "embedded-concurrency")
list(APPEND test_dependencies "embedded-concurrency")
endif()
if(TARGET "embedded-synchronization")
list(APPEND test_dependencies "embedded-synchronization")
endif()
if(TARGET "embedded-volatile")
list(APPEND test_dependencies "embedded-volatile")
endif()
if(TARGET "embedded-libraries")
list(APPEND test_dependencies "embedded-libraries")
endif()

if(NOT "${COVERAGE_DB}" STREQUAL "")
Expand Down