j3soon / ros2-essentials

A repo containing essential ROS2 (Humble) features for controlling Autonomous Mobile Robots (AMRs).
Apache License 2.0
7 stars 3 forks source link

Refactor `template_ws` #44

Closed j3soon closed 3 weeks ago

j3soon commented 3 weeks ago

This refactor addresses several issues we encountered and discussed since the initial version of template_ws.

This also simplifies some commands, improves performance, and automate commands to make the development process more streamlined.

I would appreciate any feedback from @YuZhong-Chen and @Assume-Zhan when you have time to ensure I didn’t miss anything. If it looks good, you could submit a review by simply approve this PR, and optionally leave some comments. Thank you!


After merging this PR, we still need to:

YuZhong-Chen commented 3 weeks ago

Hi, I think you missed installing Python pip in this section.

https://github.com/j3soon/ros2-essentials/blob/afdb58c658c208b7daaa6e512c8a209b7fe99961/template_ws/docker/Dockerfile#L40-L43

j3soon commented 3 weeks ago

Hi @YuZhong-Chen , I just pushed a fix, thanks!

YuZhong-Chen commented 3 weeks ago

I think we can support multi-platform builds in this PR, otherwise, it will be difficult to refactor the existing workspace, such as husky_ws. However, we might not support arm64 in every workspace, we may need to add some comments in the compose file to indicate whether this Dockerfile supports arm64.

To enable arm64 architecture support in template_ws, we can add the following lines.

https://github.com/j3soon/ros2-essentials/blob/443c2bb1d1476ed823b65a3ca732324522074584/husky_ws/docker/Dockerfile#L1-L10

https://github.com/j3soon/ros2-essentials/blob/443c2bb1d1476ed823b65a3ca732324522074584/husky_ws/docker/Dockerfile#L51-L56

https://github.com/j3soon/ros2-essentials/blob/443c2bb1d1476ed823b65a3ca732324522074584/husky_ws/docker/compose.yaml#L3-L9

j3soon commented 3 weeks ago

@YuZhong-Chen Nice suggestion! just added this.

YuZhong-Chen commented 3 weeks ago

I haven't used Docker build cache before, so I'm currently learning about it step-by-step from the official documentation. I believe our current Dockerfile is still missing some commands.

Here’s an example from BuildKit:

# syntax=docker/dockerfile:1
FROM ubuntu
RUN rm -f /etc/apt/apt.conf.d/docker-clean; echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache
RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
  --mount=type=cache,target=/var/lib/apt,sharing=locked \
  apt update && apt-get --no-install-recommends install -y gcc

Based on the example above, we should:

I'm currently not sure if we need to mount /var/lib/apt, because we have encountered issues before where this cache prevented us from updating apt lib lists. I prefer sticking with the existing approach: directly deleting it and regenerating it each time it's used.

If you want to learn more, I think this website explains it very clearly.

YuZhong-Chen commented 3 weeks ago

I'm sorry, I didn't clarify the previous example well. You should mount the cache here in commit f8e86d96a6b91c303f736a7039839b21bae3df1e

By the way, we could add some examples in the TODO section to remind users to use the cache.

# TODO: Add more commands here
# For example, to install additional packages, uncomment the following lines and add the package names
# RUN --mount=type=cache,target=/var/cache/apt \
#     apt-get update && apt-get install -y \
#     $OTHER_PACKAGES \
#     && rm -rf /var/lib/apt/lists/*

However, mounting the cache might involve changes, we can wait until the previous comment is resolved.

j3soon commented 3 weeks ago

@YuZhong-Chen Thanks for the info! I'm also new to this feature and learned a lot by going through the detailed references you provided. I agree with your suggestions on this issue, and just pushed a fix accordingly.

j3soon commented 3 weeks ago

Thanks for reviewing this PR. I'll go ahead and merge it.

I agree with you and just renamed the default branch from master to main. Thanks for the reminder!