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

Document full repos #81

Closed
wants to merge 7 commits into from

Conversation

rkent
Copy link
Contributor

@rkent rkent commented Mar 28, 2024

This is an optional but helpful PR to allow rosdoc2 to be used over a an entire repository with packages (or any other parent directory for that matter) and it will document all of the packages it finds. It also eliminates the need to specify '--package-path', defaulting to the current PWD.

Many significant ROS projects are coherent at the repository level, and are likely developed as a larger unit than just the individual package. If a developer want to view their project documentation, they would likely want to see all of the packages together. This allows that easily.

From a developer point of view, I want to use it to test cross-package linking, so I would need to run rosdoc2 over multiple packages to do that. This makes it doable in a single command.

I've also used this to document the entire set of packages for a rosdistro, starting from the checkout done by rosindex. That's the reason for the error checks and continuing, so a single package failure hours in does not bring the whole run down.

So while this is optional, it makes my life easier and could be useful to others.

@rkent rkent requested review from audrow and tfoote as code owners March 28, 2024 15:21
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Overall this seems like a reasonable generalization. I know that there was an intent to keep it simpler where you always had to pass it a single package. Especially

Part of it is that I'm now going to ask that you do things like track the success and return an error if one or more packages has failed. And to collect the name and error of the failed package(s) for a report at the end. And there's probably a desire for wanting to support continuing on error versus error at the first failure for faster feedback.

To that end might it make sense to add a new verb multi-build or maybe build-dir that will do the iteration logic and then invoke the build logic repeatedly.

We haven't done this as we have generally had scripts around the rosdoc process doing things like preparing packages and then potentially uploading the results such as https://github.com/ros-infrastructure/ros_buildfarm/blob/master/ros_buildfarm/scripts/doc/build_rosdoc2.py

But I think that you're right that it's much more useful to be able to glob quickly. So as a concrete request could you: (Sorry belay this request with the idea to make the iteration be done by a colcon extension.)

  • Change it to be a function call instead of an embedded for loop. That way the function can be tested independently from the for loop and discovery logic.
  • Add the tracking of failures and change the return code and write out a summary of failures before exiting.
  • And have an option for it to abort immediately on failure.

Part of the value in keeping it separate is that you could even see it wanting to build in parallel too. And this is getting into a lot of potential race conditions etc. And actually thinking about this, we have a tool explicitly designed for this sort of behavior which is colcon. It's designed to find and iterate packages, and invoke certain commands. A rosdoc2 extension for colcon might actually be best, where it is already designed to support all the multi processing, discovery and iterations, and package selection etc.

@cottsay Does making this a colcon extension sound like a reasonable thing to you? Instead of making this tool start to do package discovery and globbing?

@rkent
Copy link
Contributor Author

rkent commented Mar 29, 2024

Making this a colcon extension adds another level of abstraction, and I am not enthusiastic about this if the point is just to save a little globbing code. Right now I just type "rosdoc2 build" at a directory, and it just works. But then we always fear the unknown :)

The three requests starting with 'Change it to be a function call ...' are all very reasonable.

@cottsay
Copy link
Member

cottsay commented Mar 29, 2024

Hi @rkent, thanks for the contribution.

I talked with @tfoote about this today and I believe that using colcon's package discovery would be better than catkin's as well, but I believe that inverting the model is the better choice. The top-level colcon executable is intended for building and testing packages, so calling into rosdoc2 is out of scope.

That said, it has always been a goal for colcon as a project to provide each subsystem and extension point for other programs to use, and I think this would be a great place to do that. I've created a demo script which imports the relevant parts of colcon and discovers all packages in the working directory.

https://gist.github.com/cottsay/47a33dde4520e6fc7178af5f4e628d63

There are some rough edges here, and I think we could make some improvements in colcon-core to make this a little easier and support this use case better.

I look forward to your thoughts.

@tfoote
Copy link
Member

tfoote commented Mar 29, 2024

Yeah, I know it was asking to increase the scope. After talking with @cottsay I think that we've found an actual better middle ground and get many of the selection features and searching from colcon while keeping the complexity for the code here approximately the same. So to that end I think we can change it to a function and track the return codes and use the demo of the discovery that @cottsay prototyped. And users can then use the same package selection options that they're used to using for builds here. And when/if package selection logic changes it only needs to happen in one place.

@space192
Copy link

space192 commented Apr 9, 2024

is there any way it could be use to create a single documentation for all the "scaned" packages so that I can get a coherent browsing for the users ?

@rkent
Copy link
Contributor Author

rkent commented Apr 9, 2024

@space192 I am not sure what you mean by the "scaned" packages. I'll assume though that you mean all packages that are subdirectories of a parent (where the parent is typically a repository containing all of the packages).

This PR is really meant more as a convenience to the developer of a group of related packages in a single repository. You could simply call rosdoc2 build -p . from the repo directory, and it would run rosdoc2 over all of the packages in that repository. You could then view all of the package's documentation by serving the doc_build folder locally (with for example python -m http.server)

But it should not affect what is actually seen on the 'official' packager documentation pages http://docs.ros.org/en/rolling/p/.

There remains a significant issue as I see it of how to present documentation for a number of related packages. This is a very common problem faced by most large groups of related packages. It often makes sense for the bulk of the documentation to exist at the repository level and not in the individual packages. But rosdoc2 as currently designed is defined as a program to provides per-package documentation, not documentation for a group of packages.

The only support ros2 infrastructure currently provides for a group of packages, as as I understand it, is to show the readme of a repository, either if you click on the repository, or if you try to show there a package without its own readme. See for example https://index.ros.org/r/etsi_its_messages/

I've thought some personally of ways to try to extend rosdoc2 to be more useful with a group of packages, but have not proposed anything yet.

@rkent
Copy link
Contributor Author

rkent commented Nov 21, 2024

This was implemented in #139

@rkent rkent closed this Nov 21, 2024
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.

4 participants