Modernize CMake code

[kheaactua]

Status

For the moment this is still a WIP. I would love community feedback on this PR so that we can produce a modern CMake setup that functions well for everyone.

I suspect the cleanest way to organize the commits would be to squash them.

Description

General CMakeLists improvements - cmake v2→v3

• Included a .cmake-format file, and ran applied it to the main CMakeList files in this project.
  • The format file does a poor job on if and foreach blocks, but I believe overall it does a better job that the default settings. The main benefit is a consistent format
• Use targets instead of lib/include variables in CMake
• [Conditionally] Linking DLT as a target
• Write internal.hpp to build directory instead of source to prevent needless rebuilds
  • The include directives for this file are also updated in the code to use include paths rather than relative paths.
• Remove hard coded CMAKE_VERBOSE_MAKEFILE, this should be injected by the user if so desired
• "wrap"ing (ld's --wrap option) the "socket" symbol, see wrappers.cpp
• Added install directive to vsomeip/example/hello_world and vsomeip_ctrl
• Platform conditional socket lib linkage
• hello_world code uses ENABLE_SIGNALS, but its setting was missing in CMake
• Improve setup for boost stacktrace, really only helps on Linux and requires the backtrace.h file
• Added QNX platform section, though the credential code changes for QNX are not included in this commit.
• Use a CACHE variable for VSOMEIP_BASE_PATH. Upstream defaults this to /var for QNX which is a very bad choice.

Misc

• Added more ignore rules to for common IDEs

[duartenfonseca]

why did you separated this flag from the others related to LINUX?

[kheaactua]

The -Werror blocks my linux builds :) Honestly I don't think any component should ever include this. -Werror should be up to the toolchain - as the toolchain usually specifies which compile options to use, which warnings to watch for, and whether warnings should be allowed.

[kheaactua]

I am still working on this PR, I'll likely rebase to Fabio's 3.5.x branch. Stuff just takes time. :)

[duartenfonseca]

I am still working on this PR, I'll likely rebase to Fabio's 3.5.x branch. Stuff just takes time. :)

In that case maybe i will check this PR later! great job anyway, it has interesting improvements

[duartenfonseca]

@kheaactua tried to build this new CMake code and got a failure.
I did :
cmake -B build
cmake --build build

I have ubuntu 22.04, gcc version 11.4

the error i got was the following:
/home/duartef/repos/covesa/vsomeip_duarte/vsomeip/implementation/endpoints/src/../../routing/include/../../endpoints/include/netlink_connector.hpp:197:47: error: ‘VSOMEIP_MAX_NETLINK_RETRIES’ was not declared in this scope
197 | static const std::uint32_t max_retries_ = VSOMEIP_MAX_NETLINK_RETRIES;

[kheaactua]

@duartenfonseca I'll take a look! Was this on Fabio's 3.5.x? He did push a change to the network tests a few days back.

I'll also fix that conflict that appeared.

[duartenfonseca]

@kheaactua i just did a checkout to your branch and tried to build!

[duartenfonseca]

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

[kheaactua]

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

I raised it to 3.15 to accommodate Conan 2.0. From the author:

Conan 2.0 requires at least CMake 3.15.

Though not yet posted, I've been prepping covesa/capi-conan, covesa/capi-docker, and some others that all use Conan to set up CommonAPI environments.

Curious though, what would be desirable about staying at 3.13? With 3.23 vsomeip could use file sets for a cleaner install of the includes

Locally I jack it up to 3.30 :)

[duartenfonseca]

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

I raised it to 3.15 to accommodate Conan 2.0. From the author:

Conan 2.0 requires at least CMake 3.15.

Though not yet posted, I've been prepping covesa/capi-conan, covesa/capi-docker, and some others that all use Conan to set up CommonAPI environments.

Curious though, what would be desirable about staying at 3.13? With 3.23 vsomeip could use file sets for a cleaner install of the includes

Locally I jack it up to 3.30 :)

I understand your reasons, but if we could find a way to keep the CMake changes without increasing from 3.13 would be best. From our internal process, increasing this there would need to be a strong justification for this change.

[kheaactua]

@kheaactua is the increase of minimum version on cmake is really necessary? from our side, we would like to avoid that

I raised it to 3.15 to accommodate Conan 2.0. From the author:

Conan 2.0 requires at least CMake 3.15.

Though not yet posted, I've been prepping covesa/capi-conan, covesa/capi-docker, and some others that all use Conan to set up CommonAPI environments.
Curious though, what would be desirable about staying at 3.13? With 3.23 vsomeip could use file sets for a cleaner install of the includes
Locally I jack it up to 3.30 :)

I understand your reasons, but if we could find a way to keep the CMake changes without increasing from 3.13 would be best. From our internal process, increasing this there would need to be a strong justification for this change.

I could make Conan apply a patch, that would solve the issue with Conan. I'll give it a try with 3.13 as soon as I can. I may have to remove some of the generator expressions to support 3.13 as well.

Also, for me the Docker setup for the network tests fails with CMake 3.22 and below.

That said, I would strongly encourage the update.

• 3.26 supports LTO detection on QNX
• 3.26 improves the debug on try_compile - really helpful when trying to build on new systems
• 3.23 brings target_source( FILE_SET ) for better installation
• 3.18 and 3.30 improve how Boost is found (even as far back as 3.16 find_package(boost) is harder than it should be)
• 3.15 improves generator expressions
• 3.14 and 3.27 improve the build debug
• ~3.30 (I think) improves handling of generated files (helpful for CommonAPI)

Just a tonne of updates that help both with QNX and really just in general.

[duartenfonseca]

why stop using VSOMEIP_NAME ? if the name one day changes it will be hard to change everywhere

[kheaactua]

VSOMEIP_NAME names sense to control the OUTPUT_NAME of the artifact, but it doesn't make sense to control the target name in CMake it self.

For instance, in this line

add_library(${VSOMEIP_NAME}-cfg ...)`

this creates a target named ${VSOMEIP_NAME}-cfg with a default OUTPUT_NAME of ${VSOMEIP_NAME}-cfg (where ${VSOMEIP_NAME} is expanded)

But we never want to target name to change.

Instead, this changes the target to vsomeip-cfg, and line 399 sets the OUTPUT_NAME and EXPORT_NAME.

[duartenfonseca]

we need the -Wno-tsan flag, so it shouldn't be removed

[kheaactua]

Sounds good. Why do you need it though? Shouldn't this be in the sanitizer section?

[duartenfonseca]

This flag caused problems on clang so it was moved to GCC only

[kheaactua]

k, I restored it in 2d97573

I'm a little confused though, because tsan should only be present if you enable it.. But if you enable it and -Wno-tsan is always enabled, than tsan won't work.

Are you sure it wouldn't be better to handle this down in the sanitizers section? e.g.

  # Sanitizers

  if(ENABLE_UNDEFINED_SANITIZER)
    add_compile_options(-fsanitize=undefined)
  endif()

  if(ENABLE_THREAD_SANITIZER)
    if(${CMAKE_SYSTEM_NAME} MATCHES "Linux")
      message(WARNING "TSAN isn't enabled on GCC for vsomeip")
    else()
      add_compile_options(-fsanitize=thread)
    endif()
  endif()

[duartenfonseca]

the -O flag was removed, it is needed

[kheaactua]

Hmm? The optimization level should be controlled by the CMAKE_BUILD_TYPE. i.e. building in debug (-Og), or release (-O2 or -O3), etc

[duartenfonseca]

from what i read, there is no CMAKE_BUILD_TYPE that uses optimization flag -O. so in that case the optimization flag should still be added here

[kheaactua]

Not -O specifically, but the optimization is set by default. The defaults are:

Variable	Flags
CMAKE_CXX_FLAGS
CMAKE_CXX_FLAGS_DEBUG	-g -O0
CMAKE_CXX_FLAGS_RELEASE	-O3 -DNDEBUG
CMAKE_CXX_FLAGS_RELWITHDEBINFO	-O2 -g -DNDEBUG
CMAKE_CXX_FLAGS_MINSIZEREL	-Os -DNDEBUG

(Source Professional CMake: A Practical Guide, 10th edition)

I'd also steer away from -O it self. It's vague, I think -O is the same as -O1. In any case though, it's much better for the end user to be able to control this through a toolchain. Hardcoding an optimization level into the CMake removes the ability for the end user to control their optimization level.

[kheaactua]

k, take a look at a5ececf

[duartenfonseca]

why remove the INTERFACE keyword here? this folders are meant to be provided to the targets that link with vsomeip-lib

[kheaactua]

My understanding is:

• PUBLIC: The include directories are added to both the target itself and to any targets that link against it. This means that the include directories are used when compiling the target and are also propagated to consumers of the target.

• INTERFACE: The include directories are only added to targets that link against the target. They are not used when compiling the target itself, but they are propagated to consumers of the target.

In this case, libvsomeip3 uses these two directories it self, so they should be PUBLIC rather than an INTERFACE

[duartenfonseca]

isn't this a duplicate from lines 463-464?

[kheaactua]

Good eye. It is 100% redundant, I'll fix this

[kheaactua]

Fixed in 722fa96

[duartenfonseca]

is this a replacement of :
if(${CMAKE_SYSTEM_NAME} MATCHES "QNX") include_directories(${Boost_INCLUDE_DIR} ) else() include_directories(SYSTEM ${Boost_INCLUDE_DIR} ) endif()
?
it seems that it uses a different variable: Boost_INCLUDE_DIRS instead of Boost_INCLUDE_DIR

[duartenfonseca]

if this is the case, couldn't this target_include_directories be merged with the previous one on line 457?

[kheaactua]

I have to consult the CMake docs again to answer about Boost_INCLUDE_DIRS and Boost_INCLUDE_DIR - these two always confuse me. :)

This is why when possible I try to use the Boost::system target, but there's one case here where the include directory variable is directly required.

Checking it though

[duartenfonseca]

was this the line that helped you with the internal.hpp file?
if so, what problem did it fix?

[kheaactua]

Yes!

So, in the current version:

configure_file (
   "${PROJECT_SOURCE_DIR}/implementation/configuration/include/internal.hpp.in"
   "${PROJECT_SOURCE_DIR}/implementation/configuration/include/internal.hpp"
)

This writes internal.hpp into the source directory. I would argue that this is a build artifact though and should go into the build directory.

Especially because it's in the .gitignore, when there are changes to this file they're hard to see. When it's in the build directory it's trivial to delete the build directory and get a fresh build. When it's in the source directory though it persists. CMake should keep it updated, but I'm pretty sure what first brought my attention to this was the cases where it wasn't updated.

So the change here is to put it in the build directory:

configure_file(
  "${PROJECT_SOURCE_DIR}/implementation/configuration/include/internal.hpp.in"
  "${PROJECT_BINARY_DIR}/implementation/configuration/include/internal.hpp"
)

(key being ${PROJECT_BINARY_DIR})

And then the line you pointed out adds that path in the build directory to the include path.

The reason it causes me so much trouble is I have it in the binary directory in my forks, but when I switch back and forth from my forks to the upstream code that file re-appears in the source directory, and then overrides the version in the build directory.

By having it in the build directory, my builds seem more robust. Especially when I'm testing certain compiler flags, and have to call clang++ or q++ directly (without CMake) to see the different effects. Having internal.hpp be in the build directory instead of the source makes this a lot easier.

[duartenfonseca]

why is this needed, since the VSOMEIP_ENABLE_SIGNAL_HANDLING has already been set on the main cmakelists as an add_definitions() command

[kheaactua]

It is in the top level CMake file, but hello_world/CMakeLists.txt appears to be written to be a self contained project - and nothing uses add_subdirectory to include it in the build.

[duartenfonseca]

this way, the -Wl,-export-dynamic part of the EXPORT_SYMBOLS was not being removed. my suggestion would be to re-add the EXPORT_SYMBOLS as a list on the main CMakeLists.txt as:

set(EXPORTSYMBOLS
      -Wl,-export-dynamic
      -Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exportmap.gcc) 

add that to the OS_INTERFACE:

  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
    # This is only relevant for GCC and causes warnings on Clang
    target_link_options(
      OS_INTERFACE
      INTERFACE
      -Wno-tsan -Wl,-z,relro,-z,now
      ${EXPORTSYMBOLS}
    )

and here instead of this foreach loop, use this one:

foreach(symbol ${EXPORTSYMBOLS})
    list(REMOVE_ITEM os_link_options ${symbol})
endforeach()

[kheaactua]

This would make it simpler. I'll give it a try

[kheaactua]

I do think it works better. Fixed in bf943c5

[kheaactua]

The build error is weird.. Though I do get a similar one locally:

Run on (40 X 4800 MHz CPU s)
CPU Caches:
  L1 Data 48 KiB (x20)
  L1 Instruction 32 KiB (x20)
  L2 Unified 2048 KiB (x20)
  L3 Unified 38400 KiB (x1)
Load Average: 2.77, 3.31, 3.34
***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
-----------------------------------------------------------------------------------------------------------------
Benchmark                                                                       Time             CPU   Iterations
-----------------------------------------------------------------------------------------------------------------
BM_check_credentials_policies_not_loaded                                     2.82 ns         2.82 ns    265491910
BM_check_credentials_policies_loaded_invalid_values                          2.66 ns         2.66 ns    259855302
BM_check_credentials_policies_loaded_valid_values                            3.09 ns         3.09 ns    243618071
BM_check_credentials_policies_loaded_audit_mode_invalid_values               3.17 ns         3.17 ns    221014699
BM_check_credentials_policies_loaded_audit_mode_valid_values                 3.12 ns         3.12 ns    227970630
BM_check_routing_credentials_policies_not_loaded                              399 ns          399 ns      1713574
BM_check_routing_credentials_policies_loaded_invalid_values                   407 ns          407 ns      1777926
BM_check_routing_credentials_policies_loaded_valid_values                     409 ns          409 ns      1706650
BM_check_routing_credentials_policies_loaded_lazy_load_invalid_values         399 ns          398 ns      1760846
BM_check_routing_credentials_policies_loaded_lazy_load_valid_values           403 ns          403 ns      1712075
BM_get_client_to_sec_client_mapping_valid_values                             17.0 ns         17.0 ns     41920684
BM_get_client_to_sec_client_mapping_invalid_values                           14.2 ns         14.2 ns     49793167
BM_get_clients                                                               32.7 ns         32.7 ns     21492018
BM_get_sec_client_to_clients_mapping_valid_values                            35.1 ns         35.1 ns     19821518
BM_get_sec_client_to_clients_mapping_invalid_values                          18.8 ns         18.8 ns     36769476
BM_is_client_allowed_policies_not_loaded                                     3.18 ns         3.18 ns    218177708
BM_is_client_allowed_policies_loaded_valid_values                            2.90 ns         2.90 ns    241320837
BM_is_client_allowed_cache_policies_loaded                                   2.89 ns         2.89 ns    241970142
BM_is_client_allowed_policies_loaded_invalid_values                          2.99 ns         2.99 ns    234260981
BM_is_client_allowed_policies_loaded_deny_valid_values                       2.96 ns         2.96 ns    231424899
BM_is_client_allowed_policies_loaded_audit_mode_valid_values                 2.91 ns         2.91 ns    239781608
BM_is_client_allowed_cache_policies_loaded_audit_mode                        2.90 ns         2.90 ns    240085452
BM_is_client_allowed_policies_loaded_audit_mode_invalid_values               2.94 ns         2.94 ns    234957408
BM_is_client_allowed_policies_loaded_audit_mode_deny_valid_values            3.19 ns         3.19 ns    217727636
BM_is_offer_allowed_policies_not_loaded                                      3.49 ns         3.49 ns    195235638
BM_is_offer_allowed_policies_loaded_valid_values                             3.51 ns         3.51 ns    199208643
BM_is_offer_allowed_policies_loaded_invalid_values                           3.52 ns         3.52 ns    200094570
BM_is_offer_allowed_policies_loaded_deny_valid_values                        3.55 ns         3.55 ns    199501975
BM_is_offer_allowed_policies_loaded_audit_mode_valid_values                  3.52 ns         3.52 ns    200815148
BM_is_offer_allowed_policies_loaded_audit_mode_invalid_values                3.55 ns         3.55 ns    200287792
BM_is_offer_allowed_policies_loaded_audit_mode_deny_valid_values             3.56 ns         3.56 ns    196855102
terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check: __n (which is 0) >= this->size() (which is 0)
zsh: IOT instruction (core dumped)

Not sure what the cause is, will investigate.