Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cmake] Attach warnings to remaining targets #1954

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

DownerCase
Copy link
Contributor

Description

Follow up from #1948 -- makes sure all (C/C++) eCAL targets have the warnings set on them.

Detailed changes:

  • Rolls creation of the eCAL::* alias targets into the ecal_add_* helpers.
  • Fixed up PUBLIC/PRIVATE access specifiers on a few targets

I couldn't decide on the best way to handle the goal to make parts of the repo more standalone whilst also having a single source of truth. So ended up with three approaches.

  • For /app and /contrib I used the ecal_add_* helpers as they were already being used in places.
  • For /lib I used ecal_add_compiler_warnings if it was available, otherwise issued a warning.
  • For /ecal/service it appeared effort was made to make it standalone, so I check for if helper_functions/ecal_compiler_warnings can be included:
    • Yes: Great, use the _ecal_warnings target via a compatibility alias
    • No: Use duplicated logic
    • Either way, warnings are added by linking _ecal_service_warnings

Not sure what would be preferred, personally I would use the helpers everywhere and just carry those helper functions into the new split off repo.

Related issues

Cherry-pick to

@KerstinKeller KerstinKeller added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 27, 2025
@KerstinKeller
Copy link
Contributor

Thanks for making the effort!
There is one thing I am a bit hesitant about. So in general, things which might be changeable, should be introduced from the outside.

So I am wondering, if the approach of the warnings target is really the best in this case.
Most of CMakes variables are directory based, e.g. an add_subdirectory introduces a new scope, and changes to that variable are only visible in that scope.
So I am wondering, if the better way to go, is to use the CMake canonical variables CMAKE_CXX_FLAGS, but not set them in the main CMakeLists.txt file, but rather have files like ecal_windows.cmake, ecal_linux.cmake, which does set the flagse, but is included via CMake toplevel includes.

It would also allow to remove a lot of boilerplate code from main CMakeLists.txt.
Basically, a more lightweight option or enhancement over the CMakePresets.json... but I am just thinking here.

These files could be passed into anything which is build standalone...

@DownerCase
Copy link
Contributor Author

DownerCase commented Jan 27, 2025

Yep, so the problem with CMAKE_<LANG>_FLAGS is the submodule dependency provider as it will inject the dependency project at the site of the find_package call. So in any changes to the flags should be done after any external subprojects have been included into the build. In this project that would look like including all submodules we might need up-front, before doing anything else.

There are two things at play here.

Flags for all code including the dependencies, things like sanitizers are suited here as you want to know about problems from everywhere. Typically set at the command line / CMakePreset level with CMAKE_CXX_FLAGS

Flags the eCAL team want to apply to their code for development . I.e.: the compiler warnings. Things that shouldn't be pushed onto any in-build-tree dependencies. This is what I was attempting to address by adding the warning flags on a target-level.

There is a third viewpoint as well, packagers, who don't care about the projects development practices and just want the software to build. They are typically only concerned about the flags being used when it stops a build (i.e: don't set warnings as error by default!)


Having warnings explicitly on a target-by-target level also means you need not apply them to generated (or other) code (I'm looking at you, core_pb!) potentially diminishing the need of having to disable warnings on them.

Another advantage of warnings targets is that you could have more than one! I've never actually seen that done, but you could have, say, a baseline level and a strict one for critical code 🤷


These files could be passed into anything which is build standalone...

That was the idea with the include(... OPTIONAL) as developers/CI can set CMAKE_MODULE_PATH correctly and the CMake file can be source from anywhere.


I would say the design goals are:

  1. Have warning (and other) flags for eCAL code centrally managed
  2. Flags are not pushed to dependencies and users
  3. eCAL development and CI uses the flags
  4. The project builds without the flags (e.g: no library linking or include paths that are required for correct building)
  5. (Optional) The flags are opt-in, such that cmake -S . -B build does not include them

To achieve all that and incorporate your suggestion I propose continuing down the path as originally laid out, with the addition of replacing the flag selection logic (i.e: the if CMAKE_CXX_COMPILER_ID MATCHES ... stuff) to be a simple include(${ECAL_TOOLCHAIN_FLAGS}). ECAL_TOOLCHAIN_FLAGS (name not final) will be files like ecal_msvc.cmake or ecal_gcc.cmake that just sets some variables with the desired flags that are used in ecal_compiler_warnings.cmake.

ECAL_TOOLCHAIN_FLAGS would be set in CMakeLists (not presets as that causes combinatorial explosions 😞) by detecting the platform and compiler if, and only if, some other option set*.
If it is already set (i.e: by the user) it will prefer that value and skip auto-selection.

*This could be something like ECAL_DEVELOPER_MODE which also default enables things like tests.

EDIT: CMAKE_PROJECT_eCAL_INCLUDE could also work if you set that to a file that sets up "developer mode"


Sorry for the wall of text... 😅

Ps: This whole thing becomes moot if dependencies aren't built from source but come from Conan/vcpkg/system 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants