[embedded] Add a embedded-libraries CMake target to simplify the test dependencies #73870
Conversation
kubamracek
requested review from
drodriguez,
phausler,
al45tair,
NuriAmari,
rauhul,
zoecarver and
eeckstein
| @@ -164,6 +164,8 @@ elseif(BOOTSTRAPPING_MODE STREQUAL "OFF") | |||
| endif() | |||
|
|
|||
| if(SWIFT_SHOULD_BUILD_EMBEDDED_STDLIB) | |||
| add_custom_target(embedded-libraries ALL) | |||
There was a problem hiding this comment.
ALL should not be needed for a helper target for the tests.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
kubamracek
force-pushed
the
embedded-libraries-cmake
branch
from
e5e4bba to
2e9d59e
Compare
|
@swift-ci please test |
kubamracek
force-pushed
the
embedded-libraries-cmake
branch
from
2e9d59e to
dda68d6
Compare
|
@swift-ci please test Windows platform |
kubamracek
force-pushed
the
embedded-libraries-cmake
branch
from
dda68d6 to
c4b52c3
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
kubamracek
force-pushed
the
embedded-libraries-cmake
branch
from
c4b52c3 to
fc9edc8
Compare
|
@swift-ci please test |
kubamracek
force-pushed
the
embedded-libraries-cmake
branch
from
fc9edc8 to
6eeef12
Compare
|
@swift-ci please test |
NFC, just to simplify the repetitive CMake code in
test/CMakeLists.txt.