ros-infrastructure / rosdoc2

Command-line tool for generating documentation for ROS 2 packages.
Apache License 2.0
29 stars 9 forks source link

List meta dependencies in meta packages #114

Open rkent opened 2 months ago

rkent commented 2 months ago

This one might be a little more controversial, but generally meta packages have little documentation. The only thing interesting is the list of packages, so we show them. I assume that no package.build_depends but existing package.exec_depends defines a meta package. Is that correct?

I changed the default --base-url value because I test existing packages with simply rosdoc2 build -p . and I wanted real meta packages to have links that could be tested. I tried it with 'soccer_interfaces' and it worked OK.

clalancette commented 2 months ago

I assume that no package.build_depends but existing package.exec_depends defines a meta package. Is that correct?

Unfortunately, I don't think we can make that assumption. Pure python packages, for instance, have no build_depends but exec_depends. Further, I don't even think we can use build_type as ament_cmake to distinguish this case, as it is possible to have pure python packages that are installed via CMake.

We do have an existing tag called metapackage that packages can put in their export section. But we do not enforce that flag is there, and in point of fact none of the core ROS 2 metapackages actually define that flag currently.

tfoote commented 2 months ago

We have basically deprecated the metapackage as a formal designation. I think that the approach that we should look for on this is that the the full package.xml content is easily discoverable and viewable, independent of whether it's a package or a pseudo metapackge. The dependency list is just one element of that. The links added in #113 and the description in #112 are all getting the elements into the docs. Putting them all into a nice cohesive box that's moderately universal will be quite powerful. It could even be submitted to github to be rendered if we can keep it standalone in generating an rst or html block. And it would also be great if others who write a custom rst index etc can just do a directive like .. include_package_xml(./package.xml) kind of operation. And we can have that in the default template.

rkent commented 2 months ago

I have no problems including package.xml in the rosdoc2 output. I'd probably put in under "Standard Documents". I'll do a patch for that.

But I would still maintain that, for meta packages, it is useful to have a list of the included packages that can be easily clicked on to get to their documentation. Let me investigate a little more the possible ways that this could be detected, including looking at actual packages in use.

If there was a way to include this for most meta packages, even if it did not hit all of them, it could still be useful.

rkent commented 2 months ago

I did a review of packages that might be claiming to be meta packages (looking for 'meta' in package.description.lower(), and also matching "no package.build_depends but existing package.exec_depends". The additional common characteristic: all legitimate meta packages have no subdirectories, except 1: moveit also has a scripts directory. I'm going to look at packages that match that, but WITHOUT 'meta' in their description, to see how that looks.

rkent commented 2 months ago

Now reviewing a sampling of packages that do not have 'meta' in the description, but have no subdirectories (except perhaps 'doc'). The vast majority are legitimate meta packages (that is, a list of related packages that are part of a collection maintained by a single entity). A few are not meta packages of a larger collection, but include other packages. Examples:

vision_opencv: lists cv_bridge and image_geometry "Packages for interfacing ROS2 with OpenCV"

rosidl_runtime: action_msgs, service_msgs

moveit_resources: lists 5 moveitresources* packages, but also adds joint_state_publisher and robot_state_publisher

I'm going to modify the PR to match this plan.

rkent commented 2 months ago

Here we go with the subdirectory test. What's the downside to this? Maybe a handful of packages have their dependencies listed as a "meta package" when they are not true metapackages (if that even is well defined). For the vast majority of true metapackages, this adds useful documentation that should have links that can be followed by clicking.

tfoote commented 2 months ago

I agree that this information would be useful. But I think that we shouldn't actually limit this to "meta" packages. ANd we're actively working to avoid using that as another official term. The dependencies are valuable to list and link for all packages not just meta-packages. We should think about how do we effectively render all the metadata about all the packages at in one compact format that is consistent.

One example where we're doing this that's been very successful is on the ROS wiki like this: https://wiki.ros.org/tf2

image

And we also have this information and more on ROS Index aka: https://index.ros.org/p/tf2/#rolling-overview

We can't currently get the full richness here because of the need for extra index information. But I think that we should look at creating a more homogeneous way to integrate this extra information. (Things like the reverse dependencies, and knowing if the links are alive, and the CI jobs. A lot of that information comes from the rosdistro.) And the version selector for generated docs, like our core docs and on the wiki and on index.ros.org would be highly valuable.

image

Rendering the raw xml like in #115 is a good first step. But lets do it in a way that's valid for all packages instead of specifically making up a heuristic for a meta package.

rkent commented 2 months ago

I count 62 packages in recent rolling that meet this definition of a meta package. The vast majority of those have no documentation except for the package.xml file. The vast majority of these are also clearly intended to be a meta package, in the sense that they are a collection of related packages, managed by a single entity, that can be deployed as a group.

So y'all may be avoiding the concept or name "metapackage" but it appears to me that this is a concept widely in use in existing packages. And I would argue that it is fundamentally different from other types of packages, where the dependencies are really more about what you need to make the package work, and after it is working (which is mostly a build issue), the package user doesn't really care about what the dependencies are.

rosindex has access to the distro, while so far rosdoc2 does not. The info in the distro would be valuable, but it requires an internet access step that so far rosdoc2 has avoided. But it might be needed in the future. We should probably discuss that issue independently of this meta dependency issue.

If you really don't like the word "meta" perhaps we could show execution dependencies when they exist. But I fear that would get confusing because people have been using the tag that mixes issues.

I don't really have the standing or deep knowledge to push this perspective too strongly, but from what I have seen I think this should move forward. Reconsider but if you (or y'all) still don't like it, we can drop it.

clalancette commented 2 months ago

I don't really have the standing or deep knowledge to push this perspective too strongly, but from what I have seen I think this should move forward. Reconsider but if you (or y'all) still don't like it, we can drop it.

So I think my big problem with the implementation here is that it is based on heuristics. And given the thousands of packages that we have, those heuristics are inevitably going to be wrong for some subset of packages.

However, I do think that two good ideas have come out of this discussion: one is that we should show the dependencies of packages, and one is that metapackages are indeed a different "kind" of package.

For the first one, we have all of the metadata we need from the package.xml to display dependencies. So I think we can just make a PR to add that information; mostly it is a problem on how to display it.

For the second one, I think that we should allow the user to opt-in to documenting a package as a metapackage. That is, we add an option into rosdoc2.yaml (or similar) where a user can explicitly say that package so-and-so is a metapackage, and then we do special handling for it. That way we will never inadvertently call something a meta-package when it isn't one, and users always have a way to "opt-out" by not including the metapackage flag in the configuration. The downside here of course is that it isn't "automatic", and may be somewhat hard for users to discover, but I think that is preferable to heuristics.

Thoughts?

rkent commented 2 months ago

@clalancette Thanks for the input. I started to add some verbal comments, but it would be just as easy to prepare a revised PR that tries to respond to your concerns. I'll do that in the next couple of days.

rkent commented 2 months ago

The last branch push gives a version ready for re-review. This represents what I hope is an acceptable compromise, incorporating the previous comments.

Changes from before:

The heuristics established earlier are used if that value is not set.

With these changes, the downside of a false positive in show_exec_dep is:

The downside of a false negative is that the author would need to add an explicit entry in rosdoc2.yaml to show the exec_depend

The upside of the heuristics is that the vast majority of meta packages will now have some useful documentation shown, and not the virtually empty documentation as is currently shown. I doubt many existing packages would go back and explicitly add this new rosdoc2.yaml value if the heuristics were not present.

One question is, why only exec_depend and not all depend, and for all packages? For meta packages, they typically also have a dependency on ament_cmake which is not really useful to show. For all packages, I am not convinced there is value to showing them here in rosdoc2, as that is not useful information for most package users except in meta packages. Also rosindex already does a good job of showing that, with proper links since they have access to internet metadata, and you can see this in the package.xml listing that is now shown.

Anyway, this is ready for comments from @clalancette and @tfoote