moveit / moveit2_tutorials

A sphinx-based centralized documentation repo for MoveIt 2
https://moveit.picknik.ai
BSD 3-Clause "New" or "Revised" License
153 stars 194 forks source link

Building API Documentation #505

Closed peterdavidfagan closed 1 year ago

peterdavidfagan commented 2 years ago

Description

At the moment the C++ API documentation is being built by cloning particular release branches from moveit2 and building the API documentation with Doxygen.

I wish to also include Python API documentation (see here) one issue with building the html files for this documentation is that it requires the package moveit_py to be available in the build environment as documentation isn't directly generated from the source files like the C++ API docs but instead it is generated from the built package. This is due to the use of the sphinx extension autodoc.

Additional Thoughts

CI can leverage already built Docker containers containing a completed build of moveit2 and hence it seems trivial to build the API docs within this environment. When building locally or running the deploy GitHub action things are less trivial as both clone the moveit2 repository to build the html files for the API docs. If we were to include building the moveit_py package as a dependencies this would be an unnecessary waste of resources.

My suggestion would be to alter the current deploy workflow environment (https://github.com/ros-planning/moveit2_tutorials/blob/main/.github/workflows/deploy.yml) to run a container that already has a built version of moveit2 such that both the API docs for C++ and Python can be easily generated. This has the added benefit of avoiding the need to clone the moveit2 repository as part of the workflow and can be added with relative ease. One uncertainty on my end is compatibility with the current deploy action.

The above does not suggest a solution for building the docs locally. I'd be open to hearing others suggestions for the local build. One approach I have taken is to simply add the autogenerated docs to the moveit2_tutorials repository, this way the command make html will also build the Python API docs and the C++ API docs are built as they were before.

@vatanaksoytezer I was asked to consult you on the above topic, I would appreciate any thoughts you can share on the above. I am happy to open a pr if you agree with the suggested changes.

@henningkayser @v4hn (tagging for visibility). https://github.com/ros-planning/moveit/issues/2606 (mentioning related issue)

vatanaksoytezer commented 2 years ago

I see that's a tricky problem if you need to use a built repository.

CI can leverage already built Docker containers containing a completed build of moveit2 and hence it seems trivial to build the API docs within this environment. When building locally or running the deploy GitHub action things are less trivial as both clone the moveit2 repository to build the html files for the API docs. If we were to include building the moveit_py package as a dependencies this would be an unnecessary waste of resources.

That is true, but we build all the docs using a single call to multiversion-with-api (see here), which is the same for every branch and it force pushes / overrides and rebuilt all docs for all branches with every push. That is not elegant and if we can make your idea implemented here it would be much better. If you want to utilize already built docker images (which would make a lot of sense in this case), we should change the way API docs is being built. While utilizing the docker images, we should instead deploy only to it's specific branch folder and not touch the other branches folders. I don't know if there is a good way of doing it but may need some git trickeries. I'm happy to review / support if / when you want to go ahead and implement it.

peterdavidfagan commented 2 years ago

If you want to utilize already built docker images (which would make a lot of sense in this case), we should change the way API docs is being built. While utilizing the docker images, we should instead deploy only to it's specific branch folder and not touch the other branches folders.

I'll look to consider changes taking the above comments into account.

I don't know if there is a good way of doing it but may need some git trickeries. I'm happy to review / support if / when you want to go ahead and implement it.

Thanks @vatanaksoytezer I'll look to implement some changes and would really appreciate your review/opinion on these changes.