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

ROS 2 dockerized example for nmea GPS #3

Merged
merged 10 commits into from
Feb 16, 2024
Merged

ROS 2 dockerized example for nmea GPS #3

merged 10 commits into from
Feb 16, 2024

Conversation

delihus
Copy link
Contributor

@delihus delihus commented Feb 12, 2024

Created ros2 workflow with standard date and package version tag.
Moved compose.yaml to demo/ folder.

Signed-off-by: Jakub Delicat <[email protected]>
Signed-off-by: Jakub Delicat <[email protected]>
Signed-off-by: Jakub Delicat <[email protected]>
Signed-off-by: Jakub Delicat <[email protected]>
Signed-off-by: Jakub Delicat <[email protected]>
Dockerfile Outdated

## =========================== ROS builder ===============================
Copy link
Contributor

Choose a reason for hiding this comment

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

[major]
What's the purpose of having a separate entity building the package? The same packages are installed in both stages (i.e., ros-dev-tools), and rosdep is called twice with the same dependencies. In my opinion, this only complicates and unnecessarily prolongs the build process. I suggest installing and building everything sequentially in a single RUN command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some differences between ros-core and ros-base image but I can try to move this to one stage.

Dockerfile Outdated
Comment on lines 40 to 41
rm -rf /etc/ros/rosdep/sources.list.d/20-default.list && \
rosdep init && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary? If certain dependencies are already included in the husarnet/ros image, I believe they should be installed directly. Moreover, could you elaborate on the advantages and disadvantages of using this particular image? What rationale led us to choose it for this relatively straightforward use case over the basic ros docker image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the sources is not necessary but rosdep has to be initialized other way it gives:
image

Dockerfile Outdated
apt-get clean && \
rm -rf src && \
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the importance of minimizing the image size, but in my opinion, this step may not be necessary. What do you think?

Copy link
Contributor Author

@delihus delihus Feb 14, 2024

Choose a reason for hiding this comment

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

I think it is good practice to reduce the image size as much as possible. In this case removing src does not impact a lot but e. g. when mesh files exists it changes a lot.

README.md Outdated
Comment on lines 1 to 3
<h1 align="center">
Docker Images for a GPS module
</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<h1 align="center">
Docker Images for a GPS module
</h1>
# Docker Images for a GPS module

Alternatively, even the # nmea-gps-docker

README.md Outdated
The repository includes a GitHub Actions workflow that automatically deploys built Docker images to the [husarion/nmea-gps-docker](https://hub.docker.com/r/husarion/nmea-gps) Docker Hub repositories. This process is based on the [ros-drivers/nmea_navsat_driver](https://github.com/ros-drivers/nmea_navsat_driver/tree/ros2) repository.

[![.github/workflows/build-docker-image.yaml](https://github.com/husarion/nmea-gps-docker/actions/workflows/build-docker-image.yaml/badge.svg?branch=ros2)](https://github.com/husarion/nmea-gps-docker/actions/workflows/build-docker-image.yaml)
### GPS API
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we align this ROS API and the whole README with our standards, like here or here?

required: true
default: 'main'

default: development
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, is it still in development or stable? I think we should aim to stabilize it and move away from reliance on this image. Are there plans to release tag v1.0.0 or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the docker repositories we added the development and the release build. Here are the options:
image

The purpose is to push images with date and package version.
image

After the PR is closed we can trigger the release build to make sure that image is tested and does work.
image
It adds to existing image e. g. humble-2.0.1-20240209 -stable to the tag what gives humble-2.0.1-20240209-stable.

Referring to the version. The tag of images are versions of the built package described in package.xml and the date of docker image build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I apologize for the misunderstanding. I've used this feature before, but I was referring to the GitHub tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay! So after the PR I can release this!

Signed-off-by: Jakub Delicat <[email protected]>
@pkowalsk1 pkowalsk1 merged commit 3aa4712 into ros2 Feb 16, 2024
@pkowalsk1 pkowalsk1 deleted the ros2-devel branch February 16, 2024 10:10
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