Skip to content
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

Fix conf.py so rosdoc2 will work #808

Draft
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Draft

Conversation

rkent
Copy link

@rkent rkent commented Nov 7, 2024

rosdoc2 stopped generating useful output for launch, probably because of rosdoc2 changes. This should make it work again.

Signed-off-by: R. Kent James <kent@caspia.com>
@@ -35,7 +35,7 @@
# with the same name of this package within the docs_build directory.
# Hence we add the parent folder to the system path so that the modules from
# this package can be imported.
sys.path.insert(0, os.path.abspath('.'))
sys.path.insert(0, os.path.abspath(os.path.join('..', '..', '..', '..', 'launch')))
Copy link
Contributor

Choose a reason for hiding this comment

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

can you clarify why do you need to use the 4th upper folder ? I tried to generate the current documentation and it's working for me.

Also the documentation online is looking fine https://docs.ros.org/en/rolling/p/launch/

@rkent
Copy link
Author

rkent commented Nov 8, 2024

There are several things going on in this PR.

First, the older format for intersphinx_mapping was deprecated long ago, and has been removed in Sphinx 8.0 See https://github.com/sphinx-doc/sphinx/pull/11247/files The build farm is using an older version of Sphinx, so you get some output from rosdoc2 with launch. Once the buildfarm starts running Sphinx 8, which it will soon, launch documentation with rosdoc2 will fail completely.

Second, though you are currently getting some output from rosdoc2, you are not actually getting any of the launch python API. See for example https://docs.ros.org/en/rolling/p/launch/launch.actions.append_environment_variable.html which is empty.

I have a run of rosdoc2 for launch using the current rosdoc2 main branch, with this PR applied. See https://prrosdoc2.rosdabbler.com/rolling/launch/launch.actions.append_environment_variable.html (Or for the home page, https://prrosdoc2.rosdabbler.com/rolling/launch) (Note this rosdabbler run is temporarily using launch with my PR applied, so in a few days that link may no longer be valid.) launch has beautiful python API documentation that is not currently being shown, but this PR will allow.

You will notice that home page looks significantly different from the current page. That is not related to this PR, but is the result of rosdoc2 change in ros-infrastructure/rosdoc2@1f933d5 which changes the behavior of packages (such as launch) that provide conf.py, but do not define sphinx_sourcedir. This (launch) PR does not make an attempt to redo launch documentation in response to that (rosdoc2) commit, but should be done in a followup.

Now to your actual question. I'm going the DRAFT this PR, because I really don't like having to add that sys.path value. The value is fragile in any case, as it depends on how rosdoc2 is run. rosdoc2 PR ros-infrastructure/rosdoc2#155 will fix that, so that the sys.path line can be removed from this (launch) PR. I'm not actually confident that this PR as presented will work on the build farm as I thought about it. So I'll wait until that PR lands before trying to proceed with this PR. It will

The reasons behind this? rosdoc2 "wraps" the package code, and actually runs in a directory under a docs_build folder. (This is to prevent rosdoc2 from spewing output into the repo when rosdoc2 is run locally) So the directory structure is:

(repo root)
  launch/
    docs_build/
      launch/
        launch/
          wrapped_sphinx_directory/
            conf.py
    launch/
        package.xml
        launch/
          __init__.py
          event.py
          ...

This is very confusing and really difficult for the poor package author to navigate, so let's wait until that rosdoc2 PR lands, then you should just remove the sys.path statement from conf.py

@rkent rkent marked this pull request as draft November 8, 2024 17:31
@rkent
Copy link
Author

rkent commented Nov 14, 2024

Update on this.

We fixed the sys.path issue upstream in rosdoc2, so that is no longer an issue. I run regular rebuilds of rosdoc2 on my buildfarm, and this package is working better there without any required changes.

See http://docs.ros.org/en/rolling/p/launch/launch.actions.append_environment_variable.html and note it is empty.

But the version with current rosdoc2 is https://prrosdoc2.rosdabbler.com/rolling/launch/launch.actions.append_environment_variable.html and that shows the python API.

My plans are to revamp this PR to better support current rosdoc2 and Sphinx 8, but that is not a high priority since the existing code is working adequately.

For the documentation to work on pre-rolling versions of launch, you would need to backport 65a2010

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