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

Add noble as an Ubuntu distro of choice. #738

Merged
merged 10 commits into from
Feb 1, 2024
Merged

Conversation

marcoag
Copy link
Contributor

@marcoag marcoag commented Jan 15, 2024

Starting this PR to gather feedback.

I believe with should hold the merge until the bootstrap repository step is done.

Note that:

  • No ROS1 has been set.
  • The dockerfile will have noble for ROS Rolling.

@@ -120,7 +120,7 @@ def main(argv=None):
'enable_coverage_default': 'false',
'dont_notify_every_unstable_build': 'false',
'build_timeout_mins': 0,
'ubuntu_distro': 'jammy',
'ubuntu_distro': 'noble',
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to switch the default to noble yet, so let's leave this as jammy for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed back in 89f939d.

@@ -94,7 +94,7 @@ RUN if test \( ${UBUNTU_DISTRO} = jammy -a ${ROS_DISTRO} != humble \); then apt-
# Iron uses python3-lttng, but then starting after Iron we switch to liblttng-ctl-dev.
RUN if test \( ${UBUNTU_DISTRO} = jammy -a ${ROS_DISTRO} = iron \); then apt-get update && apt-get install --no-install-recommends -y \
python3-lttng; fi
RUN if test \( ${UBUNTU_DISTRO} = jammy -a ${ROS_DISTRO} = rolling \); then apt-get update && apt-get install --no-install-recommends -y \
RUN if test \( ${UBUNTU_DISTRO} = noble -a ${ROS_DISTRO} = rolling \); then apt-get update && apt-get install --no-install-recommends -y \
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we want to support Rolling on both jammy and noble for now. So please update the conditional so either will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 89f939d.

@@ -94,7 +94,7 @@ RUN if test \( ${UBUNTU_DISTRO} = jammy -a ${ROS_DISTRO} != humble \); then apt-
# Iron uses python3-lttng, but then starting after Iron we switch to liblttng-ctl-dev.
RUN if test \( ${UBUNTU_DISTRO} = jammy -a ${ROS_DISTRO} = iron \); then apt-get update && apt-get install --no-install-recommends -y \
python3-lttng; fi
RUN if test \( ${UBUNTU_DISTRO} = jammy -a ${ROS_DISTRO} = rolling \); then apt-get update && apt-get install --no-install-recommends -y \
RUN if test \(\( ${UBUNTU_DISTRO} = jammy -o ${UBUNTU_DISTRO} = noble \) -a ${ROS_DISTRO} = rolling \); then apt-get update && apt-get install --no-install-recommends -y \
Copy link
Contributor

Choose a reason for hiding this comment

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

After looking at this and thinking about it again, I think I steered you wrong. In particular, we don't need the UBUNTU_DISTRO check at all, we can really just get by with the ROS_DISTRO check. I'm going to push a new commit here that does that, along with simplifies a few of the above lines.

@clalancette
Copy link
Contributor

OK, with the latest fixes, I think we can run some CI here. Even without a deploy, we can still test that the changes to the Dockerfile work, so here are some runs (all on Jammy):

Rolling: * Linux Build Status

Iron: * Linux Build Status

Humble: * Linux Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

Marco A. Gutierrez and others added 3 commits January 24, 2024 10:17
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
It is enough to just have the ROS_DISTRO checks here.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Marco A. Gutierrez added 3 commits January 24, 2024 15:47
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@marcoag
Copy link
Contributor Author

marcoag commented Jan 30, 2024

I didn't manage to run a git subtree pull --prefix ros2_batch_job/vendor/osrf_pycommon osrf_pycommon_upstream master to update the subtree. I used some git merge command to run an update but I kept it on a separate branch for now in case there's a proper way to do the subtree update. The branch is https://github.com/ros2/ci/tree/marcoag/add_noble_reset_subtree and it seems to pass for Rolling with the latest changes :

test_ci_linux:

  • Jammy Build Status
  • Noble Build Status

test_ci_linux-aarch64:

  • Jammy Build Status
  • Noble Build Status

test_packaging_linux:

  • Jammy Build Status
  • Noble Build Status

test_packaging_linux-aarch64:

  • Jammy Build Status
  • Noble Build Status

Edit: I managed to manually update the subtree and create a manual squash merge commit that is usable by the git subtree tool so I guess that works. I rerun the jobs above for this specific branch. I will send a separate PR with hints on how to do a manual subtree update.

Marco A. Gutierrez added 2 commits January 31, 2024 03:22
git-subtree-dir: ros2_batch_job/vendor/osrf_pycommon
git-subtree-split: f65f8594161dcff276813d59d3c1145ce767bb75

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@marcoag
Copy link
Contributor Author

marcoag commented Jan 31, 2024

I realized if we Squash and merge this we would loose the subtree commit which is undesirable. Maybe once we are good with the PR I can do a rebase to squash all commits into one and keep the subtree update so we can just run a merge without squashing afterwards.

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
@clalancette
Copy link
Contributor

Overall, this looks great to me.

I'm going to suggest running a couple more pieces of CI on this, because of the change to ros2_batch_job/util.py.

In particular, we are still using old versions of Python on both Rolling Windows (Python 3.8.3), and on Humble RHEL-8 (3.6). So it is possible that we will run into problems on those platforms. So please run CI with this branch on both of those. Once we confirm that those are good, I'm happy to approve and merge this PR. Thanks!

@clalancette
Copy link
Contributor

clalancette commented Jan 31, 2024

Given the updates to osrf_pycommon, here is a full suite of test jobs to see what happens with this in place:

Rolling:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Debug Build Status
  • RHEL-9 Build Status

Humble:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • RHEL-8 Build Status

@marcoag
Copy link
Contributor Author

marcoag commented Feb 1, 2024

test_ci_windows: Build Status

test_ci_linux-rhel: Build Status

@clalancette
Copy link
Contributor

Windows Debug is failing, but that is a completely different issue unrelated to this PR.

This otherwise looks good to me. I'm going to approve it, merge it in, and deploy it so we get a full day of running it to be sure it still works.

@clalancette clalancette merged commit f062dc4 into master Feb 1, 2024
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the marcoag/add_noble branch February 1, 2024 13:13
@clalancette
Copy link
Contributor

And this is now deployed!

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.

None yet

2 participants