search

ros2 / ci

ROS 2 CI Infrastructure
http://ci.ros2.org/
Apache License 2.0
48 stars 30 forks source link

Install LTTng and related packages on Linux #690

Closed christophebedard closed 1 year ago

christophebedard commented 1 year ago

Part of https://github.com/ros2/ros2/issues/1177

See https://github.com/ros2/ros2_tracing/pull/31

Signed-off-by: Christophe Bedard christophe.bedard@apex.ai

christophebedard commented 1 year ago

However, these Dockerfiles are also used for Humble (Ubuntu 22.04/RHEL-8) and Foxy (Ubuntu 20.04/RHEL-8). In both cases, we'll be adding packages that weren't there before. What is ros2_tracing going to do in those circumstances? I don't want to enable it in the stable distributions (particularly Foxy), for fear it might break something.

Oh, I missed that. That's indeed not great, because then tracetools would find LTTng and would include the tracepoints: https://github.com/ros2/ros2_tracing/blob/foxy/tracetools/CMakeLists.txt#L27.

I can do ${UBUNTU_DISTRO} = jammy and ${ROS_DISTRO} != humble for Ubuntu and ${EL_RELEASE} != 8 for RHEL for all these new packages.

clalancette commented 1 year ago

I can do ${UBUNTU_DISTRO} = jammy and ${ROS_DISTRO} != humble for Ubuntu and ${EL_RELEASE} != 8 for RHEL for all these new packages.

Yeah, that is probably a good idea. I'll also suggest putting this in a separate block in the Ubuntu Dockerfile, similar to https://github.com/ros2/ci/blob/7cbb9f6d57c41cac15fa8fd2b8020d2f7fe388de/linux_docker_resources/Dockerfile#L97

christophebedard commented 1 year ago

I'll also suggest putting this in a separate block in the Ubuntu Dockerfile

Done!

clalancette commented 1 year ago

This looks good to me, though I'll want to see Linux CI for all of Foxy, Humble, and Rolling on both Ubuntu and RHEL. I'd also like to get one more approval from @cottsay .

Oh, actually, I guess I need to run it, since we can only do tests on local branches (not forks). I'll wait to hear from Scott before kicking that off.

clalancette commented 1 year ago

OK, I've pushed an exact copy of this branch to the origin. Here are some CI runs with that in place:

Foxy:

Humble:

Rolling:

christophebedard commented 1 year ago

I thought we didn't need lttng-tools (which provides the CLI), but I forgot that I added it as an <exec_depend> of tracetools and that we need it in tracetools_trace: https://github.com/ros2/ros2_tracing/blob/e664e5bbd28b42da6430623fc1bcf644d6b6a449/tracetools_trace/tracetools_trace/tools/lttng.py#L90-L94. I'll add it as an exec dependency of tracetools_trace in https://github.com/ros2/ros2_tracing/pull/31 and add it here.

christophebedard commented 1 year ago

I also noticed that there was no rosdep rule for lttng-tools for RHEL, so I added it: https://github.com/ros/rosdistro/pull/36641

clalancette commented 1 year ago

All right. I'm only going to run the latest change against Rolling again, given that it is protected under the same checks as the previous code.

clalancette commented 1 year ago

Rolling:

christophebedard commented 1 year ago

The test failures on RHEL don't seem to be related, but I can't find the same failures in recent jobs.

clalancette commented 1 year ago

The test failures on RHEL don't seem to be related, but I can't find the same failures in recent jobs.

That one is a flaky test; you can see a failure of it in https://ci.ros2.org/view/nightly/job/nightly_linux-rhel_repeated/1448/#showFailuresLink . I really don't think it is involved here at all.

So I think we are good with this change.

Once we merge this in, that means that we'll be building tracepoints in by default for Rolling in our nightly CI jobs and our nightly packaging (tarball) jobs. What needs to happen to have it enabled in the debians? Do we just need to do a release of ros2_tracing for that to happen?

christophebedard commented 1 year ago

Once we merge this in, that means that we'll be building tracepoints in by default for Rolling in our nightly CI jobs and our nightly packaging (tarball) jobs. What needs to happen to have it enabled in the debians? Do we just need to do a release of ros2_tracing for that to happen?

Specifically after merging https://github.com/ros2/ros2_tracing/pull/31 (which I think we should merge right after this), yes! LTTng will be picked up as a dependency (otherwise the build will fail), and the tracepoints will be compiled in.

Well, that's what I think will happen, anyway. I can check the testing repo and validate once it's re-built.

christophebedard commented 1 year ago

Next step before merging would be to run CI using this branch and https://github.com/ros2/ros2_tracing/pull/31. See https://github.com/ros2/ros2_tracing/pull/31#issuecomment-1489374096

clalancette commented 1 year ago

Next step before merging would be to run CI using this branch and ros2/ros2_tracing#31. See ros2/ros2_tracing#31 (comment)

Awesome, the jobs there look good.

So what is the next step here? Do you want to merge the ros2_tracing PR, then I can merge this one?

christophebedard commented 1 year ago

So what is the next step here? Do you want to merge the ros2_tracing PR, then I can merge this one?

Since packages will now depend on LTTng, and since CI doesn't seem to run rosdep install (except for RHEL, it seems), we should merge this first. Then I'll merge https://github.com/ros2/ros2_tracing/pull/31 right after.

clalancette commented 1 year ago

Since packages will now depend on LTTng, and since CI doesn't seem to run rosdep install (except for RHEL, it seems), we should merge this first. Then I'll merge ros2/ros2_tracing#31 right after.

OK, got it.

So, the only thing is that I'm on PTO, not checking email, starting about now until Monday (April 3). Despite the fact that I think we've done sufficient testing here, I'm somewhat reluctant to merge something like this and disappear. So either we should have @cottsay merge it and keep an eye on it tomorrow, or I can do it first thing Monday morning when I'm back. @cottsay what do you think?

christophebedard commented 1 year ago

Waiting until Monday as a precaution is fine with me.