catkin workspace cross-package target dependencies

1,233 views
Skip to first unread message

tkruse

unread,
Feb 7, 2013, 8:40:56 AM2/7/13
to ros-sig-b...@googlegroups.com
Hi,

(this is my second attempt to post this, as my first attempt seems to be AWOL.).

I became aware yesterday that catkin workspaces currently rely on users
defining cross-package dependencies to cmake targets. IMO that should be
cleaned up.

Assume we have two catkin packages, foo and bar, where bar depends on
foo. Meaning e.g. there is a cmake target bar_lib in bar's
CMakeLists.txt, and that target requires some file that gets generated
by project foo.

When foo and bar are build in isolation, first all of foo is build,
then bar uses find_package(foo) to locate the build artifacts, and can
build using them.

In a catkin workspace, foo and bar are build in parallel, meaning
there is a stated dependency, a target in bar can be build while a
target in foo has not finished building.

Two cases currently commonly exist in ROS, depending on binary
libraries (this is already handled by the target_link_libraries()
macro), and depending on message headers. Other cases may exist in the
future, so this is not strictly a message generation issue, though
message generation is the most obvious case.

So in a catkin workspace, before building bar_lib, all build artifacts
of foo that bar_lib requires have to be build. Currently the only way
to achieve this it to do something like this:

In bar/CMakeLists.txt:
add_dependencies(bar_lib foo_gencpp)

This looks innocent enough at a glance, but it is not to me. To me,
this is an invalid Cmakefile, and we should not require or encourage
user to write this. The problem is that bar should never need to know
the name of targets in another cmake project. This makes cmake
targetnames a new part of the public API of a catkin package, and
means packages start communicating with each other over cmake
targetnames. This is also confusing to cmake users as cmake projects
start to declare dependencies that are not defined in the project.

In isolated build, this should fail, really. Currently it does not,
due to a long-standing cmake bug, which may be fixed in the future one
way or the other:
http://www.cmake.org/Bug/print_bug_page.php?bug_id=9188
The bug means that if you write
add_dependencies(bar_lib some_target_never_heard_of)
with a clearly invalid dependency, cmake will never fail.
This means a typo in there can lead to builds failing
infrequently due to race conditions and such.

IMO, this should be fixed in catkin. there are several thinkable
solutions. A very clean one would be if in bar, targets would not
depend on mere targets, but on filenames, such as the names of the
headers in message_generation (which are already part of the public
API, as you write those into your #include statements). For
this, catkin should provide the convenience necessary.

If such a solution is not possible or too clumsy, then
add_dependencies() should at least be wrapped and tucked away so that
users never have to write cross-package dependencies. So catkin should
then generate an infrastructure,such that when I find_package(foo),
this call declares new targets for build artifacts of foo that the
user can use, which depend on targets in foo only if this
find_package(foo) call is done in the context of a catkin_workspace
where foo is among the packages of the build space (only then is it
valid for catkin to create cross-target dependencies).

I assume any fix is going to be a lot of effort unless someone here
has a bright idea I did not come up with.

So what do other users think, am I too harsh in my judgement and we
can just keep the design as it is until cmake fixes its bug, or should
we fix this?

regards,
  Thibault


PS: related to this question:
http://answers.ros.org/question/52744/how-to-specify-dependencies-with-foo_msgs-catkin-packages/

Vincent Rabaud

unread,
Feb 7, 2013, 10:09:01 AM2/7/13
to ros-sig-b...@googlegroups.com
So a few things:
1) I agree add_dependencies(bar_lib foo_gencpp) is not future proof
2) I agree add_dependencies(bar_lib foo_gencpp) is confusing for the user (it is indeed weird to have to be aware of the inside of another package but then again, good docs can solve that).
3) I don't think adding a dependency on a per file basis is a solution: there could be a lot of generated headers and headers are definitely not the only use case: in one of my cases, I am generating boost python modules that I then use in a Python script that CMake calls (yeah, it's twisted).

So for 1), why don't we add an option in the catkin macro:
IMPORTED my_library
That will make sure that whenever that package is defined as a CATKIN_DEPENDS in another package, the following is called add_library(my_library SHARED IMPORTED) (and probably also define IMPORTED_LOCATION for that library). This way, the target is made local and that should solve the isolated build case/future proof-ness.

for 2), I I don't know. We could create a special target "my_package_imported" that lists all the imported targets (_gencpp, _genpy ....) and people only have to do an add_dependency to that one target. I like the current individual targets as they give finer granularity obviously, but we already have packed data like in the catkin_LIBRARIES, catkin_INLUDE_DIRS variables so why not one last time ? :)





--
You received this message because you are subscribed to the Google Groups "ROS Buildsystem Special Interest Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsy...@googlegroups.com.
To post to this group, send email to ros-sig-b...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msg/ros-sig-buildsystem/-/QtXUOf7EOhQJ.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

tkruse

unread,
Feb 8, 2013, 4:50:27 AM2/8/13
to ros-sig-b...@googlegroups.com
Hi Vincent,

thanks for taking the time.

Regarding your second suggestion, there already is (in genmsg head):

..._generate_messages_cpp -> same as ..._gencpp
..._generate_messages_py -> same as ..._genpy
..._generate_messages_lisp -> same as ..._genlisp

..._generate_messages all of the above

But that does not help avoiding cross-package dependencies.

A admit that I don't yet understand the first solution you suggested, as I have never worked with imported library targets. I'll have to play around with that to understand how it works, but thanks for the pointer.

cheers,
  Thibault
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsystem+unsub...@googlegroups.com.

Jonathan Bohren

unread,
Feb 10, 2013, 2:55:14 PM2/10/13
to ros-sig-buildsystem

So what's the point of forbidding catkin/ros specific macros if we're just going to require catkin/ros specific CMake usage patterns? Especially if they just involve a ton of boilerplate code?

-j

To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsy...@googlegroups.com.

To post to this group, send email to ros-sig-b...@googlegroups.com.

tkruse

unread,
Feb 10, 2013, 5:00:19 PM2/10/13
to ros-sig-b...@googlegroups.com
Hi Jon,

we tried collecting the design goals that led to the catkin design in the review of catkin_fuerte (http://ros.org/wiki/catkin/Reviews/2012-08-01_API_Review) that I started when I did not understand the catkin design. Goals ctk_G6 and ctk_G8 justify refusing to provide catkin convenience wrapper macros where cmake functionality exists.
  • ctk_G6: Less custom tool maintenance effort than rosbuild
  • ctk_G8: use existing tools as much as possible, be as little invasive as possible, work well together with other projects
I believe that those design goals were the main motivator for leaving out convenience macros, though other arguments may exist.

Specific user patterns and tons of boilerplate code do not violate those design goals (as much as custom macros would), I guess.

Note that a refusal to provide such convenience macros with catkin is not "forbidding" users to use such macros from other sources.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsystem+unsubscribe...@googlegroups.com.

To post to this group, send email to ros-sig-b...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msg/ros-sig-buildsystem/-/QtXUOf7EOhQJ.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--
You received this message because you are subscribed to the Google Groups "ROS Buildsystem Special Interest Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsystem+unsub...@googlegroups.com.
To post to this group, send email to ros-sig-b...@googlegroups.com.

Dirk Thomas

unread,
Feb 21, 2013, 9:31:38 PM2/21/13
to ros-sig-b...@googlegroups.com
I will first comment on some of the "proposed ideas" and than summaries the (rare) options.


On 07.02.2013 07:09, Vincent Rabaud wrote:
> So a few things:
> 1) I agree add_dependencies(bar_lib foo_gencpp) is not future proof

Yes, specifying a target which potentially does not exist is bad.


> 2) I agree add_dependencies(bar_lib foo_gencpp) is confusing for the user (it is indeed weird to have to be aware of the inside of another package but then again, good docs can solve that).

Yes, an improved name of the target (i.e. messagepackage_generate_messages_cpp) and good docs might help.


> 3) I don't think adding a dependency on a per file basis is a solution: there could be a lot of generated headers and headers are definitely not the only use case: in one of my cases, I am generating
> boost python modules that I then use in a Python script that CMake calls (yeah, it's twisted).

CMake does not allow you to depend on the header files.
(More on that approach below)


> So for 1), why don't we add an option in the catkin macro:
> IMPORTED my_library
> That will make sure that whenever that package is defined as a CATKIN_DEPENDS in another package, the following is called add_library(my_library SHARED IMPORTED) (and probably also
> define IMPORTED_LOCATION for that library). This way, the target is made local and that should solve the isolated build case/future proof-ness.

The *_gencpp target is not a library and can therefore not be imported.


> for 2), I I don't know. We could create a special target "my_package_imported" that lists all the imported targets (_gencpp, _genpy ....) and people only have to do an add_dependency to that one
> target. I like the current individual targets as they give finer granularity obviously, but we already have packed data like in the catkin_LIBRARIES, catkin_INLUDE_DIRS variables so why not one last
> time ? :)

Yes, a combined target name (i.e. messagepackage_generate_messages) works (and is available in genmsg branch) but it does not solve the problem from 1).
Also if the documentation teaches the user that he has to specify that it should directly mention the language specific target name as it improves performance.


So where are we?

We don't want to build all targets of package "foo" before any target of package "bar".
That would be significantly slower and anyway it is not possible in CMake since catkin does not "know" what target "bar" defines (without overriding all relevant CMake commands).

We can not depend on the header files.
I implemented a small prototype where each message package provides a variable containing all generated files.
But CMake only allows depending on files which are output of custom command in the same folder / CMakeLists.txt.
But in our case the messages are generated in another package so CMake will not allow you to write something like:
add_library(my_lib my_cpp_files.cpp)
add_custom_target(my_lib_required_generated_files DEPENDS ${std_msgs_GENERATED_FILES_cpp})
add_dependencies(my_lib my_lib_required_generated_files)

The only way I can see is having an explicit dependency between the library/executable target and the "pseudo" target with generated the messages.
(Actually behind the scene every message has its own target.)
Since the targets may not exist each dependency must be added conditionally:
if(TARGET msgpkg_gencpp)
add_dependencies(mytarget msgpkg_gencpp)
endif()


What are the options?

(1) give the targets better names (already done in genmsg branch: _gencpp -> _generate_messages_cpp)

(2) always define the targets (this can be generated into a CFG_EXTRA file for each message package), which avoids the check if the target exists.
The downside of that is that a make target exists which does not do anything.
Consider calling cmake on a package and make autocompletion shows target names from a dependent package.

(3) provide a function which encapsulates the check for the target existence and add_dependencies() call.
I.e. add_message_dependencies_cpp(mytarget msgpkgA msgpkgB) would expand to:
if(TARGET msgpkgA_gencpp)
add_dependencies(mytarget msgpkgA_gencpp)
endif()
if(TARGET msgpkgB_gencpp)
add_dependencies(mytarget msgpkgB_gencpp)
endif()

(4) let the user write the initially mentioned code (check if target exists + add_dependencies)

Please feel free to propose other options with a detailed description how they are supposed to work.
(Stating that we "should just depend on the header files instead of non-existing targets" does not help if you don't provide an example how that should be realized in CMake.)


IMHO:
(1) is independent of the rest and should be done anyway.
(2) introduces "bad" target names which don't belong to a package and should not be realized.
(4) would be my preferred approach.
(3) I would consider it unnecessary wrapping of existing CMake functionality but could be convinced that we should still do it.

- Dirk

Tully Foote

unread,
Feb 21, 2013, 11:03:43 PM2/21/13
to ros-sig-b...@googlegroups.com
Although 3 adds a command, it does make the users code noteably easier to read. I think that it's worth doing that. 

Tully 


- Dirk
--
You received this message because you are subscribed to the Google Groups "ROS Buildsystem Special Interest Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsystem+unsub...@googlegroups.com.
To post to this group, send email to ros-sig-buildsystem@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


tkruse

unread,
Feb 22, 2013, 4:51:17 AM2/22/13
to ros-sig-b...@googlegroups.com


On Friday, February 22, 2013 3:31:38 AM UTC+1, Dirk Thomas wrote:
We don't want to build all targets of package "foo" before any target of package "bar".
That would be significantly slower and anyway it is not possible in CMake since catkin does not "know" what target "bar" defines (without overriding all relevant CMake commands).

Some people may value consistency and convenience over a speed improvement.
What you are saying could be rephrased as:
By overriding (wrapping) relevant Cmake commands, catkin could achieve this at the cost of some speed for for make commands.
I think now that the ROS platform development goes from Willow Garage to OSRF, maybe the community could be involved a bit more in the decision making than before.

(3) provide a function which encapsulates the check for the target existence and add_dependencies() call.
     I.e. add_message_dependencies_cpp(mytarget msgpkgA msgpkgB) would expand to:
       if(TARGET msgpkgA_gencpp)
         add_dependencies(mytarget msgpkgA_gencpp)
       endif()
       if(TARGET msgpkgB_gencpp)
         add_dependencies(mytarget msgpkgB_gencpp)
       endif()

Like Tully I find such a solution most acceptable in principle, however please
remember that this problem also occurs outside message
generation (Vincent gave one example).

So IMO the naming has to be more generic, and packages
to depend upon need a generic way of exporting targets that way.

I may add fromour last meeting that I brough t up the issue of inconsistent builds. IN the meeting,my opinion that we should strive for consistentency builds where possible, and in this case strive to generate allmessage bindings not just the c++ ones in such a case was overruled by "Inconsistent builds are part of what happens with make, so there is no point in even trying to reduce inconsistent builds, if it we gain speed by being inconsistent" or something like that.

Instead of the given solution that is restricted to message genration and cpp dependencies above I'd rather have packages declare dependency sets in an explicit way,
and those be used by depending packages in an explicit but convenient, way such as the macro sketched above.

Dirk Thomas

unread,
Feb 25, 2013, 8:27:55 PM2/25/13
to ros-sig-b...@googlegroups.com
After going through the posted feedback the sketch for stating cross-package target dependencies is:

a package can "export" target names via catkin_package(CODE_GENERATION_TARGETS xxx).
When that package is find_package()-ed it populates the variable "pkgname_CODE_GENERATION_TARGETS" with these target names.

Other macros (i.e. generate_messages() in genmsg) can prepopulate that variable so that the user does not have to export these target explicitly.
When building a library or executable which requires generated code the CMake file states that dependency with add_dependencies(mytarget ${xxx_CODE_GENERATION_TARGETS}).

As usually the values for multiple packages get aggregated as catkin_CODE_GENERATION_TARGETS when multiple components are found together.

catkin_package() will error out if any implicitly or explicitly specified target does not exist.
The generated CMake config files for these packages guarantee that the targets are available even if they have been build in a separate CMake context than the target are empty.

- Dirk

Vincent Rabaud

unread,
Feb 25, 2013, 8:38:04 PM2/25/13
to ros-sig-b...@googlegroups.com
+1
That's exactly the same workflow as catkin_INCLUDE_DIRS and so on so it's trivial to understand/apply. It seems it answers all the original concerns too and no new macro got created.

I am not crazy about the name though as code generation implies it's really just headers/python code but it could be a config/CMake file for example. How about using the IMPORTED from add_library/add_executable in CMake: pkgname_IMPORTED _TARGETS (and catkin_package has the keyword: EXPORTED_TARGETS)


--
You received this message because you are subscribed to the Google Groups "ROS Buildsystem Special Interest Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsystem+unsub...@googlegroups.com.
To post to this group, send email to ros-sig-buildsystem@googlegroups.com.

Tully Foote

unread,
Feb 25, 2013, 9:05:37 PM2/25/13
to ros-sig-b...@googlegroups.com
+1

I too find the name a little bit awkward.  How about simply GENERATED_TARGETS implying they're not written, but they're not necessarily generated code.  

Tully


To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsy...@googlegroups.com.
To post to this group, send email to ros-sig-b...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.
 
 

tkruse

unread,
Feb 26, 2013, 6:15:00 AM2/26/13
to ros-sig-b...@googlegroups.com
Just to confirm whether get this right:
For message generation this would bundle all message generation targets of a package into a single target, such that in:
add_dependencies(mytarget ${nav_msgs_CODE_GENERATION_TARGETS).
mytarget will only be build once all message generators are done. Will this also become the recommended practice and the standard for released packages?

Else +1, with a slight name change as Vincent or Tully suggested.

Dirk Thomas

unread,
Feb 26, 2013, 3:25:01 PM2/26/13
to ros-sig-b...@googlegroups.com
On 26.02.2013 03:15, tkruse wrote:
> Just to confirm whether get this right:
> For message generation this would bundle all message generation targets of a package into a single target, such that in:
> add_dependencies(mytarget ${nav_msgs_CODE_GENERATION_TARGETS).

No, this is not a single target.
The variable only contains a list of target names.
I.e. std_msgs_CODE_GENERATION_TARGETS will be: "std_msgs_generate_messages_cpp;std_msgs_generate_messages_lisp;std_msgs_generate_messages_py".

Both ways are valid use cases: adding dependencies on explicit target names like "std_msgs_generate_messages_cpp" or using the new variable for convenience.
(The "old" suffixed like "_gencpp" still exist for BC and are identical to the corresponding verbose name.)


> mytarget will only be build once all message generators are done.

Yes.


> Will this also become the recommended practice and the standard for released packages?

It will be the recommended practice.
But the side effect (less performance) will be stated clearly.
The doc should also mention the target names behind that variable so that users can use them at will.


> Else +1, with a slight name change as Vincent or Tully suggested.

The variable has been renamed to EXPORTED_TARGETS.

- Dirk


> On Tuesday, February 26, 2013 2:27:55 AM UTC+1, Dirk Thomas wrote:
>
> After going through the posted feedback the sketch for stating cross-package target dependencies is:
>
> a package can "export" target names via catkin_package(CODE_GENERATION_TARGETS xxx).
> When that package is find_package()-ed it populates the variable "pkgname_CODE_GENERATION_TARGETS" with these target names.
>
> Other macros (i.e. generate_messages() in genmsg) can prepopulate that variable so that the user does not have to export these target explicitly.
> When building a library or executable which requires generated code the CMake file states that dependency with add_dependencies(mytarget ${xxx_CODE_GENERATION_TARGETS}).
>
> As usually the values for multiple packages get aggregated as catkin_CODE_GENERATION_TARGETS when multiple components are found together.
>
> catkin_package() will error out if any implicitly or explicitly specified target does not exist.
> The generated CMake config files for these packages guarantee that the targets are available even if they have been build in a separate CMake context than the target are empty.
>
> - Dirk
>
> --
> You received this message because you are subscribed to the Google Groups "ROS Buildsystem Special Interest Group" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to ros-sig-buildsy...@googlegroups.com.
> To post to this group, send email to ros-sig-b...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msg/ros-sig-buildsystem/-/-86MxXP56LgJ.
Reply all
Reply to author
Forward
0 new messages