-
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
Use ros docker images in github runner #72
Conversation
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. |
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...
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. |
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? |
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. |
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. |
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. |
"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? |
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. |
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. |
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. |
I'm still tweaking on this, the python setup and pip cache is not being done correctly yet. |
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. |
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 all your effort cleaning this up!
I'm not super worried about the underlying host now that we're working inside the containers. We can trust the isolation pretty well for this sort of thing.
OK, I didn't know that. Thanks for the info. |
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 |
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.