ros-infrastructure / rosdoc2

Command-line tool for generating documentation for ROS 2 packages.
Apache License 2.0
29 stars 9 forks source link

Use ros docker images in github runner #72

Closed rkent closed 3 months ago

rkent commented 3 months ago

It's a fairly trivial point, but people running ros2 (and rosdoc2) are more likely to be on ubuntu-22.04 rather than ubuntu-20.04, so update the action runner to support this. Also add python3.10 since that is now the default version.

tfoote commented 3 months ago

This is a good point that most people will be on 22.04 and soon 24.04. There's a bit of a question of whether we are worried about breaking our oldest platform or testing our newest platform. But I think that our general policy has been to keep this as close to what's happening on the buildfarm so make sure that keeps working. @clalancette @nuclearsandwich have we moved the buildfarm forward?

Optimally we'd actually switch this to support a matrix test of all our supported platforms 20.04 (for the next year), 22.04 and soon 24.04. If we did that we could potentially switch this to use the built in versions of the tools instead of pip which would also increase reproducibility for users.

clalancette commented 3 months ago

@clalancette @nuclearsandwich have we moved the buildfarm forward?

Yes; all Rdoc jobs are now using 24.04.

However, the Idoc jobs are still using 22.04, so we should test both. Which brings me to...

Optimally we'd actually switch this to support a matrix test of all our supported platforms 20.04 (for the next year), 22.04 and soon 24.04.

We only need to test 22.04 and 24.04. We never had working documentation on any 20.04-based distribution, and all 20.04-based distributions are now end-of-life anyway.

rkent commented 3 months ago

I tried adding a github runner matrix with ubuntu-24.04 but the job is hanging. I don't see any evidence from github resources that 24.04 is an available os. Do you know how to use that?

tfoote commented 3 months ago

Yeah, github hasn't updated to make the prebuilds of 24.04 available yet. For most of our builds we use the Ubuntu ones or our own base images instead of the GitHub ones that gives us more flexibilty. Such as just running it on ros:rolling or ros:iron which is actually closer to what we expect our users to run on anyway.

rkent commented 3 months ago

So where are we at with this? Is there some way to use ubuntu24 on github actions that I don't know about? If not, then this PR as-is seems to be the best option.

Really though this is not critical to anything I am doing, so I'd be happy to drop it if that is what you want. To me, a much more interesting change would be to add a Windows runner, which is more likely to find regressions or new issues. But one way or another, it would good to get this either landed or rejected.

clalancette commented 3 months ago

To me, a much more interesting change would be to add a Windows runner, which is more likely to find regressions or new issues.

To be honest, I don't think a Windows runner adds much value, and adds more work. The point of this tool is, in the end, to generate HTML. Realistically, we only need a single platform where that works, and since it already works on Linux, we may as well stay there. I guess it might be nice for people to be able to do it natively on Windows, but with WSL nowadays, they can do it there.

rkent commented 3 months ago

"Realistically, we only need a single platform where that works" You are looking at this from the perspective of someone who is maintaining the rosdoc2 website. To me, another important audience for rosdoc2 is the individual package developer, who we would like to run rosdoc2 when they are developing the documentation for their package. rosdoc2 should work cleanly in all environments where we expect developers to live. I thought Windows was supposed to be a first-class environment in ros2. Has that changed?

clalancette commented 3 months ago

I thought Windows was supposed to be a first-class environment in ros2. Has that changed?

No, but I'm trying to be realistic about what work we are going to do here. We've never tested this tool on Windows, and I suspect that it is going to be quite a lot of work to make it work there.

rkent commented 3 months ago

Back when the big patch adding python support was being worked on, we tested rosdoc2 with Windows, and added where needed calls to a function to make it it work. I have not tested if for a couple of years, and without unit tests I doubt if it still works, but I suspect the changes are not major.

tfoote commented 3 months ago

I think we can integrate what I tested in #74 here to have specific coverage of images. I think that we can also remove some of the installation boilerplate too such as setup python which should already be in the image.

rkent commented 3 months ago

I'm still tweaking on this, the python setup and pip cache is not being done correctly yet.

rkent commented 3 months ago

There is one remaining issue with this, and that is that the pip caching does not work in a docker image, but that is a pretty trivial issue. If you look at previous runs, even with the pip cache "working", the github runner cache does not seem to prevent the re-download of all of the various pip dependencies. I've already put much more effort into this than I intended, that was supposed to be trivial.

The last patch incorporates using the ros:iron and ros:rolling images as suggested by @tfoote, and eliminates the unneeded python setup as well as the (not working) cache setup.

The only remaining change that might be interesting would be to add an additional image using ubuntu:24.04 like the build farm. But otherwise I don't think this is worth any more effort.

clalancette commented 3 months ago

Back when the big patch adding python support was being worked on, we tested rosdoc2 with Windows, and added where needed calls to a function to make it it work. I have not tested if for a couple of years, and without unit tests I doubt if it still works, but I suspect the changes are not major.

OK, I didn't know that. Thanks for the info.

ros-discourse commented 3 months ago

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rosdoc2-undergoing-significant-change/36905/2