Skip to content

Only add doxygen tags and intersphinx for packages we depend on#213

Open
rkent wants to merge 1 commit into
ros-infrastructure:mainfrom
rkent:cr-fixes
Open

Only add doxygen tags and intersphinx for packages we depend on#213
rkent wants to merge 1 commit into
ros-infrastructure:mainfrom
rkent:cr-fixes

Conversation

@rkent

@rkent rkent commented Nov 22, 2025

Copy link
Copy Markdown
Contributor

Generated-by: Portions of this commit may include code completion from github.copilot version 1.372.0 or later

Presently, rosdoc2 will include doxygen tag files, and intersphinx object.inv files, for any files it finds in the cross-reference directory. When running rosdoc2 in cases where there are a lot of pre-existing cross reference files, this can slow things down considerably as hundreds of cross references must be processed. In addition, it creates a dependency on order of execution, which can be problematic as doxygen results can change significantly depending on the available tagfiles, not only in cross references but also in what it chooses to document.

This PR only includes .tag and object.inv files for packages that exist in the exec_depends for a package. I believe this captures all of the important cross-references, without including unneeded ones.

This partially solves the dependency on execution issue. An upcoming PR will modify 'rosdoc2 scan' to process packages in dependency order. This should allow a single run of 'rosdoc2 scan' to generate needed cross-references for an entire ROS distro repository (which runs in a couple of hours on my computer). That could be implemented on the buildfarm in the future to get cross-references finally enabled.

Signed-off-by: R Kent James <kent@caspia.com>
Generated-by: Portions of this commit may include code completion from github.copilot version 1.372.0 or later
@rkent rkent requested review from audrow and tfoote as code owners November 22, 2025 19:27

@tfoote tfoote left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is a reasonable optimization to make. It was a specific design that we have used in the past that you can actually make forward references. This has been valuable when there's a group of closely related packages. Such as a base class that forward references to known implementations in other packages.

And on the buildfarm we can guarantee approximately complete availability of tags on the second pass. The first pass will rely on a graceful degredation.

But I don't think that this is something that we're significantly using right now and if it has large computation impact we don't need to retain that behavior.

collect_inventory_files(self.build_context.tool_options.cross_reference_directory)
collect_inventory_files(
self.build_context.tool_options.cross_reference_directory,
[d.name for d in package.exec_depends + package.doc_depends]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would build_depend not make more sense than exec_depend? As this is about build symbols?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.doc_depends is used typically to fix rosdoc2 issues that are not apparent from exec_depend issues. The most common use is when you need to mock a package upon rosdoc2 build to a different name. But I could imagine another use being if you wanted to include cross-references to some items in the python standard libraries, which would not typically be shown in exec_depend. Or perhaps you don't like this decision to limit cross-references to dependencies for some reason, and you wanted to force other cross-references to be included.

So I can think of reasons to include doc_depend but I cannot think of reasons to exclude it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my question was not to remove doc_depend but to replace exec_depends with build_depends

Suggested change
[d.name for d in package.exec_depends + package.doc_depends]
[d.name for d in package.build_depends + package.doc_depends]

For symbols that you're going to use the build depends seem like the better analogy. Though I guess that's a python vs c++ by default. In that case maybe we should be capturing the union?

@audrow audrow removed their request for review January 5, 2026 15:23
@tfoote

tfoote commented Jan 6, 2026

Copy link
Copy Markdown
Member

I wanted to check in if this was a mechanism you expected to be working

https://github.com/ros2/rclpy/blob/803716be691c4de81444fe1d5f956f7fcc92adf6/rclpy/package.xml#L70-L76

@rkent

rkent commented Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

Sorry to be slow here, I guess I never really caught up after the holidays.

Short answer, yes. I did a little investigation here of the history. pkgs_to_mock in the template is generated from exec_depends here but exec_depends is actually the package.xml exec_depends extended with doc_depends here. That extension was added here These are from the original support of python in rosdoc2, an era where I commented but was not the main contributor.

That AFAIK is actually the only place in rosdoc2 where doc_depends is used. But it is an important use, and really its only real use at the moment, in spite of the definition in REP-0140

rosdoc2 builds failing because a python package import name does not match the depends name is a significant issue throughout ros2. I've never been able to come up with an automatic way of handling that, so we currently depend on dep_depends entries per package to extend the mocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants