ros-infrastructure / rep

ROS Enhancement Proposals
http://www.ros.org/reps/rep-0000.html
149 stars 136 forks source link

REP 149: consolidate depends tag variations #257

Open rotu opened 4 years ago

rotu commented 4 years ago

The current way to specify a dependency needed for multiple build stages is with the separate tags <build_depend>, <exec_depend>, etc.

It seems more natural to have a way of way of specifying a package for multiple purposes in one declaration. Currently <depend> does this but only for a special case (<depend> is equivalent to <build_depend>, <build_export>, <exec> all at once).

I propose a for attribute on the depends tag which is a whitespace-separated list of one or more functional purposes, chosen from “build”, “build_export”, “buildtool”, “buildtool_export”, “exec”, “doc”, “test” (and possibly arbitrarily).

The default value of this attribute would be “build build_export exec” to preserve the semantics of existing depend tags. Thus the existing <build_depend> would become a synonym of <depend for=‘build’>.

This could be used with tools like rosdep as a natural way of filtering dependencies (e.g. rosdep install --from_path src --for build test doc).

dirk-thomas commented 4 years ago

The package manifest format provides only a single approach rather than multiple different alternatives. Using a single tag with attributes was considered as part of REP 127 and the alternative (separate tags) was chosen.

The need to repeat tags is pretty small overhead and also something only rarely changed. So I doubt that adding an alternative second approach is justified (also considering implementation, documentation and maintenance effort).

This could be used with tools like rosdep as a natural way of filtering dependencies

The described rosdep usage isn't related to this topic since it can easily implemented with the existing spec.

rotu commented 4 years ago

The package manifest format provides only a single approach rather than multiple different alternatives.

I would like to see <build_depend> etc. deprecated in the eventual future, and thus have a single, non-redundant approach, but that can be put off for a long time.

Using a single tag with attributes was considered as part of REP 127 and the alternative (separate tags) was chosen.

I think it was a worse choice and should be reconsidered. In fact, judging by the contemporaneous discussion, consensus was building that <depends scope="..."> was the way to go. I can't figure out why that idea was abandoned in https://github.com/ros-infrastructure/rep/commit/bc6c5009148f798fecd1fa48c8d39c55977f9c3d, except that @tfoote seemed to have a strong opinion.

At any rate, whether it's an element or an attribute, the scope is a property of the dependency. The two equivalent schemas to be consider should have been <depends><scope>build</scope><package>rclcpp</package></depends> versus <depends scope='build'>rclcpp</depends> Having many semantically similar <..._depends> tags seems a classic code smell - it's preferring duplication when composition is more appropriate.

The need to repeat tags is pretty small overhead and also something only rarely changed.

It seems that the repeated tag was enough of an issue between <build_depend> and <run_depend> that it motivated the <depend> tag in the first place, and reintroducing it in the package xml version 2!

So I doubt that adding an alternative second approach is justified (also considering implementation, documentation and maintenance effort).

Implementation and maintenance are pretty simple.

There definitely is already need for more documentation effort, judging by the confusion in the discussion here https://github.com/ros2/rmw_cyclonedds/pull/132, so please document the current spec better, regardless of any future changes.