Skip to content

Conversation

Nils-ChristianIseke
Copy link
Contributor

@Nils-ChristianIseke Nils-ChristianIseke commented May 9, 2025

Use noble for CI.

@Nils-ChristianIseke Nils-ChristianIseke marked this pull request as draft May 9, 2025 15:12
@Nils-ChristianIseke Nils-ChristianIseke mentioned this pull request May 9, 2025
4 tasks
@Nils-ChristianIseke Nils-ChristianIseke marked this pull request as ready for review May 10, 2025 12:58
@Nils-ChristianIseke
Copy link
Contributor Author

@christophebedard some open questions from my side. Happy if you could have a look?

@christophebedard christophebedard self-requested a review May 10, 2025 13:06
@christophebedard
Copy link
Member

I will take a look once I'm back from vacation on Tuesday-ish!

@christophebedard christophebedard self-assigned this May 22, 2025
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I have some comments and suggestions.

@Nils-ChristianIseke
Copy link
Contributor Author

Thank you @christophebedard !

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

thanks for the PR. a couple of comments.

@Nils-ChristianIseke Nils-ChristianIseke marked this pull request as draft June 24, 2025 05:57
@christophebedard
Copy link
Member

from my end, I think we just need to clarify how the constraints.txt file was updated, and then we're good.

@christophebedard
Copy link
Member

@Nils-ChristianIseke do you plan to continue working on this PR? The only thing missing is documenting how you generated the constraints.txt file: #5595 (comment).

If you don't have time, that's OK, I can take over.

Nils-ChristianIseke and others added 8 commits October 6, 2025 09:30
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
Signed-off-by: Nils-Christian Iseke <[email protected]>
@christophebedard christophebedard marked this pull request as ready for review October 6, 2025 16:47
@christophebedard
Copy link
Member

I'm now trying to compare the docs output before and after the bump to Ubuntu 24.04 and the constraints updates.

@christophebedard
Copy link
Member

I'm now trying to compare the docs output before and after the bump to Ubuntu 24.04 and the constraints updates.

I built the docs in multiversion mode (1) on rolling and (2) using this branch (i.e., with the updates constraints.txt). I quickly compared some Rolling docs pages, both visually and by diff-ing some HTML files. Some non-visible HTML things got moved around, but I think it's functionally the same.

One visual difference I noticed is this, from the end of this sub-section: :kbd: is formatted differently

  1. Before:
    image
  2. After:
    image

I assume it's due to the bump from Sphinx 7.2.6 to 8.2.3. So this LGTM.

To resolve this error:

  Step 17/19 : COPY requirements.txt constraints.txt .
  When using COPY with more than one source file, the destination must be a directory and end with a /

Signed-off-by: Christophe Bedard <[email protected]>
@christophebedard
Copy link
Member

I pushed this branch to this repo (as christophebedard/Nils-ChristianIseke/UpdateDependecies), changed the doc_ros2-documentation job on build.test.ros2.org to point to that branch instead of rolling (see this config diff), and triggered a build.

I had to fix the COPY command (b983b79): https://build.test.ros2.org/job/doc_ros2-documentation/2/console. After that, the job passed (minus a mail server connection failure): https://build.test.ros2.org/job/doc_ros2-documentation/3/console. And the resulting docs on the test site look good: https://repo.test.ros2.org/en/rolling/index.html

@claraberendsen I tested this with build.test.ros2.org/job/doc_ros2-documentation/ (24.04) instead of the test version of build.ros.org/job/doc_ros2doc/ (20.04; there doesn't seem to be a http://build.test.ros.org/). Merging this PR will just make the existing build.ros.org/job/doc_ros2doc/ job use the updated 24.04 Docker image to build the docs instead of a 22.04 image. Does that sound good to you? It's kind of separate from making the CI job itself use Ubuntu 24.04.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@Nils-ChristianIseke @christophebedard i think this is breaking the devcontainer environment that i usually use... can you check it on your env too?

@christophebedard
Copy link
Member

I don't use the devcontainer stuff. Can you describe what's broken?

@christophebedard
Copy link
Member

christophebedard commented Oct 7, 2025

Ah, I think it was missing the context, since previously requirements.txt and constraints.txt weren't COPYied during the image build. And the postCreateCommand needed to be updated.

The only other "issue" left is that the venv isn't sourced when you open a terminal, so you need to source it manually (. /opt/ros2doc/bin/activate) before building (make html). Do you know how to make it source this automatically?

@fujitatomoya
Copy link
Collaborator

i think that we need to rely on the shell in the userland of container, devcontainer does not have control this user environment.

before adding this change, how come do we need the python venv in the 1st place? having docker build environment is kinda dedicated and containerized environment for this specific task? then why do we need python venv??? what is the benefit to have python venv? i maybe missed some context here...

@christophebedard
Copy link
Member

I think it was meant to be a cleaner/more modern alternative to using --break-system-packages (to get rid of the corresponding error in the Python version shipped with 24.04). And having to install pip dependencies as the user (not simply in a venv) inside the Docker container makes dealing with user/UID a bit more complicated. However, it seems to work, so I guess we can go with that.

@christophebedard
Copy link
Member

christophebedard commented Oct 7, 2025

But, in my opinion, there are too many ways to build the docs:

  1. Locally using venv (simplest)
  2. GitHub Codespaces
  3. Devcontainer
  4. Docker image (what's used by CI)

I think the Codespace and devcontainer options are secondary, and I wouldn't necessarily avoid using venvs (and use a more fragile & less modern approach) in the 4th option just to avoid running 1 command in a devcontainer terminal.

@fujitatomoya
Copy link
Collaborator

@christophebedard thanks for fixing up.

i usually use devcontainer to build and test the documentation, Codespaces is actually devcontainer.
AFAIK, we do not really promote using docker directly to the user, right? that means, we actually maintain 2 ways to build the documentation, i say that is not bad ... 🤔

anyway, lgtm 👍 i think we can address minor adjustment if there are, once we upgrade the base system to ubuntu 24.04. again, thanks for addressing the comments.

@christophebedard
Copy link
Member

I'll run a test doc build/deployment job again (after my last changes) tomorrow just to make sure it's still good.

@claraberendsen
Copy link
Contributor

Merging this PR will just make the existing build.ros.org/job/doc_ros2doc/ job use the updated 24.04 Docker image to build the docs instead of a 22.04 image. Does that sound good to you?

Yes! Thanks for the work here, it's separate but part of the same migration and I was trying to land both so we can push everything to 24.04.

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