-
Notifications
You must be signed in to change notification settings - Fork 11
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
Support user doc dir #148
Support user doc dir #148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good. message_filters looks a lot more complete.
I think unrelated I saw that at https://prrosdoc2.rosdabbler.com/rclcpp/generated/classrclcpp_1_1Node.html#classrclcpp_1_1Node
There's a lot of extra class references, whereas at
https://docs.ros.org/en/rolling/p/rclcpp/generated/classrclcpp_1_1Node.html#classrclcpp_1_1Node it looks to be only local references. Is this because you're using the scan approach?
And when I tried to click on it the URLs were wrong for the class references. Aka BatteryStateDisplay links to a bad url http://docs.ros.org/en/latest/p/battery_state_rviz_overlay/generated/classBatteryStateDisplay.html#classBatteryStateDisplay It looks like that might be getting a wrong site prefix.
https://prrosdoc2.rosdabbler.com/battery_state_rviz_overlay/generated/classBatteryStateDisplay.html#classBatteryStateDisplay seemed to resolve with that. And it's also referencing latest instead of rolling
raise RuntimeError( | ||
f"Error Sphinx SOURCEDIR '{value}' does not exist relative " | ||
f"to '{configuration_file_path}', or is not a directory.") | ||
self.sphinx_sourcedir = sphinx_sourcedir | ||
logger.info(f'The user specified sphinx_sourcedir as {value}') | ||
self.sphinx_sourcedir = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally change this to be a relative directory instead of an absolute one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind I see it's joined below
Concerning "I think unrelated I saw that at ... ": Two things are going on here, both more related to my Jenkins instance than to rosdoc2 changes. First, yes because I am using a scan of the entire distro (unlike buildfarm which I think is using a buildfarm equivalent of the scan, but only for a single repo), I get full cross-references, and not just the repo cross references. Also even though I make no attempt to run the individual rosdoc2 instances in a dependency order, because the cross reference files are preserved between runs, the previous run will almost always show a correct cross-reference link even if the runs are not dependency order. Node just happens to have a lot of cross-reference links. Actually that list is useful, it shows you all of the packages currently defining C++ nodes. Second, I am running rosdoc2 simply as I'll fix my buildfarm to point to the correct URL and rerun so we can see the dependency links. |
The links should now be correct (back to https://prrosdoc2.rosdabbler.com/) on https://prrosdoc2.rosdabbler.com/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the demo site. This looks good.
Under current rosdoc2, several concepts are conflated:
These three concepts were enabled by whether you included a conf.py (either in standard location, or specified by sphinx_sourcedir). There was no way to specify, for example, that my documentation is in docs/ rather than doc/, but I do not include conf.py, and I do not want to override the standard rosdoc2 output.
With this change, you specify the three choices as follows (using the three options above):
This is a behavior change that will affect a few existing packages: those that specified conf.py in a standard doc location (either doc/ or doc/source), but did not specify sphinx_sourcedir. Previously, it was assumed that this was equivalent to specifying sphinx_sourcedir, but that is no longer the case. This PR has two effects on those packages. First, the documentation that previously was the entire rosdoc2 output is now a link under "Documentation", and the root of the rosdoc2 output shows the standard rosdoc2 items, like "Links", "Standard Documents", etc. Second, previously the root of the project was the implied sphinx_sourcedir (either doc/ or doc/source), now the root of the project is the package.xml directory. In many cases this has no effect, but if the supplied documentation has links to generated content, that now needs to be at "../generated" not "generated" so those links fail (but the generated content still shows by default in the standard rosdoc2 links).
I have prepared a full run of rosdoc2 on rolling with this PR applied, that is available at https://prrosdoc2.rosdabbler.com/ I also scanned rolling packages, looking for those that define conf.py. I include a full list later, but here are a few examples;
Packages that previously showed only documentation, now show full rosdoc2 output plus documentation: message_filters, rviz2, image_proc
Packages that have broken API links in their custom documentation: realtime_tools, control_toolbox
A few packages had odd behavior which I investigated and decided were unrelated to this PR:
launch: works in https://docs.ros.org/en/ros2_packages/rolling/api/launch/ but broken at https://prrosdoc2.rosdabbler.com/ This is because the build farm is using Sphinx 7.4.7, with launch showing: "WARNING: The pre-Sphinx 1.0 'intersphinx_mapping' format is deprecated and will be removed in Sphinx 8" rosdabbler uses Sphinx 8, and this is now broken.
cv_bridge: is broken in both (but is fixed in PRs to come, see https://rosdoc2.rosdabbler.com/cv_bridge/)
ur_client_library: shows from June 2024 at https://docs.ros.org/en/ros2_packages/rolling/api/ur_client_library/ but is broken in rosdabbler. I believe this is from a change in the repo, and the current repo would also break in current rosdoc2.
I did not check all of the packages that might be affected, but you are free to do your own checks.
There is a related change in this PR as well. Because essentially package content is copied to the build directory in rosdoc2, there is no need to rename directories as was done previously. I am reverting to using the existing directory names (like 'doc') rather than an artificial name (like 'user_docs'). By preserving the existing doc structure, relative links in user documentation are more likely to work. I am not aware of any current packages that this affects.
In the future, many packages can specify user_doc_dir and have documentation shown that is currently missing. At https://rosdoc2.rosdabbler.com I have included (from future PRs) a curated "correction" file for package rosdoc2.yaml that exposes some of this. See for example rmw_dds_common, tracetools_analysis or rosidl_runtime_c which need user_doc_dir to show their existing documentation.
There are other existing packages that specify sphinx_sourcedir, but would be better off just specifying user_doc_dir. I have not seached for those here.
Affected packages: