fsfw/fsfw
fsfw/fsfw
10
3
Fork 6
Code Issues 31 Pull Requests 4 Releases 10 Wiki Activity

Hotfix CMake & ETL #604

Merged
gaisser merged 6 commits from mueller/hotfix-etl into development 2022-05-02 14:36:33 +02:00
Conversation 0 Commits 6 Files Changed 1 +16 -5
1 changed files with 16 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 6aa72892ed - Show all commits

21
CMakeLists.txt
View File

@ -77,9 +77,7 @@ if(FSFW_BUILD_UNITTESTS)
GIT_TAG ${FSFW_CATCH2_LIB_VERSION}
)
FetchContent_MakeAvailable(Catch2)
#fixes regression -preview4, to be confirmed in later releases
set_target_properties(Catch2 PROPERTIES DEBUG_POSTFIX "")
list(APPEND FSFW_FETCH_CONTENT_TARGETS Catch2)
endif()
set(FSFW_CONFIG_PATH tests/src/fsfw_tests/unit/testcfg)
@ -124,8 +122,21 @@ if(NOT ${FSFW_ETL_LIB_NAME}_FOUND)
GIT_TAG ${FSFW_ETL_LIB_VERSION}
)
FetchContent_MakeAvailable(${FSFW_ETL_LIB_NAME})
add_library(${FSFW_ETL_LINK_TARGET} ALIAS ${FSFW_ETL_LIB_NAME})
list(APPEND FSFW_FETCH_CONTENT_TARGETS ${FSFW_ETL_LIB_NAME})
endif()
gaisser marked this conversation as resolved Outdated
gaisser commented 2022-04-27 10:00:01 +02:00
Outdated
Review
Copy Link

If I read this correct, we should only call FetchContent_MakeAvailable once:

# WRONG: Should declare all details first
FetchContent_Declare(uses_other ...)
FetchContent_MakeAvailable(uses_other)

FetchContent_Declare(other ...)    # Will be ignored, uses_other beat us to it
FetchContent_MakeAvailable(other)  # Would use details declared by uses_other

# CORRECT: All details declared first, so they will take priority
FetchContent_Declare(uses_other ...)
FetchContent_Declare(other ...)
FetchContent_MakeAvailable(uses_other other)

This ensures dependency collection works correct for dependencies of the dependency (which we might not have ATM)

If I read [this](https://cmake.org/cmake/help/latest/module/FetchContent.html) correct, we should only call FetchContent_MakeAvailable once: ``` # WRONG: Should declare all details first FetchContent_Declare(uses_other ...) FetchContent_MakeAvailable(uses_other) FetchContent_Declare(other ...) # Will be ignored, uses_other beat us to it FetchContent_MakeAvailable(other) # Would use details declared by uses_other ``` ``` # CORRECT: All details declared first, so they will take priority FetchContent_Declare(uses_other ...) FetchContent_Declare(other ...) FetchContent_MakeAvailable(uses_other other) ``` This ensures dependency collection works correct for dependencies of the dependency (which we might not have ATM)
muellerr commented 2022-04-27 13:24:00 +02:00
Outdated
Review
Copy Link

The Catch2 dependency only needs to be loaded for unit tests I think

The Catch2 dependency only needs to be loaded for unit tests I think
muellerr commented 2022-04-27 13:26:03 +02:00
Outdated
Review
Copy Link

This could be solved with an if clause which either makes etl available or makes both available depending on whether Catch2 was loaded.

Or maybe MakeAvailable can deal with undeclared dependencies.. maybe I will test this

This could be solved with an if clause which either makes etl available or makes both available depending on whether Catch2 was loaded. Or maybe MakeAvailable can deal with undeclared dependencies.. maybe I will test this
# The documentation for FetchContent recommends declaring all the dependencies
# before making them available. We make all declared dependency available here
# after their declaration
if(FSFW_FETCH_CONTENT_TARGETS)
FetchContent_MakeAvailable(${FSFW_FETCH_CONTENT_TARGETS})
if(TARGET ${FSFW_ETL_LIB_NAME})
add_library(${FSFW_ETL_LINK_TARGET} ALIAS ${FSFW_ETL_LIB_NAME})
endif()
if(TARGET Catch2)
# Fixes regression -preview4, to be confirmed in later releases
set_target_properties(Catch2 PROPERTIES DEBUG_POSTFIX "")
endif()
endif()
set(FSFW_CORE_INC_PATH "inc")