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

Refactor template_ws #44

Merged
merged 17 commits into from
Aug 21, 2024
Merged

Refactor template_ws #44

merged 17 commits into from
Aug 21, 2024

Conversation

j3soon
Copy link
Owner

@j3soon j3soon commented Aug 20, 2024

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:

j3soon and others added 11 commits August 20, 2024 15:19
Originally, if a newly created workspace doesn't update the author info, it could lead to incorrect attribution.
…etter caching

For example, common tools such as curl are seldomly changed, and could be executed before other dockerfile commands to save build time.

As a side note, `useradd` is preferred over `adduser` for the following reasons:

    https://askubuntu.com/a/345986
…in ros base image

As we will source the global ros2 env in `.bashrc`
…ns and set domain ID to zero

Co-authored-by: YuZhong-Chen <[email protected]>
Co-authored-by: assume <[email protected]>
…bashrc`, Remove redundant `postCreateCommand.sh`
@YuZhong-Chen
Copy link
Collaborator

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

# Install Python pip
RUN --mount=type=cache,target=/var/cache/apt \
apt-get update && apt-get install -y \
&& rm -rf /var/lib/apt/lists/*

@j3soon
Copy link
Owner Author

j3soon commented Aug 21, 2024

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

@YuZhong-Chen
Copy link
Collaborator

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.

# Base Image : https://hub.docker.com/r/arm64v8/ros/tags?page=1&name=humble
FROM arm64v8/ros:humble AS arm64
# Base Image : https://hub.docker.com/r/osrf/ros/tags?page=1&name=humble
FROM osrf/ros:humble-desktop-full AS amd64
# Use docker automatic platform args to select the base image.
# It may be `arm64` or `amd64` depending on the platform.
# Reference:
# - https://docs.docker.com/reference/dockerfile/#automatic-platform-args-in-the-global-scope
FROM $TARGETARCH

RUN if [ "$TARGETARCH" = "amd64" ]; then \
apt-get update && apt-get install -y \
ros-$ROS_DISTRO-gazebo-ros-pkgs \
ros-$ROS_DISTRO-gazebo-ros2-control \
&& rm -rf /var/lib/apt/lists/*; \
fi

build:
context: .
dockerfile: Dockerfile
# Specify the target platform to build the image.
# Reference: https://docs.docker.com/compose/compose-file/build/#platforms
# platforms:
# - "linux/arm64"

@j3soon
Copy link
Owner Author

j3soon commented Aug 21, 2024

@YuZhong-Chen Nice suggestion! just added this.

@YuZhong-Chen
Copy link
Collaborator

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:

  • Remove docker-clean and add some commands in keep-cache. Ref1 Ref2.
  • Add sharing=private to prevent errors when building multiple Docker images in parallel. Ref

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
Copy link
Collaborator

YuZhong-Chen commented Aug 21, 2024

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

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.

… sharing to allow concurrent builds

We only cache `/var/cache/apt` and do not cache `/var/lib/apt`, since the main purpose of this cache mount is to speed up package downloads. Caching `/var/lib/apt` might offer a slight speed improvement, but its potential drawbacks could outweigh the benefits.

We use `sharing=private` instead of `sharing=locked`, since if we are building multiple images simultaneously, locking the cache may make the builds slower. In addition, the `private` sharing mode can still share caches across different image builds when the cache isn't accessed simultaneously.

For more information, see the references in comments or the discussion in the PR:

    #44 (comment)

Co-authored-by: YuZhong-Chen <[email protected]>
@j3soon
Copy link
Owner Author

j3soon commented Aug 21, 2024

@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 j3soon requested a review from YuZhong-Chen August 21, 2024 14:06
Remove the Gazebo cache since we use Docker volumes instead of cache in each workspace.
…ite errors.

Sometimes we might use packages that are no longer maintained. Although these packages can compile normally, `rosdep` might report errors because the `package.xml` is not updated. Using this flag can help avoid this issue. If a package is missing, it will still be displayed in the terminal, so you don’t need to worry about missing error messages.
Copy link
Collaborator

@YuZhong-Chen YuZhong-Chen left a comment

Choose a reason for hiding this comment

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

I have added some commits to fix the issues I observed. If these commits are fine, I believe the current changes can be merged into the master branch.

By the way, should we rename the master branch to main? Although this change won’t affect anything, I prefer the main naming convention, and the official default branch name has been updated. You can refer to this link for more information.

@j3soon
Copy link
Owner Author

j3soon commented Aug 21, 2024

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!

@j3soon j3soon merged commit aae39be into main Aug 21, 2024
1 check passed
@j3soon j3soon deleted the refactor/template-ws branch August 21, 2024 17:06
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