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

Ubuntu 24.04 support for Rolling and Jazzy distros #3114

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

Arun-Prasad-V
Copy link
Contributor

@Arun-Prasad-V Arun-Prasad-V commented May 30, 2024

Tracked on LRS-1104

@SamerKhshiboun
Copy link
Collaborator

  • can you explain more about using the venv ?
  • about the rolling.. shouldn't we keep the temp fix ?

@Arun-Prasad-V
Copy link
Contributor Author

Arun-Prasad-V commented Jun 10, 2024

@Arun-Prasad-V Arun-Prasad-V changed the title Jazzy support Ubuntu 24.04 support for Rolling and Jazzy distros Jun 10, 2024
@Arun-Prasad-V Arun-Prasad-V marked this pull request as ready for review June 10, 2024 09:20
@@ -173,7 +173,7 @@ namespace realsense2_camera
class CimuData
{
public:
CimuData() : m_time_ns(-1) {};
CimuData() : m_data({0,0,0}), m_time_ns(-1) {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we don't initialize m_data, warnings are raised in Jazzy.

@@ -126,7 +128,8 @@ jobs:
# the next command might be needed for foxy distro, since this package is not installed
# by default in ubuntu 20.04. For other distro, the apt install command will be ignored.
sudo apt install -y ros-${{matrix.ros_distro}}-sensor-msgs-py
python3 src/realsense-ros/realsense2_camera/scripts/rs2_test.py non_existent_file
source ../.venv/bin/activate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this source command?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think to activate the virtual env (venv) created in line 116.
when creating venv, we should activate it to "run" inside it.
every command after it will be treated as inside the virtual env.
@Arun-Prasad-V am I right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, to use python in virtual env.

rosdep update --rosdistro ${{ matrix.ros_distro }} --include-eol-distros
echo "================= ROSDEP INSTALL ===================="
rosdep install -i --reinstall --from-path src --rosdistro ${{ matrix.ros_distro }} --skip-keys=librealsense2 -y
echo "================== COLCON BUILD ======================"
colcon build --cmake-args '-DBUILD_TOOLS=ON'
colcon build --cmake-args '-DBUILD_TOOLS=ON' --no-warn-unused-cli
Copy link
Collaborator

@SamerKhshiboun SamerKhshiboun Jun 11, 2024

Choose a reason for hiding this comment

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

nice catch :)
Just an explanation to the record:
BUILD_TOOLS is relevant only for realsense2_camera package, so warnings on unused cmd line variable were printed for realsense2_camera_msgs and realsense2_description.
now with --no-warn-unused-cli, there will be no warnings.
ref: https://cmake.org/cmake/help/v3.0/manual/cmake.1.html
--no-warn-unused-cli
Don’t warn about command line options.
Don’t find variables that are declared on the command line, but not used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!
I suggest adding a short sentence as a comment above this line.
Same for python venv usage

Copy link
Collaborator

@SamerKhshiboun SamerKhshiboun left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -126,7 +128,8 @@ jobs:
# the next command might be needed for foxy distro, since this package is not installed
# by default in ubuntu 20.04. For other distro, the apt install command will be ignored.
sudo apt install -y ros-${{matrix.ros_distro}}-sensor-msgs-py
python3 src/realsense-ros/realsense2_camera/scripts/rs2_test.py non_existent_file
source ../.venv/bin/activate
../.venv/bin/python3 src/realsense-ros/realsense2_camera/scripts/rs2_test.py non_existent_file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can use python3 instead of ../.venv/bin/python3, because the venv is already activated in previous line and no need to point the venv location here.

@Nir-Az
Copy link
Collaborator

Nir-Az commented Jun 11, 2024

@Arun-Prasad-V please add a Jira number (Tracked on xxx)

@@ -114,9 +112,13 @@ jobs:

- name: Install Packages For Tests
run: |
sudo apt install python3-venv
python3 -m venv .venv
Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant here 😀

@SamerKhshiboun SamerKhshiboun merged commit bd5705d into IntelRealSense:ros2-development Jun 11, 2024
8 checks passed
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

3 participants