Invalid Dependencies Listed

[tfoote]

I was using rosindex to look at rclpy and noticed some non-dependencies in the dependency list

It's true for all distros: https://index.ros.org/p/rclpy/#rolling-deps

The list includes the following non-packages

rclpy.duration
rclpy.qos
rclpy.clock

@rkent Have you seen this before?

[tfoote]

Ahh, this appears to be doc_depends getting rendered

https://github.com/ros2/rclpy/blob/803716be691c4de81444fe1d5f956f7fcc92adf6/rclpy/package.xml#L74

There's actually crossover with ros-infrastructure/rosindex#621 that I randomly noticed this when preparing to file.

And some crossover to #213

[rkent]

I can't think of any good reason for doc_depends to be listed in rosindex, or anywhere else for that matter outside of package.xml. As far as I can tell, its only real use is to suggest mocking of python includes (where it is very important). Unless you have a different idea, I'll remove it.

[tfoote]

The intent of doc_depend was to provide dependencies that were required for building/rendering the documentation which are potentially different than the build or exec depends. For example if you needed mermaid for your doc build and it wasn't in the assumed core. Or you needed contents of a specific package defined. The system would make sure to install/provide that package before attempting to generate your packages documentation. Just like the build depends before build or the exec depends before running.

I think that we should find a more principled place to store the mocking suggestions. That seems more like a rosdoc.yaml type of content. Or potentially in an export section of the package.xml but not in the dependency list. The doc depends should be in the package/rosdep key space not python space.

To your specific question I think that the dependency should be rendered just like the other ones but clearly marked as a doc dependency. And likely the doc ones are pretty uncommon.

[rkent]

The intent of doc_depend was to provide dependencies that were required for building/rendering the documentation which are potentially different than the build or exec depends.

Is there any support anywhere for this anymore? I take it there was a time when infrastructure would build documentation beyond the standard rosdoc2 doxygen/exhale/breathe/sphinx,, but we don't do that anymore, right?

[tfoote]

We used to build the packages before we ran documentation runs against them to get the generated outputs. This could still be used to do something like add a dependency on a sphinx module for use in the documentation process.

I think that the python modules in there are failing to install but we are not catching the failure and erroring out as we do aggregate the dependencies in the doc task generator

https://github.com/ros-infrastructure/ros_buildfarm/blob/80bf1616f38a8e7baa1bbfe7446a105194c741e6/ros_buildfarm/scripts/doc/create_doc_task_generator.py#L668

The spec for the key is pretty clear here: https://www.ros.org/reps/rep-0140.html#doc-depend-multiple

[rkent]

I think the tl;dr of this issue is we need for rosindex to support the current usage of doc_depends (which is solely to signify python mocking in rosdoc2) rather than the REP-0140 definition. So rosindex should ignore doc_depends. Not entirely satisfying, but what else could we do that would not be disruptive?

[tfoote]

It seems that fixing rosdoc2 can resolve this and we can patch the handful of places this alternative is used pretty quickly.

We should use the exports section or the rosdoc.yaml to capture this list.

rclpy already has a declaration:

https://github.com/ros2/rclpy/blob/803716be691c4de81444fe1d5f956f7fcc92adf6/rclpy/package.xml#L80

Presumably autodoc_mock_imports should be able to be set in the sphinx.conf https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports

Just insert it into this list here: https://github.com/ros2/rclpy/blob/803716be691c4de81444fe1d5f956f7fcc92adf6/rclpy/rosdoc2.yaml#L69

diff --git a/rclpy/package.xml b/rclpy/package.xml
index 6e4db37..df5199b 100644
--- a/rclpy/package.xml
+++ b/rclpy/package.xml
@@ -68,13 +68,6 @@
 
   <doc_depend>python3-sphinx</doc_depend>
   <doc_depend>python3-sphinx-rtd-theme</doc_depend>
-  <!-- We add these modules as doc_depends in order to include them into the
-  autodoc_mock_imports list. This prevents TypeErrors from being thrown
-  when these modules import constants from rclpy C modules.
-  TODO: Remove these depends once a better solution is found.-->
-  <doc_depend>rclpy.duration</doc_depend>
-  <doc_depend>rclpy.qos</doc_depend>
-  <doc_depend>rclpy.clock</doc_depend>
 
   <export>
     <build_type>ament_cmake</build_type>
diff --git a/rclpy/rosdoc2.yaml b/rclpy/rosdoc2.yaml
index dd6d44d..da6dc40 100644
--- a/rclpy/rosdoc2.yaml
+++ b/rclpy/rosdoc2.yaml
@@ -66,5 +66,6 @@ builders:
         # doxygen_xml_directory: 'generated/doxygen/xml',
         # todo: Provide user sphinx_sourcedir below after warnings have been addre
ssed.
         sphinx_sourcedir: 'docs/source/',
-        output_dir: ''
+        output_dir: '',
+        autodoc_mock_imports: ['rclpy.duration', 'rclpy.qos', 'rclpy.clo
ck']
     }

This doesn't work with the sphinx builder conf, I think it needs to get passed through. But it feels like the right approach.

Alternatively we could use the exports section to gather this extra data like this, but maybe slightly different xml content.

diff --git a/rclpy/package.xml b/rclpy/package.xml
index 6e4db37..66471f4 100644
--- a/rclpy/package.xml
+++ b/rclpy/package.xml
@@ -68,16 +68,12 @@
 
   <doc_depend>python3-sphinx</doc_depend>
   <doc_depend>python3-sphinx-rtd-theme</doc_depend>
-  <!-- We add these modules as doc_depends in order to include them into the
-  autodoc_mock_imports list. This prevents TypeErrors from being thrown
-  when these modules import constants from rclpy C modules.
-  TODO: Remove these depends once a better solution is found.-->
-  <doc_depend>rclpy.duration</doc_depend>
-  <doc_depend>rclpy.qos</doc_depend>
-  <doc_depend>rclpy.clock</doc_depend>
 
   <export>
     <build_type>ament_cmake</build_type>
     <rosdoc2>rosdoc2.yaml</rosdoc2>
+    <autodoc_mock_imports>rclpy.clock</autodoc_mock_imports>
+    <autodoc_mock_imports>rclpy.duration</autodoc_mock_imports>
+    <autodoc_mock_imports>rclpy.qos</autodoc_mock_imports>
   </export>
 </package>

With this second approach the injection will have to happen from the new metadata area, but switching to use a dedicated ones I think makes sense.

[rkent]

rclpy seems to be a rather special case as they seem to have to mock their own items. I would hope in their special case, since they control the naming, there would be a better solution.

In order to not be disruptive introducing autodoc_mock_imports, we could scan existing released ROS2 packages to see what are the typical cases where our existing heuristics to guess python package import names are failing, and generate a default list of imports to mock to fix those cases. That would catch the overwhelming majority of existing cases - and would probably fix a few packages where rosdoc2 is failing to generate documentation. That's the kind of thing that ai would be good at generating a on-off script to run.

If we are going to generate a new mechanism like autodoc_mock_imports, it might be better to do that in rosdoc2.yaml rather than clogging up package.xml

[rkent]

The last few days I've vibe coded some scripts that take python.yaml from rosdep, try to install using apt or pip if appropriate, then see if you can import <name> where is the mocked name based on the heuristic in rosdoc2. If that fails, then the script tries to locate and report the correct . The ai came up with a couple of clever methods to get the correct name by examining the pip or apt installation. It actually seems to work pretty well.

Because the universe of python names in rosdep is finite, and this can be done at least initially with a script, I think the right thing to do is going to be to move the responsibility of coming up with the correct mocked from the user (using either <doc_depend/> or <autodoc_mock_imports/> and instead make that a lookup in rosdoc2. We could create the lookup initially using the script, then in the future either rerun the script occasionally, or manually maintain that list in the future.

I think that would be better than keeping the responsibility with the package author (when most authors won't bother).

rclpy would not work with that approach, but I really don't understand the issue with them.