Hotfix CMake & ETL #604

muellerr · 2022-04-27T09:22:00+02:00

muellerr commented

2022-04-27 09:22:00 +02:00

If find_package is used to find the ETL package, the target will be named etl::etl. This is what I have seen in most installed packages.

If etl is linked in that case, CMake will assume this is a system library and add -letl, which of course does not work as etl is a header-only library.

It would be better if the ETL version fetched by FetchContent would be located
inside the etl namespace as well but I have not found a way to do this. This is due to the nature target names work if add_subdirectory or FetchContent are used. I think there is an ongoing effort to improve CMake in the context of mixing FetchContent and find_package anyway:

For now, the cleanest solution I found to have the same target name is to simply introduce a new alias target.

I also bumped the revision to https://github.com/ETLCPP/etl/tree/20.27.3 .
The CMake install support in the ETL is new and another bump might be good soon (maybe for next major version 21, and then try to remain stable)

If `find_package` is used to find the ETL package, the target will be named `etl::etl`. This is what I have seen in most installed packages. If `etl` is linked in that case, CMake will assume this is a system library and add `-letl`, which of course does not work as etl is a header-only library. It would be better if the ETL version fetched by `FetchContent` would be located inside the `etl` namespace as well but I have not found a way to do this. This is due to the nature target names work if `add_subdirectory` or `FetchContent` are used. I think there is an ongoing effort to improve `CMake` in the context of mixing `FetchContent` and `find_package` anyway: - https://gitlab.kitware.com/cmake/cmake/-/issues/16414 - https://gitlab.kitware.com/cmake/cmake/-/issues/21687 For now, the cleanest solution I found to have the same target name is to simply introduce a new alias target. I also bumped the revision to https://github.com/ETLCPP/etl/tree/20.27.3 . The CMake install support in the ETL is new and another bump might be good soon (maybe for next major version 21, and then try to remain stable)

muellerr added 2 commits 2022-04-27 09:22:05 +02:00

c80f06fbcb

hotfix for ETL lib dep

fsfw/fsfw/pipeline/head This commit looks good Details

fsfw/fsfw/pipeline/pr-development This commit looks good Details

64f0166b64

hotfix for new ETL dependency

muellerr requested review from gaisser 2022-04-27 09:24:11 +02:00

muellerr added the

bug

label 2022-04-27 09:24:16 +02:00

muellerr added this to the v5.0.0 milestone 2022-04-27 09:24:19 +02:00

muellerr changed title from ~~Hotfix ETL~~ to Hotfix CMake & ETL

2022-04-27 09:27:12 +02:00

muellerr added 1 commit 2022-04-27 09:37:28 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

17e609c3a5

some more var replacements

muellerr added 1 commit 2022-04-27 09:41:34 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

b00d83cb1a

bump ETL revision

gaisser approved these changes 2022-04-27 09:57:31 +02:00

Dismissed

gaisser left a comment

LGTM

gaisser dismissed gaisser’s review 2022-04-27 09:58:24 +02:00

gaisser reviewed 2022-04-27 10:00:01 +02:00

CMakeLists.txt Outdated

						
				@ -124,3 +125,3 @@

				    )

				    FetchContent_MakeAvailable(etl)

				    FetchContent_MakeAvailable(${FSFW_ETL_LIB_NAME})

gaisser commented

2022-04-27 10:00:01 +02:00

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

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

muellerr commented

2022-04-27 13:26:03 +02:00

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

gaisser marked this conversation as resolved

muellerr added a new dependency 2022-04-27 14:26:27 +02:00

#605 Bump Catch2 dependency & Catch2 pre-installed for CI/CD

muellerr added 1 commit 2022-04-27 21:54:44 +02:00