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

Arm64 fixes #28

Open
wants to merge 9 commits into
base: humble
Choose a base branch
from
Open

Arm64 fixes #28

wants to merge 9 commits into from

Conversation

marc-hanheide
Copy link
Member

@marc-hanheide marc-hanheide commented Dec 12, 2024

Quite a few fixes were necessary to the limo image. Among other things, gazebo is not supported out of the box for arm64. This was previously ignored and not handled properly. Also, the lidar driver was using the head of the repository, which contained bugs. I had to tie down the version correctly. Finally, the build process did not take the latest source but there was a possible race condition in the way it was handled.

This pull request further includes several important changes to the Docker setup, workflow configuration, and package dependencies. The changes aim to improve the build process, streamline dependencies, and enhance the overall setup.

Dockerfile and Build Process Improvements:

  • Updated the libuvc clone command to use a specific commit and shallow clone for efficiency (.devcontainer/Dockerfile).
  • Commented out old Zenoh installation steps and added a new installation method to ensure proper setup (.devcontainer/Dockerfile).
  • Removed the install.sh script and integrated its steps directly into the Dockerfile for a more streamlined build process (.devcontainer/install.sh).
  • Updated the Docker build workflow to include separate jobs for amd64 and arm64 architectures, and added Docker login steps for both LCAS and Docker Hub (.github/workflows/docker-build.yml). [1] [2]

Dependency Management:

  • Removed the limo_ros2 repository from the .devcontainer/lcas.repos file, indicating a shift in how dependencies are managed (.devcontainer/lcas.repos).
  • Added several new exec_depend entries in the package.xml file to include necessary packages for limo_bringup (src/limo_bringup/package.xml).

…dencies

- Introduced TARGETARCH argument in Dockerfile for architecture detection.
- Modified install.sh to conditionally build packages based on TARGETARCH.
- Updated package.xml to include additional dependencies for limo_bringup.
…emove deprecated scripts, and enhance build process for multiple architectures
@marc-hanheide marc-hanheide self-assigned this Dec 12, 2024
@marc-hanheide marc-hanheide marked this pull request as ready for review December 12, 2024 17:32
@marc-hanheide
Copy link
Member Author

forgot to add: also now uses the latest stable zenoh

@marc-hanheide
Copy link
Member Author

This pull request includes several updates to the .devcontainer setup, improvements to the GitHub Actions workflow, and modifications to the package.xml dependencies. The most important changes include optimizing the Dockerfile, updating the GitHub Actions workflow for better build management, and adding new dependencies to the package.xml.

Dockerfile Optimization:

  • Changed the git clone command for libuvc to use a shallow clone (--depth 1) to speed up the build process.
  • Replaced the old Zenoh installation steps with a more streamlined process, including shallow cloning and building the Zenoh plugin.

GitHub Actions Workflow:

  • Commented out the old trigger job and added new steps for logging into Docker registries and building/pushing Docker images for amd64 and arm64 architectures. [1] [2]

Dependency Management:

  • Added several new exec_depend entries to the package.xml file for limo_bringup, ensuring all necessary packages are included.

Cleanup and Removal:

  • Removed the old install.sh script from the .devcontainer directory, as its functionality has been integrated into the Dockerfile.
  • Deleted the lcas.repos file, as it is no longer needed with the updated build process.

Copy link
Member

@cooperj cooperj 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 - with some well needed improvements 😃

However, there is some very minor improvements that could be made (i.e. the commented out lines) but they aren't the reason i'm requesting changes/blocking the merge - and i'd be happy to let the slide.

But, there possibly is a misunderstanding from my end - but either this image is what is used on the robot only or it is used both for simulation and on the robot.

If the image is directly used for both the robot and in simulation we should consider if we need to support Gazebo for ARM64.

But if this docker image is only being used on the LIMOs - is there a need to add Gazebo to the image or even build for amd64 anymore - now we are using the ros2_pkg_template?

# WORKDIR /opt/lcas/extensions/zenoh-plugin-ros2dds
# RUN bash -c "source '$HOME/.cargo/env'; cargo build --release -p zenoh-bridge-ros2dds"
# RUN install target/release/zenoh-bridge-ros2dds /usr/local/bin/
# WORKDIR /
Copy link
Member

Choose a reason for hiding this comment

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

do we want to remove this to clean up the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

# owner: LCAS
# ref: refs/heads/nvidia_develop # or main
# workflow: docker-latest.yml # Or Workflow ID
# token: ${{ secrets.ORGA_GH_TOKEN }} # GitHub Token With Relevant Permissions
Copy link
Member

Choose a reason for hiding this comment

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

same as previous - shall we remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, residual of the work in progress

. /opt/ros/humble/setup.sh && \
echo "target: $TARGETARCH" && \
if [ ${TARGETARCH} = "arm64" ]; then \
# no support for gazebo on arm64
Copy link
Member

Choose a reason for hiding this comment

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

will need to confirm if this works as expected both on an arm mac and on the actual limo?

Copy link
Member Author

Choose a reason for hiding this comment

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

so, this is a bigger challenge. Gazebo is not supported on jammy on arm64.

I have made it work though for CMP3103, using a PPA for an arm64 version of plain gazebo and compiling gazebo ROS from source, see https://github.com/LCAS/teaching/blob/2425-devel/.devcontainer/gazebo_ros_pkgs.repos and https://github.com/LCAS/teaching/blob/07e7c6ae4cef6642a26f2b04dc7808bf5d131e5c/.devcontainer/Dockerfile#L14

So, these are changes that would have to be made to any image we use for teaching with simulation on e.g. Apple Mx machines. But all these images to my knowledge include this repository and compile it from source. They don't use the image generated in this repository. The image here is mainly to be deployed on the limos themselves, and there is no need for gazebo. It would just make it more bloated. Hence my decision to not include the steps to compile gazebo into the image generated for arm64. Unless I'm missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

2 participants