doc_depend resolution + unit testing

[veimox]

Tackles #498 .

I have added a unit for it.

I am not an expert in all ROS infrastructure so i don't know the consequences of this change in other parts of the system. I have run manually bloom-generate rosdebian in a package and the <doc_depend> are not being injected as dependencies but I think this change has to be well reviewed because it might start creating dependencies where not desired 😕

[veimox]

The unit testing is broken due to the bug presented in the PR #647 . Once merged, the unit testing has to be run again in the PR.

[codecov-io]

Codecov Report

Merging #646 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #646   +/-   ##
=======================================
  Coverage   75.46%   75.46%           
=======================================
  Files          32       32           
  Lines        2967     2967           
=======================================
  Hits         2239     2239           
  Misses        728      728

Impacted Files	Coverage Δ
src/rosdep2/rospkg_loader.py	93.84% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c61b918...9cbf2fc. Read the comment docs.

[tfoote]

I merged this with the recent changes so it's now passing CI. However, we will need to audit where this will take effect.

@nuclearsandwich probably has a better idea of where this will effect.

I believe that bloom and the packaging pipeline will collect the dependencies directly not through this code path.

Since we have the different dependency types probably the best way to move forward on this would be to improve/extend the command line to be able to select the types of dependencies that you want for your use case. Otherwise we'll break some people's use cases while fixing others as some people will want doc depends and some will not.

[veimox]

IMHO if a person is using rosdep to resolve dependencies is because they need to work with the source code and as such, building documentation is as important as the rest of the dependencies. Thus, I don't think we should discriminate in rosdep the "target dependency type".

If we don't agree in the frist point, I guess you are proposing to have a multipurpose flag rather than a "--with-docs". If so, what about:

--dependency-type (-d) <type>

Types:

• exec (e): Execution deps
• build (b): Building deps
• docs (d): Docs deps
• test (t): Test deps

For expressing the type I can think of:

1. Assigning a number to it (like in chmod)
2. Combination of letters ("ebd" will be "exec, build and docs")
3. Comma separated string ("build,test")

Default option should be "exec, build and test" as of today, maybe we can discuss also the "docs".

[nuclearsandwich]

@nuclearsandwich probably has a better idea of where this will effect.

Hooboy this turned into a bit of a dive.

• Bloom selects its own dependencies from the parsed package.xml and resolves the individual packages via rosdep. (source example 🔗)

• ros_buildfarm also picks out situational dependencies for the different job types

• We don't make wide-use of doc_depend in ROS 2 at the moment but this change would affect the ROS 2 Linux archive instructions if any doc_depend keys are used there.

• While checking rospkg I actually noticed that this PR creates an inconsistency between two codepaths. This PR changes the resolution for packages in the catkin_pkg paths but if a package is in the loadable resources (I haven't determined what makes one path more likely than the other) then resolution is pushed to a get_rosdeps method on the _rospack member. Following the trail there gets me to this line which aggregates build, buildtool, run, and test dependencies. Again I'm not sure what situations will trigger these two codepaths but it seems to me that they should agree. Potentially through refactoring that eliminates the duplication.

That's everywhere this function turned up via regexp of the common tools or everywhere I've had contact with rosdep recently.

---

IMHO if a person is using rosdep to resolve dependencies is because they need to work with the source code and as such, building documentation is as important as the rest of the dependencies.

I understand your reasoning but I think that installing only build-time dependencies (with or without tests), when you want to build your own artifacts for deployment and don't need to build docs in the same process is a valid use-case. As is installing just exec-time dependencies or doc=time dependencies. The way our tools and tutorials do this currently is either by aggregating dependencies directly from the manifest and resolving just the individual packages with rosdep, or in the case of the ROS 2 Linux archive instructions, accepting the fact that you'll end up installing build dependencies along with exec dependencies even if you don't need them.

With the above in mind I think it's worth iterating toward flexibility rather than assumed use-cases. Your proposal for flags is a good step in this direction that I'm in favor of.

For expressing the type I can think of:

Of the proposed options a comma or space separated list is the one I'm most in favor of. There's no justification for having folks memorize a numeric encoding and initialisms already have conflicts between build, build_export, buildtool, buildtool_export.

rosdep install --from-paths src --ignore-src --rosdistro melodic --dependency-types build build_export doc

[veimox]

We agree then in an extension of the cli to support these use cases. I will use this PR so that, even though the name of the branch is not very appropriate, the conversation remains.

Discussion points:

1. I see the inconsistency and I think should be out of the scope of this PR.
2. The default value of the new param is: build, buildtool, exec and test (as of now).
3. The dependency types are: build, buildtool, exec, test and doc.
4. Format is space separated types

Once those points are closed I will start working on it 😃

[nuclearsandwich]

1. I see the inconsistency and I think should be out of the scope of this PR.

Since continued work will not change the default here, I agree.

I'd give other collaborators some time to weigh in but that plan seems fine to me.

[tfoote]

The proposed approach from @veimox sounds good to me.

[veimox]

Good! I will prioritize this :)

[nuclearsandwich]

@veimox are you still working on the updates to the command line interface?

There's a good discussion here but the current state and title of the PR don't reflect the conclusions and I think the scope of the changes probably warrants multiple pull requests. Would you want to open a tracking issue that summarizes your plan and close this PR in favor of it?

[nuclearsandwich]

Late update: I've just come across #727 which adds a command line option similar to the one discussed here. I'm going to link to the relevant parts of this discussion but review the approach there.

[veimox]

Sorry for lack of progress. I dont think I will continue this work so I propose to go with #727

[nuclearsandwich]

No worries. I appreciate the effort you put into formulating this PR and proposing the feature!