cmake remove circular linking and reorganize #9292

dagar · 2018-04-12T01:21:17Z

PX4 is supposed to be modular, but because of the current cmake build system everything is built and linked together as a large pile of code. The lines between different modules and different layers of the system have become blurred in many cases.

This PR changes the PX4 cmake build slightly to allow us to actually enforce and maintain modularity. We can then tease apart some of these interconnected components and work towards more well defined layers (#8784 (comment)).

First instead of calling everything a module (px4_add_module) and linking it all together (with multiple passes to resolve circular dependencies) I changed px4_add_module to require a MAIN. Any modules included in a configuration (eg nuttx_px4fmu-v2_default.cmake) are included in the final PX4 binary. Modules without a main are now simply libraries (px4_add_module -> px4_add_library). Some libraries are part of PX4 and need access to uORB, the parameter system, logging system, etc (PX4 libraries: px4_add_library), but many don't (regular cmake: add_library).

Libraries aren't automatically included (linked) in your final PX4 binary, they must be added as a module dependency. As a result we don't need to list them per configuration. Including a particular module pulls in a library only if it's needed as a dependency. With that dependency comes additional include directories and build flags if specified by the library.

PX4 CMake Changes

px4_add_module (PX4 Modules)

now requires MAIN
access to uORB, params, and other libraries
name is defined for the logging system (PX4_INFO, PX4_WARN, PX4_ERR)
no access to other modules by default

px4_add_library (PX4 libraries)

access to PX4 uORB, params, etc
not linked to final binary by default, must be a dependency (DEPENDS) of a module
name is defined for the logging system (PX4_INFO, PX4_WARN, PX4_ERR)
use INTERFACE to specify include directories, build flags, etc that should be used by the module
can use other libraries (PX4 or not)

add_library

use for regular libraries that shouldn't have access to anything PX4 specific (uORB, params, etc)
can't use logging system (PX4_INFO, PX4_WARN, PX4_ERR)
not linked to final binary by default, must be a dependency (DEPENDS) of a module

cmake configurations

all libraries (src/lib) are automatically included in every build
only the ones that are dependencies of PX4 modules (px4_add_module DEPENDS) are actually built
other common pieces (eg src/drivers/boards/${BOARD}) are also included automatically now
this removes the need for every cmake configurations to exhaustively list every component and core system piece. Modules are specified directly, and then the libraries are pulled in as dependencies.
example, here's how the px4fmu-v2 configuration (nuttx_px4fmu-v2_default.cmake) is simplified.

This also removes the need to link with --start-group/--end-group to resolve the circular dependencies.

Notes

A few ideas taken from CppCon 2017: Mathieu Ropert “Using Modern CMake Patterns to Enforce a Good Modular Design”.
- declare all properties per target (PRIVATE, PUBLIC, or INTERFACE)
  - target_link_libraries for dependencies (somewhat poorly named, but very useful)
  - target_compile_defintions
  - target_compile_options
  - target_include_directories
- forbid/report circular and hidden dependencies
- build interfaces are transitive
- help developer reason at module level
Related issues and PRs move posix, nuttx, qurt components into platforms #8784 (comment), RFC: Directory structure refactoring #8127, Build overlay #7705, Core rearchitecture for modularity #7806, and PX4 Flight Stack Structure Proposal #7318

dagar · 2018-04-14T15:17:15Z

@Stifael you might be interested in some of this. Once this is in PX4 we can update ECL with something more modular that's also a proper standalone cmake library.

Stifael · 2018-04-16T06:51:24Z

CMakeLists.txt

 		set(PX4_BUILD_TYPE "MinSizeRel")
-	elseif (${OS} STREQUAL "bebop")
-		set(PX4_BUILD_TYPE "MinSizeRel")


I assume this is depreciated?

It's actually just wrong. ${OS} is never set to bebop.

Stifael · 2018-04-16T08:18:38Z

src/systemcmds/tune_control/CMakeLists.txt

@@ -38,6 +38,5 @@ px4_add_module(
 	SRCS
 		tune_control.cpp
 	DEPENDS
-		platforms__common
+		tunes


so much better

Stifael · 2018-04-16T08:29:48Z

src/modules/simulator/CMakeLists.txt

@@ -53,6 +53,8 @@ if (NOT ${OS} STREQUAL "qurt")
 		simulator_mavlink.cpp)
 endif()

+add_subdirectory(ledsim)
+
 px4_add_module(


would be nice to free ecl a from px4 dependencies such that it could be included similar to the simulator

Stifael · 2018-04-16T08:44:37Z

this makes the px4 cmake structure much more understandable

Stifael · 2018-04-16T08:45:25Z

this makes the px4 cmake structure much more understandable

dagar · 2018-04-17T03:44:59Z

Here's another good summary of modern cmake. https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1

- px4_add_module now requires MAIN - px4_add_library doesn't automatically link

- set BOARD_NUMBER_BRICKS to 0 for boards without analog power bricks

bkueng

Looks good as far as I can tell (proper cmake has remained a mystery to me so far).

There's an incremental build issue when removing an aiframe file & from CMakeLists.txt, you get:

ninja: error: '../../ROMFS/px4fmu_common/init.d/4040_reaper', needed by 'genromfs/px4fmu_common/init.d/rcS', missing and no known rule to make it

But it already exists on master.

Let's make sure get it in soon.

bkueng · 2018-04-25T09:44:25Z

src/modules/mc_pos_control/CMakeLists.txt

-# vim: set noet ft=cmake fenc=utf-8 ff=unix :
+		controllib
+		flight_tasks
+		git_ecl


Are both git_ecl and ecl needed?

At the moment yes. The git_ecl target is on the Firmware side and ensures the submodule is actually in place. Then ecl is the actual library in the ecl project. Mixing submodule dependencies into the build system like this is a bit unusual, but has worked surprisingly well.

Later we can split the ecl project into separate libs for ekf, tecs, etc that build standalone or within PX4.

Ok, it's just not obvious for someone adding a new module

dagar · 2018-04-25T14:33:47Z

Thanks for the review. Can you open an issue about any incremental build issues you hit?

bkueng · 2018-04-25T15:02:43Z

Can you open an issue about any incremental build issues you hit?

#9369

dagar force-pushed the pr-cmake_lib_module branch 6 times, most recently from 9d002bd to a026f9e Compare April 12, 2018 06:04

dagar changed the title ~~[WIP] cmake remove circular linking and reorganize~~ cmake remove circular linking and reorganize Apr 12, 2018

dagar force-pushed the pr-cmake_lib_module branch 5 times, most recently from 422a415 to 66161e9 Compare April 14, 2018 15:03

dagar added the Admin: Enhancement (improvement) 💡 label Apr 14, 2018

dagar added this to the Release v1.8.0 milestone Apr 14, 2018

Stifael reviewed Apr 16, 2018

View reviewed changes

dagar force-pushed the pr-cmake_lib_module branch 2 times, most recently from 8bacecc to b06b977 Compare April 17, 2018 14:17

dagar requested a review from bkueng April 17, 2018 14:17

dagar mentioned this pull request Apr 19, 2018

stop manually defining physical constants #9319

Merged

dagar force-pushed the pr-cmake_lib_module branch 2 times, most recently from 4005167 to 27debf9 Compare April 20, 2018 21:44

dagar added 2 commits April 21, 2018 12:51

cmake remove circular linking and reorganize

897bd6f

- px4_add_module now requires MAIN - px4_add_library doesn't automatically link

delete unused ADCSIM

f57f22c

- set BOARD_NUMBER_BRICKS to 0 for boards without analog power bricks

dagar force-pushed the pr-cmake_lib_module branch from 27debf9 to f57f22c Compare April 21, 2018 16:58

bkueng approved these changes Apr 25, 2018

View reviewed changes

dagar merged commit 8404889 into master Apr 30, 2018

dagar deleted the pr-cmake_lib_module branch April 30, 2018 01:48

mrpollo mentioned this pull request Jun 5, 2018

v1.8 Changelog scratchpad #9593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake remove circular linking and reorganize #9292

cmake remove circular linking and reorganize #9292

dagar commented Apr 12, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Apr 14, 2018

Stifael Apr 16, 2018

dagar Apr 16, 2018

Stifael Apr 16, 2018

Stifael Apr 16, 2018

Stifael commented Apr 16, 2018

Stifael commented Apr 16, 2018

dagar commented Apr 17, 2018

bkueng left a comment

bkueng Apr 25, 2018

dagar Apr 25, 2018

bkueng Apr 25, 2018

dagar commented Apr 25, 2018

bkueng commented Apr 25, 2018

cmake remove circular linking and reorganize #9292

cmake remove circular linking and reorganize #9292

Conversation

dagar commented Apr 12, 2018 • edited by AuterionWrikeBot Loading

PX4 CMake Changes

Notes

dagar commented Apr 14, 2018

Stifael Apr 16, 2018

Choose a reason for hiding this comment

dagar Apr 16, 2018

Choose a reason for hiding this comment

Stifael Apr 16, 2018

Choose a reason for hiding this comment

Stifael Apr 16, 2018

Choose a reason for hiding this comment

Stifael commented Apr 16, 2018

Stifael commented Apr 16, 2018

dagar commented Apr 17, 2018

bkueng left a comment

Choose a reason for hiding this comment

bkueng Apr 25, 2018

Choose a reason for hiding this comment

dagar Apr 25, 2018

Choose a reason for hiding this comment

bkueng Apr 25, 2018

Choose a reason for hiding this comment

dagar commented Apr 25, 2018

bkueng commented Apr 25, 2018

dagar commented Apr 12, 2018 •

edited by AuterionWrikeBot

Loading