CMake: promote recommended good practices

[massonal]

Linked to #648

Hello,

I have been studying the CMake files generated to build to build code with cbuild (CMakeLists.txt + GCC/IAR/AC6.cmake). I have found several anti-patterns there, that I think both obfuscate the file and potentially trigger bugs (cf. below for an actual example).

I would do the following to improve the quality of the CMake files:

• remove redundant variables (INCLUDES, ....): prefer direct calls to target_include_directories()...
• use standard variables and notations (as opposed to: PRJ_DIR, "-Wl,", ...)
• absolute paths: prevent moving and sharing projects + hit filesystems limits, as a full tree is created in the build directory (I think this is already being discussed in Proposal for shorter file/pathnames #314 ; [csolution] [cbuild] directory tree gets difficult to use #221 )
• do not rely on "magic" internal variables (e.g., TARGET, used in a recipe in the toolchain file; this recipe would be more robust as a function) (cf. [csolution] [cbuild] directory tree gets difficult to use #221 (comment))

If any of these limitations is due to an actual technical limitation and can't be fixed, I think this should be documented in the file, in order not to confuse future readers.

[brondani]

@massonal
Thanks for the suggestions. Can you please elaborate each item?

[massonal]

Sure can do ;)
Here you go:

remove redundant variables

Several items are defined as variables first, and then used exactly once in a target configuration function call. It would be better to not define the variable at all and call the function directly (to improve readability and robustness, mostly)
Example: turn this

set(INC_PATHS
  ".../RTE/Device/STM32U585AIIx"
  ".../cmsis/CMSIS/Core/Include"
  ".../stm32u5xx/Include"
  ".../targets/B-U585I-IOT02A"
)

# possibly
# lots
# of stuff
# in between

target_include_directories(${TARGET} PUBLIC ${INC_PATHS})

into that

target_include_directories(${TARGET} PUBLIC
  ".../RTE/Device/STM32U585AIIx"
  ".../cmsis/CMSIS/Core/Include"
  ".../stm32u5xx/Include"
  ".../targets/B-U585I-IOT02A"
)

This way everything is in one place!

An even worse example is the DEFINES variables, which is set in CMakeLists.txt and used in the "toolchain" (even though it's not really a CMake toolchain file, cf. #648 ).

use standard variables and notations

Some variables are defined that seem to have the same role as variables defined and standardized by CMake (cmake-variables.7).
Examples include:

• PRJ_DIR -> CMAKE_SOURCE_DIR
• OUT_DIR -> CMAKE_BINARY_DIR
• INT_DIR -> CMAKE_BINARY_DIR (redundant? It has the same value as OUT_DIR)
• BYTE_ORDER -> CMAKE_<LANG>_BYTE_ORDER
• OPTIMIZE -> CMAKE_BUILD_TYPE
• DEBUG -> CMAKE_BUILD_TYPE
• WARNINGS -> CMAKE_BUILD_TYPE
  ...

Also, to ease portability, CMake has the LINKER: syntax to help pass options to the linker without hard-coding syntax such as GCC's -Wl,-foo,-bar: CMAKE_<LANG>_LINKER_WRAPPER_FLAG.

absolute paths

The point is already being discussed in the issues I liked in my first post (#221 #314).
Briefly, all the paths that end up in CMakeLists.txt are absolute paths:

set(CC_SRC_FILES
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal.c"
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal_pwr.c"
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal_pwr_ex.c"
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal_rcc.c"
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal_rcc_ex.c"
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal_uart.c"
  "C:/Users/massonal/path/to/hal/repository/drivers/stm32u5xx_hal/Src/stm32u5xx_hal_uart_ex.c"
  "C:/Users/massonal/path/to/project/workspace/my/project/main.c"
)

(Source files, and include paths, and linker scripts, ...)

This has several drawbacks:

• Such a full hierarchy will be duplicated in the build directory (e.g., main.o will land in C:/Users/massonal/path/to/project/workspace/my/project/build/c_/Users/massonal/path/to/project/workspace/my/project/main.o. This can, trigger filesystem path length limitations, and prevent the project from building at all.
• CMake projects are meant to be relocatable: you should be able to copy/paste the project directory, and re-run cmake --build (or, equivalently, ninja, in our case), without taking any supplementary step. Notably, in the current situation, the whole CMakeLists.txt has to be regerated!

Now, I can give some technical hints to help replace absolute paths with relative ones:

• through a judicious use of CMAKE_SOURCE_DIR et al. -- Most CMake functions interpret paths relative to this variable, or one of a few others.
• Modularization: CMake promotes modularization; having a CMakeLists.txt per pack could help in this regard.

do not rely on "magic" internal variables

This is close to the point I made earlier about DEFINES being defined in CMakeLists.txt and used in the toolchain. An other example is the TARGET variable, or CC_DEFINES, CC_CPU, CC_SECURE...
What I mean is that these two files belong in separate universes, have different responsibilities, and should not assume what the other does, or communicate with one another at all, really!
This is a philosophy and robustness issue, but also a very practical one: as a simple example, a well-defined toolchain file can be called more than once during the project generation, including super early, possibly before CMakeLists.txt has had the chance to define some variables it should have. This can end up in a broken, where one part is properly defined while the other is not, that are very complicated to debug and fix (speaking from experience here 😢).

A related point can be made about TOOLCHAIN_ROOT, which is initialized with an environment variable.
This breaks the assumption that vanilla builds work and prevents a user (or an external tool) from running CMake manually.
I would replace that with $PATH editions in cbuild before it calls CMake: this way, there is less ambiguity, and find_program() behaves in a more predictable fashion (this is the usual, recommended command to help find the compiler executable).

[brondani]

@massonal Thanks for the explanations.
Some observations:

remove redundant variables

DEFINES are used for compiler agnostic key-value pairs, being processed by <toolchain>.<version>.cmake for compatibility with the supported compiler/assembler syntaxes. This is not yet supported by CMake standard functions.

use standard variables and notations

OUT_DIR contains output files such as build artifacts and can be set to a different path from INT_DIR that contains intermediate files such as object files.
OPTIMIZE, DEBUG, WARNINGS contains individual compiler agnostic selections for each compiler option (optimization level, debug information generation, warnings level), while CMAKE_BUILD_TYPE specifies a build type configuration.

Also, to ease portability, CMake has the LINKER: syntax

Unfortunately CMake does not offer the same for compiler and assembler, do you know more on this matter?

absolute paths
Such a full hierarchy will be duplicated in the build directory

It seems CMake always generates full path for object files, independently of source paths being relative. From CMAKE_OBJECT_PATH_MAX:
CMake computes for every source file an object file name that is unique to the source file and deterministic with respect to the full path to the source file. This allows multiple source files in a target to share the same name if they lie in different directories without rebuilding when one is added or removed.

CMake projects are meant to be relocatable.

This is a conceptual difference. Currently generated CMakeLists.txt are not the root of CMSIS projects. It is an intermediate file exclusively for the building scope.

Modularization: CMake promotes modularization; having a CMakeLists.txt per pack could help in this regard.

This can be a complex matter, it has been discussed several times. I would recommend to read about CMSIS Pack features to understand why a CMSIS Pack is different from a CMake based library.

do not rely on "magic" internal variables
What I mean is that these two files belong in separate universes, have different responsibilities, and should not assume what the other does, or communicate with one another at all, really!

Explained in #648

I would replace that with $PATH editions in cbuild before it calls CMake: this way, there is less ambiguity, and find_program() behaves in a more predictable fashion (this is the usual, recommended command to help find the compiler executable).

It is actually less predictable than the TOOLCHAIN_ROOT approach, which should be set to a known toolchain version, since it also considers the semantic version of each supported toolchain. But there is certainly room for improvement on this area, as commented here.

---

Please feel free to take part in Open-CMSIS-Pack meetings, good suggestions are always welcomed.