Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REP 149: consolidate depends tag variations #257

Open
rotu opened this issue Apr 30, 2020 · 2 comments
Open

REP 149: consolidate depends tag variations #257

rotu opened this issue Apr 30, 2020 · 2 comments
Labels

Comments

@rotu
Copy link
Contributor

rotu commented Apr 30, 2020

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
Copy link
Member

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
Copy link
Contributor Author

rotu commented Apr 30, 2020

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 bc6c500, 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 ros2/rmw_cyclonedds#132, so please document the current spec better, regardless of any future changes.

@rotu rotu changed the title REP 149: repeated depends REP 149: consolidate depends tag variations May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants