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

Drop the rustup iron requirement #163

Merged
merged 2 commits into from
Apr 26, 2024
Merged

Conversation

Timple
Copy link
Contributor

@Timple Timple commented Apr 25, 2024

It seems to work just fine in Iron without it:

FROM ros:iron
SHELL [ "/bin/bash", "-c" ]
RUN apt-get update -qq && apt-get install -qqy python3-colcon-core ros-iron-demo-nodes-cpp
RUN mkdir -p /ws/src
WORKDIR /ws
RUN git clone https://github.com/ros2/rmw_zenoh /ws/src/rmw_zenoh
RUN rosdep update && rosdep install --from-paths src --ignore-src -y
RUN source /opt/ros/iron/setup.bash && colcon build
CMD [ "bash", "-c", "source /ws/install/setup.bash && \
  export RMW_IMPLEMENTATION=rmw_zenoh_cpp && \
  ros2 run rmw_zenoh_cpp rmw_zenohd & \
  ros2 run demo_nodes_cpp talker & \
  ros2 run demo_nodes_cpp listener" ]

@clalancette
Copy link
Collaborator

That is...extremely surprising. The Zenoh folks assure us that we need at least Rust 1.75.0 to build Zenoh currently, and Ubuntu 22.04 only has 1.62. But I haven't tried this myself yet.

@Timple
Copy link
Contributor Author

Timple commented Apr 25, 2024

Well, if there is a good reason to keep it, I don't mind.
But this is easier testing 🙂

@Timple
Copy link
Contributor Author

Timple commented Apr 25, 2024

Ah, found it:

$ docker run just-build-docker rustc --version
rustc 1.75.0 (82e1608df 2023-12-21) (built from a source tarball)

@Yadunund
Copy link
Member

Yadunund commented Apr 25, 2024

Yeah I'm also surprised the build passes without pinning rustc to 1.75. See #138

@Timple can you push a change to this PR to update this line in CI to install @stable instead of @1.75.0?

@Yadunund
Copy link
Member

Yadunund commented Apr 25, 2024

Ah, found it:

$ docker run just-build-docker rustc --version
rustc 1.75.0 (82e1608df 2023-12-21) (built from a source tarball)

For systems that do not have rustc installed, running rosdep on jammy will install the apt version of rustc which @clalancette pointed out which should cause build to fail.

@Timple
Copy link
Contributor Author

Timple commented Apr 25, 2024

Sure, I can do. But why is there actually a github action for installing rust? The ROS tooling can take care of this right? Just as with the commands in the README.md

@Yadunund
Copy link
Member

The ROS tooling can take care of this right? Just as with the commands in the README.md

The ROS tooling, ie rosdep will install the apt version of rustc. On Jammy this is 1.62 which will cause a build failure. On Noble, it will directly install 1.75 which works readily. Hope this clarifies.

@Timple
Copy link
Contributor Author

Timple commented Apr 25, 2024

For systems that do not have rustc installed, running rosdep on jammy will install the apt version of rustc which @clalancette pointed out which should cause build to fail.

Seems it got updated?

apt-cache policy rustc
rustc:
  Installed: 1.75.0+dfsg0ubuntu1~bpo0-0ubuntu0.22.04
  Candidate: 1.75.0+dfsg0ubuntu1~bpo0-0ubuntu0.22.04
  Version table:
 *** 1.75.0+dfsg0ubuntu1~bpo0-0ubuntu0.22.04 500
        500 http://archive.ubuntu.com/ubuntu jammy-updates/main amd64 Packages
        500 http://security.ubuntu.com/ubuntu jammy-security/main amd64 Packages
        100 /var/lib/dpkg/status
     1.58.1+dfsg1~ubuntu1-0ubuntu2 500
        500 http://archive.ubuntu.com/ubuntu jammy/main amd64 Packages

@Yadunund Yadunund closed this Apr 25, 2024
@Yadunund Yadunund reopened this Apr 25, 2024
@Yadunund
Copy link
Member

Sorry for accidentally closing the PR.
Do you mind deleting the step in the CI that installs rustc? We can let rosdep install rustc for Iron on Jammy

@Timple
Copy link
Contributor Author

Timple commented Apr 25, 2024

I'm have c++ code style violations?

Copy link
Member

@Yadunund Yadunund 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 changes. It's great to know that the apt version of rustc has been bumped to 1.75.0 on jammy.

We can ignore the failing style job. I'll fix that in a follow-up PR (will likely need to create an Iron branch since uncrustify config changed between iron and rolling/jazzy on noble)

@Yadunund Yadunund merged commit fa9397c into ros2:rolling Apr 26, 2024
5 of 6 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.

3 participants