Restructure CMake project layout: make interacting with Artery's options and dependencies easier #382

AshFungor · 2025-08-08T20:52:23Z

I think we should reconsider our project CMake layout. I can mention the following issues from my personal experience:

Dependency management is quite chaotic and spread across all project, which makes it hard to reuse previously declared packages and extern projects (individual components may declare same find_package for protobuf, for example);
Options are also spread across all project and some configure-time decisions are performed via checking target existence (veins & inet), which may confuse people at first;
Some of our components do not have CMakeLists.txt at all, and instead are managed by CMakeLists.txt of a parent directory, that comes in conflict with CMake philosophy of delegating work to children nodes.

I propose:

Simple and expressive top-level CMakeLists file with find_packages, options and other project-wide configuration. Also, primary libs are declared at that scope;
External components are managed by /extern/CMakeLists, and each component may be turned off;
Artery::Core aggregates sources from directories for now;
Components manage no external projects or deps, just declare and patch existing targets.

riebl

All in all, I am in favour of this PR. I have major doubts, however, if separating "dependency finding" from "dependency consuming" is a good idea.

Edit: Up to now, I would say it is okay to have multiple "features" calling the same "find_package". But I agree, there is room for improvement.

CMakeLists.txt

riebl · 2025-08-08T21:00:16Z

src/ots/CMakeLists.txt

@@ -11,14 +11,8 @@ target_include_directories(ots PUBLIC $<TARGET_PROPERTY:core,INCLUDE_DIRECTORIES
 set_property(TARGET ots PROPERTY NED_FOLDERS ${CMAKE_CURRENT_SOURCE_DIR})
 set_property(TARGET ots PROPERTY OMNETPP_LIBRARY ON)

-find_package(PkgConfig MODULE REQUIRED)
-pkg_check_modules(ZEROMQ REQUIRED libzmq)


I am not convinced by this (and similar changes): Before this PR, the "find_package" part is co-located with the "consuming" part. By moving "find_package" away, we have only the usage of ZEROMQ_* variables left but it is not clear where they are defined.

riebl · 2025-08-08T21:01:58Z

src/artery/CMakeLists.txt

+add_subdirectory(networking)
+add_subdirectory(application)
+
+target_sources(core PRIVATE


I like this part handling sources of each subdirectory separately, nice!

To be honest, I wish we had src/artery/core directory and a number of static libs for each core component. I'm sort of terrified by the idea of patching numerous includes, but maybe :)

I agree, that separating each component in its own library would be a clean approach. I have considered this in the past but found no clear benefit.

CMakeLists.txt

AshFungor · 2025-08-08T21:45:51Z

cmake/GenerateOppMessage.cmake

+
+    # force cmake to generate message first, in case actual args_TARGET
+    # sources use results from custom_command (include message headers)
+    add_custom_target(dummy_${msg_name}_target DEPENDS "${msg_output_header}")


Actually, how it managed to work before? :)
There was no guarantee that cmake decides to execute custom_command and generate message source & header file before compiling any other source included to args_TARGET that uses it? I think that OUTPUTS dependency that is created by custom_command exists for target file only, which should imply that cmake may start building other args_target sources before it bumps into non-existent codegen file and invokes our custom_command.

I might be horribly wrong though. How it even happened that refactoring caused this...

Huh? Why is a dummy target introduced? The generated files are added as sources to the given TARGET and thus the generator command is pulled in by the respective target.
I had no trouble so far adding not-yet-existing sources to a target as long as a matching generation rule exists.

I removed dummy target so you can observe what happens in CI :)

On my machine, with ninja, I have the following rule to generate a file:

############################################# # Assume dependencies for generated source file. build /workspaces/artery/build/Debug/opp_messages/artery/utility/AsioData_m.cc: CUSTOM_COMMAND || cmake_object_order_depends_target_core restat = 1

Then, here comes object file:

build CMakeFiles/core.dir/opp_messages/artery/utility/AsioData_m.cc.o: CXX_COMPILER__core_unscanned_Debug /workspaces/artery/build/Debug/opp_messages/artery/utility/AsioData_m.cc || cmake_object_order_depends_target_core CONFIG = Debug DEFINES = ... DEP_FILE = CMakeFiles/core.dir/opp_messages/artery/utility/AsioData_m.cc.o.d FLAGS = -g -std=c++17 -fPIC INCLUDES = ... OBJECT_DIR = CMakeFiles/core.dir OBJECT_FILE_DIR = CMakeFiles/core.dir/opp_messages/artery/utility TARGET_COMPILE_PDB = CMakeFiles/core.dir/ TARGET_PDB = libartery_core.pdb

Finally, core target:

############################################# # Link the shared library libartery_core.so build libartery_core.so: CXX_SHARED_LIBRARY_LINKER__core_Debug ... CMakeFiles/core.dir/src/artery/utility/AsioScheduler.cc.o CMakeFiles/core.dir/src/artery/utility/AsioTask.cc.o CMakeFiles/core.dir/src/artery/utility/Channel.cc.o CMakeFiles/core.dir/src/artery/utility/Identity.cc.o CMakeFiles/core.dir/src/artery/utility/IdentityRegistry.cc.o CMakeFiles/core.dir/src/artery/utility/FilterRules.cc.o CMakeFiles/core.dir/src/artery/utility/Geometry.cc.o CMakeFiles/core.dir/opp_messages/artery/utility/AsioData_m.cc.o ... CONFIG = Debug DEP_FILE = CMakeFiles/core.dir/link.d LANGUAGE_COMPILE_FLAGS = -g LINK_FLAGS = -Wl,--dependency-file=CMakeFiles/core.dir/link.d LINK_LIBRARIES = ... LINK_PATH = -L/omnetpp/lib OBJECT_DIR = CMakeFiles/core.dir POST_BUILD = : PRE_LINK = : SONAME = libartery_core.so SONAME_FLAG = -Wl,-soname, TARGET_COMPILE_PDB = CMakeFiles/core.dir/ TARGET_FILE = libartery_core.so TARGET_PDB = libartery_core.pdb

You can see that AsioData_m.cc.o comes in the same line as Identity.cc.o (for example), which means that ninja might decide to build Identity.cc.o before AsioData_m.cc.o, when headers for AsioData_m.cc.o have not been generated yet.

CI run is here (for future reference): https://github.com/riebl/artery/actions/runs/16848441102/job/47731266081?pr=382

The same build configuration on master yields these:

############################################# # Custom command for opp_messages/artery/utility/AsioData_m.cc build opp_messages/artery/utility/AsioData_m.cc opp_messages/artery/utility/AsioData_m.h | ${cmake_ninja_workdir}opp_messages/artery/utility/AsioData_m.cc ${cmake_ninja_workdir}opp_messages/artery/utility/AsioData_m.h: CUSTOM_COMMAND /workspaces/artery/src/artery/utility/AsioData.msg /omnetpp/bin/opp_msgc || extern/libINET.so extern/libveins.so extern/vanetza/lib/libvanetza_access.so extern/vanetza/lib/libvanetza_asn1.so extern/vanetza/lib/libvanetza_asn1_its.so ... /* more shared libs */ COMMAND = cd /workspaces/artery/build/Debug/opp_messages/artery/utility && /omnetpp/bin/opp_msgc -s _m.cc -h /workspaces/artery/src/artery/utility/AsioData.msg DESC = Generating artery/utility/AsioData restat = 1

Also, buildorder rule exists:

build cmake_object_order_depends_target_core: phony || cmake_object_order_depends_target_INET cmake_object_order_depends_target_access ... /* more targets */ ... cmake_object_order_depends_target_gnss cmake_object_order_depends_target_net cmake_object_order_depends_target_security cmake_object_order_depends_target_traci cmake_object_order_depends_target_veins opp_messages/artery/utility/AsioData_m.cc opp_messages/artery/utility/AsioData_m.h

Ok, mystery resolved :)
If I move

generate_opp_message(AsioData.msg TARGET core)

To a different directory other than the one where original TARGET (Artery::core) is defined, it does not generate the command.

So, @riebl, what do you think? Should we force that dummy target to allow such cases, or just declare sub-core targets to actually make cmake generate a rule?

Come to think of it, we should probably adopt the idea of codegen target. This will be useful for linter in CI, and generally feels like a better approach (during development, you may invoke that target to re-generate or generate new auto-generated files)

https://cmake.org/cmake/help/latest/policy/CMP0171.html

Good catch! I would like to support Debian 12 as a baseline for the moment, which ships with CMake 3.25, i.e. without built-in codegen support.
Since the utility sub-directory contains non-optional sources, we may move their handling back to src/artery/CMakeLists.txt to avoid the issue. A follow-up PR may then add a codegen target.

AshFungor · 2025-08-08T22:07:47Z

But I agree, there is room for improvement.

Can you elaborate on this one, please?)
I think it might be a good idea to have our standalone components more visible in project structure. I mean, there probably should be a clear way of understanding if certain piece of project may configure dependencies or not, like:

src/artery/some-component - ok, directory is correct, there might be find_package there
src/artery/some-dir/some-component - no, not ok. Component discovery should be moved upwards

What do you think?

Or we may leave this one out, since there are no guidelines or best practices about it :)

riebl · 2025-08-09T08:20:16Z

Can you elaborate on this one, please?

Well, the built-in features of CMake are rather limited when it comes to managing projects with many optional features and thus variable dependencies. I have seen some projects adding their own dependency management on top. For Artery, I can imagine a lightweight variant.

Let me briefly sketch a possible approach:

Declare all dependencies in the root directory with a dedicated macro, e.g. declare_artery_dependency. The macro will look-up if the dependency is available quietly using find_package(... QUIET). If the dependency is found, it creates a target to be used by the consuming components.
The components invoke require_artery_dependency which will abort the CMake configuration step if the dependency is not available. We may also conditionally add subdirectories depending on the available dependencies, e.g. add_artery_subdirectory(dir DEPENDENCIES foo). We may even combine it with feature flags like WITH_TESTBED and bailing out with a useful message: add_artery_subdirectory(testbed DEPENDENCIES protobuf SWITCH WITH_TESTBED).

AshFungor · 2025-08-13T11:26:31Z

Hey! I think it's implemented now. I did not create declare_artery_dependency, though - CMake has no standard interface to interact with packages, so declaring a package tends to introduce different code each time.

riebl · 2025-08-15T08:32:54Z

CMakeLists.txt

+# Primary Artery targets #
+##########################
+
+# TODO: temporarily targets are moved into src/artery


Why temporarily? What is still "to-do" here?

I moved because of that generate_opp_message issue, but I wish to move them back once codegen target appears and there won't be any requirements for main targets' placement.

extern/CMakeLists.txt

CMakeLists.txt

AshFungor · 2025-08-15T15:36:20Z

We should probably also make Find* configs for some extern packages - this will help integrating them with our new approach to handle deps.

AshFungor · 2025-08-20T16:26:32Z

Hey, @riebl! Any updates on this one?)

cmake/GenerateOppMessage.cmake

cmake/AddArterySubdirectory.cmake

AshFungor · 2025-08-21T09:19:38Z

I also added all features to CI, current runs will fail since package rebuild is required.

riebl · 2025-08-24T20:41:50Z

Looks all good now, but I don't want do override the PR checks. Would you mind adding the required Dockerfile adjustments to another preparatory PR?

src/artery/CMakeLists.txt

AshFungor · 2025-08-25T14:50:10Z

After small adjustments to package config - CI should pass alright now. :)

AshFungor · 2025-08-25T16:08:16Z

Hey, @riebl!

Something odd happens to protobuf-dependent features (testbed and transfusion). I'll investigate that later alongside implementing global codegen target.

This PR has grown quite big :) Think we can merge it now.

AshFungor requested a review from riebl August 8, 2025 20:56

riebl reviewed Aug 8, 2025

View reviewed changes

AshFungor commented Aug 8, 2025

View reviewed changes

AshFungor self-assigned this Aug 9, 2025

AshFungor requested a review from riebl August 13, 2025 11:23

riebl reviewed Aug 15, 2025

View reviewed changes

extern/CMakeLists.txt Show resolved Hide resolved

riebl reviewed Aug 15, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

AshFungor requested a review from riebl August 15, 2025 15:36

riebl requested changes Aug 21, 2025

View reviewed changes

cmake/GenerateOppMessage.cmake Outdated Show resolved Hide resolved

cmake/AddArterySubdirectory.cmake Outdated Show resolved Hide resolved

AshFungor requested a review from riebl August 21, 2025 09:19

riebl reviewed Aug 25, 2025

View reviewed changes

src/artery/CMakeLists.txt Outdated Show resolved Hide resolved

AshFungor added 11 commits August 25, 2025 14:07

cmake: restructure project

4215cb0

scripts: remove debug message

9fb4f9d

cmake: fix misspellings, patch deps resolution

6c206df

add_opp_target: force build order

0f03fe7

cmake: set shared libs before extern inclusion

4e81f0d

add message: remove dummy target to demonstrate my point

4db4bc3

cmake: added function for components inclusion

6b626a0

cmake: add requires to add_subdirectory wrapper

e67fd2c

rebased refs

bb4565d

cmake: remove commented out experemental code

d3534d4

testbed: remove sea_v2x & protos from requires

d8d4d46

AshFungor force-pushed the different-cmake-layout branch from cb38424 to d8d4d46 Compare August 25, 2025 14:08

pkg-config: force prefix for libzmq

b4b0e43

AshFungor requested a review from riebl August 25, 2025 14:47

AshFungor added 5 commits August 25, 2025 15:08

ci: return ninja instead of make

63cffbd

ci: add line break

2e9f29c

transfusion: modernize protobuf consumption

4ad74d4

ci: remove transfusion for now

a4f71d8

ci: remove testbed for now

8156d3e

riebl approved these changes Aug 26, 2025

View reviewed changes

riebl merged commit 0e910ee into riebl:master Aug 26, 2025
3 checks passed

Restructure CMake project layout: make interacting with Artery's options and dependencies easier #382

Restructure CMake project layout: make interacting with Artery's options and dependencies easier #382

Uh oh!

Conversation

AshFungor commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riebl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AshFungor Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AshFungor Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AshFungor commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riebl commented Aug 9, 2025

Uh oh!

AshFungor commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AshFungor Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AshFungor commented Aug 15, 2025

Uh oh!

AshFungor commented Aug 20, 2025

Uh oh!

Uh oh!

Uh oh!

AshFungor commented Aug 21, 2025

Uh oh!

riebl commented Aug 24, 2025

Uh oh!

Uh oh!

AshFungor commented Aug 25, 2025

Uh oh!

AshFungor commented Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

AshFungor commented Aug 8, 2025 •

edited

Loading

riebl left a comment •

edited

Loading

AshFungor Aug 8, 2025 •

edited

Loading

AshFungor Aug 9, 2025 •

edited

Loading

AshFungor commented Aug 8, 2025 •

edited

Loading

AshFungor Aug 15, 2025 •

edited

Loading