Skip to content

Commit

Permalink
[nrf noup] cmake: tools: Dont add a custom command for running the ma…
Browse files Browse the repository at this point in the history
…nifest

This commit is [nrf noup] because I would like to user-test this for a
few months in case of unintended side-effects before upstreaming.

In the TF-M build scripts we run the manifest tool twice, first from
CMake and then from ninja.

It is bad practice to configure CMake projects like this. Instead, if
configuration from CMake is necessary, one should configure from CMake
only, and then re-run CMake when necessary, not just the command.

This organization has been causing problems for our users as they have
been required to rebuild TF-M twice.

This is due to this scenario playing out:

CMake generates config_impl.cmake by invoking the manifest tool at
Configure time.

CMake generates build.ninja.

Ninja generates config_impl.cmake by invoking the manifest tool at
build time.

When the user then invokes ninja a second time config_impl.cmake will
be newer than build.ninja. But CMake is supposed to be includ'ing
config_impl.cmake, so build.ninja is now considered out-of-date
wrt. config_impl.cmake.

ninja therefore invokes CMake again, and then ninja afterwards.

Signed-off-by: Sebastian Bøe <[email protected]>
Change-Id: Icef588479d27fa3a172b40b09eacad417922fba5
  • Loading branch information
SebastianBoe authored and jfischer-no committed Mar 7, 2024
1 parent 5694fc5 commit 6a3bbde
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions tools/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,17 @@ set(MANIFEST_COMMAND
-o ${CMAKE_BINARY_DIR}/generated
${PARSE_MANIFEST_QUIET_FLAG})

set(NO_BUILD_CMD_FOR_MANIFEST 1)

if(NO_BUILD_CMD_FOR_MANIFEST)
else()
add_custom_command(
OUTPUT ${CMAKE_BINARY_DIR}/generated
COMMAND ${MANIFEST_COMMAND}
DEPENDS ${MANIFEST_LISTS} ${GENERATED_FILE_LISTS}
${MANIFEST_FILES} ${TEMPLATE_FILES}
)
endif()

add_custom_target(
manifest_tool
Expand Down

0 comments on commit 6a3bbde

Please sign in to comment.