Take advantage of REP-0140 tag changes

[wjwwood]

This is obviously blocked on the ratification of REP-0140 and its implementation.

[dirk-thomas]

The necessary infrastructure has been rolled out by now. So this should not be blocked anymore.

[NikolausDemmel]

With format 2 in the package.xml of catkin_simple itself the <run_depend>catkin</run_depend> could become a buildtool_export_depend, right?

[wjwwood]

Yes, I believe that's right.

[NikolausDemmel]

Looking at ros/rosdistro#3460, the only real change to catkin_make for package format 2 was ros/catkin#612, it seems.

In order to address this issue, I would start with updating the following script to contain the format 2 dependency specifications, right?
https://github.com/ros/catkin/blob/kinetic-devel/cmake/parse_package_xml.py

[wjwwood]

@NikolausDemmel yes, you'd need to make it possible to access the more granular versions of the dependencies, if they're available. That file is probably the right place to start.

Thanks.

[NikolausDemmel]

What should catkin_simple actually do with this information?

AFAICT, catkin simple currently only uses RUN_DEPENDS to determine what to add to the catkin_package call:

catkin_simple/cmake/catkin_simple-extras.cmake.em

Lines 211 to 225 in 0e62848

	set(${PROJECT_NAME}_CATKIN_RUN_DEPENDS)
	foreach(dep ${${PROJECT_NAME}_RUN_DEPENDS})
	find_package(${dep} QUIET)
	if(${dep}_FOUND_CATKIN_PROJECT)
	list(APPEND ${PROJECT_NAME}_CATKIN_RUN_DEPENDS ${dep})
	endif()
	endforeach()
	catkin_package(
	INCLUDE_DIRS ${${PROJECT_NAME}_LOCAL_INCLUDE_DIR} ${CS_PROJECT_INCLUDE_DIRS}
	LIBRARIES ${${PROJECT_NAME}_LIBRARIES} ${CS_PROJECT_LIBRARIES}
	CATKIN_DEPENDS ${${PROJECT_NAME}_CATKIN_RUN_DEPENDS} ${CS_PROJECT_CATKIN_DEPENDS}
	DEPENDS ${CS_PROJECT_DEPENDS}
	CFG_EXTRAS ${CS_PROJECT_CFG_EXTRAS}
	)

I guess we would want to change that to consider only BUILD_EXPORT_DEPENDS and add those as CATKIN_DEPENDS/DEPENDS in catkin_package.

What about BUILDTOOL_EXPORT_DEPENDS? Should those appear in catkin_package?

One more thing (that is not really specific to format 2): Shouldn't catkin_simple check if a build_export_depend is a catkin package, or not, and add it to CATKIN_DEPENDS or DEPENDS accordingly? At the moment, it adds all of them to CATKIN_DEPENDS.

Actually, catkin itself should probably also be updated to consider BUILD_EXPORT_DEPENDS, and not RUN_DEPENDS, right? See: https://github.com/ros/catkin/blob/fd229014461c5d494c721f8dbc51857bdeb51e41/cmake/catkin_package.cmake#L215

[NikolausDemmel]

With format 2 in the package.xml of catkin_simple itself the <run_depend>catkin</run_depend> could become a buildtool_export_depend, right?
Yes, I believe that's right.

I believe catkin is not equipped to handle that yet. I get the following:

CMake Error at /opt/ros/indigo/share/catkin/cmake/catkin_package.cmake:217 (message):
  catkin_package() DEPENDS on the catkin package 'catkin' which must
  therefore be listed as a run dependency in the package.xml
Call Stack (most recent call first):
  /opt/ros/indigo/share/catkin/cmake/catkin_package.cmake:98 (_catkin_package)
  CMakeLists.txt:6 (catkin_package)

Actually, catkin itself should probably also be updated to consider BUILD_EXPORT_DEPENDS, and not RUN_DEPENDS, right? See: https://github.com/ros/catkin/blob/fd229014461c5d494c721f8dbc51857bdeb51e41/cmake/catkin_package.cmake#L215

To fix the above error, catkin would need to consider both BUILD_EXPORT_DEPENDS and BUILDTOOL_EXPORT_DEPENDS. Does that sound right?

[dirk-thomas]

There is a big difference between the tags in the manifest file and the API of catkin_pkg. The manifest can be format 1 or format 2. But catkin_pkg always provides the exact same API.

The data storage within catkin_pkg is based on the format 2 dependencies because they are the more fine grain specification: https://github.com/ros-infrastructure/catkin_pkg/blob/6d79544f7fa4b646f7574910d085806de393bb46/src/catkin_pkg/package.py#L63-L69

But the API also provide the convenience of the format 1 style dependencies - so run_depends are also accessible: https://github.com/ros-infrastructure/catkin_pkg/blob/6d79544f7fa4b646f7574910d085806de393bb46/src/catkin_pkg/package.py#L108-L113

In the other direction if a format 1 manifest is being read its generic run dependencies is interpreted as the more fine grain build_export and exec deps: https://github.com/ros-infrastructure/catkin_pkg/blob/6d79544f7fa4b646f7574910d085806de393bb46/src/catkin_pkg/package.py#L95-L99

So whenever you CMake code is only interested in the set of run dependencies (and doesn't care what specific subtype in format 2 they are) it can simply use RUN_DEPENDS.

To summarize catkin is currently perfectly able to handle format 1 as well as format 2 correctly. It only doesn't provide variables for the detailed format 2 dependencies. But it does provide the values of the format 2 dependencies through the format 1 CMake variables.

[NikolausDemmel]

To summarize catkin is currently perfectly able to handle format 1 as well as format 2 correctly. It only doesn't provide variables for the detailed format 2 dependencies. But it does provide the values of the format 2 dependencies through the format 1 CMake variables.

Sure. That doesn´t mean* it takes advantage of the new tags added with format 2 in all cases though.

The following should still be true, as far as I understand it. Could you comment? If we agree, I can open a PR on catkin.

Actually, catkin itself should probably also be updated to consider BUILD_EXPORT_DEPENDS, and not RUN_DEPENDS, right? See: https://github.com/ros/catkin/blob/fd229014461c5d494c721f8dbc51857bdeb51e41/cmake/catkin_package.cmake#L215

To fix the above error, catkin would need to consider both BUILD_EXPORT_DEPENDS and BUILDTOOL_EXPORT_DEPENDS. Does that sound right?

EDIT: *doesnt mean neccessarily

[dirk-thomas]

AFAICT, catkin simple currently only uses RUN_DEPENDS to determine what to add to the catkin_package call...
I guess we would want to change that to consider only BUILD_EXPORT_DEPENDS and add those as CATKIN_DEPENDS/DEPENDS in catkin_package.

catkin has no notion to distinguish build or buildtool dependencies. If you only pass build_export along that would mean buildtool_export has no effect at all. I am not sure if that makes any sense.

I believe catkin is not equipped to handle that yet. I get the following:
CMake Error at /opt/ros/indigo/share/catkin/cmake/catkin_package.cmake:217 (message):
catkin_package() DEPENDS on the catkin package 'catkin' which must
therefore be listed as a run dependency in the package.xml

I don't think this needs any change. buildtool_export is implicitly a run dependency so the check works find. Once CMake distinguishes between format 1 and format 2 it could explicitly require buildtool_export for format 2 though. But that is just an improvement and not necessary to work correctly. And again some packages might use format 2 already but have catkin in a different run dependency tag and they should continue to work.

[NikolausDemmel]

catkin has no notion to distinguish build or buildtool dependencies. If you only pass build_export along that would mean buildtool_export has no effect at all. I am not sure if that makes any sense.

So are you saying that as a general rule (w/ or w/o catkin simple) that a buildtool_export_depend should be exported in the catkin_package command?

Let me give an example to make sure I have understood correctly. Let's assume that in a foo_pkg with <buildtool_export_depend>bted_pkg</buildtool_export_depend> we add bted_pkg to CATKIN_DEPENDS in the catkin_package call. Now package bar_pkg has <buildtool_depends>foo_pkg</buildtool_depends> and subsequently calls find_package(foo_pkg). Now bted_pkg's exported variables (e.g. bted_pkg_LIBRARIES) are contained in foo_pkg
's exported variables (e.g. foo_pkg_LIBRARIES), which is what we want. Right?

---

buildtool_export is implicitly a run dependency

No, it does not seem to be. In the definition of run_depend it only combines build_export_depend and exec_depend, notbuildtool_export_depend`: https://github.com/ros-infrastructure/catkin_pkg/blob/6d79544f7fa4b646f7574910d085806de393bb46/src/catkin_pkg/package.py#L108-L113 ?

And I don't think it should be.

---

the check works fine

No it doesn't. As the buildtool_export does not appear in RUN_DEPENDS, we get the error message I pasted. This is of course assuming that what I asked initially in this thread is the right thing to do:

With format 2 in the package.xml of catkin_simple itself the <run_depend>catkin</run_depend> could become a buildtool_export_depend, right?

---

But that is just an improvement and not necessary to work correctly. And again some packages might use format 2 already but have catkin in a different run dependency tag and they should continue to work.

If I export a package with CATKIN_DEPEND, I should build_export_depend or buildtool_export_depend on it, right? If in a format 2 package I list it only as a exec_depend, currently catkin will not complain, since the exec_depend will be in RUN_DEPENDS, but catkin should in fact complain.

[NikolausDemmel]

the check works fine

No it doesn't. As the buildtool_export does not appear in RUN_DEPENDS, we get the error message I pasted. This is of course assuming that what I asked initially in this thread is the right thing to do:

ros/catkin#790 addresses this