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

Docker #151

Merged
merged 11 commits into from
Mar 4, 2024
Merged

Docker #151

merged 11 commits into from
Mar 4, 2024

Conversation

mhubii
Copy link
Member

@mhubii mhubii commented Jan 21, 2024

Refers to #149

Nicolai-98 and others added 2 commits January 19, 2024 15:12
#149 Added Dockerfile and scripts. Updated README.

thank you @Nicolai-98 for doing this so quickly!
Copy link
Member Author

@mhubii mhubii 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 overall, re-iterate on file placement

README.md Outdated Show resolved Hide resolved
lbr_humble_docker/Dockerfile Outdated Show resolved Hide resolved
@mhubii mhubii self-assigned this Feb 27, 2024
@mhubii mhubii added the enhancement New feature or request label Feb 27, 2024
@mhubii
Copy link
Member Author

mhubii commented Feb 27, 2024

hi @Nicolai-98 , thank you again for contributing this. I now have time to review properly and would like to merge into humble.

do you think it makes sense to put the shell scripts / the dockerfile into the root folder, since they are no ros package. Also, in terms of build instructions, does it make sense to separate docker instructions? Thank you for the input and the help!

@Nicolai-98
Copy link
Contributor

Hi! I'm always happy to be able to contribute. Ideally you want the docker related files in the lbr-stack directory that is created using the quick start instructions from the ReadMe: mkdir -p lbr-stack/src && cd lbr-stack. This directory does not exist yet when cloning the repository. This is why I suggested copying them after following the quick start by using:

cp -r src/lbr_fri_ros2_stack/lbr_humble_docker/* .
chmod +x container_build.sh 
sudo ./container_build.sh

To answer the first question: The files can also be put into the root directory as long as they end up in the correct directory after building the project. This allows an easy workflow where you only have to use the container_build script once and then you can start a container whenever you need it using the container_start script.
I am not 100% sure what you mean by separating docker instructions from build instructions. Could you clarify?

@mhubii
Copy link
Member Author

mhubii commented Feb 29, 2024

okay got it! By that I mean just running with / without Docker.

Hm okay, would it be useful to turn these shell scripts into entry points a la

ros2 run lbr_docker build_docker

that would justify a package, right? Or is that rather pointless. Can setup.py turn shell scripts into entry points? But then that's also strange because one would have to install the package prior to building a container.

@mhubii
Copy link
Member Author

mhubii commented Mar 1, 2024

okay I tested it and it works, got to double check on real robot. But I would say lets merge this PR ASAP and go from here. Thank you again @Nicolai-98 !

@mhubii mhubii mentioned this pull request Mar 1, 2024
32 tasks
@Nicolai-98
Copy link
Contributor

Its my pleasure. I think the current setup with the changes you made is perfectly fine. I don't think turning the shell scripts into entry points adds any extra value.

Copy link
Contributor

@Nicolai-98 Nicolai-98 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 and tidy!

RUN rosdep install -i --from-paths src --rosdistro ${ROS_DISTRO} -y

# "--symlink-install" allows the code in the locally mounted volume ./src to be adjusted without rebuilding
RUN source /opt/ros/${ROS_DISTRO}/setup.bash && \
Copy link
Member Author

@mhubii mhubii Mar 2, 2024

Choose a reason for hiding this comment

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

okay so I cleaned some dependencies, as these are mostly in the base images already.

One more question before merge @Nicolai-98: I don't use Docker a lot, so please let me know if you need scipy / numpy / vim / python3 is python etc installed, so it is more convenient for your use.

There will likely be a couple changes moving forward but this is a great first step.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I went ahead and merged this anyways already. Thank you again! You are now listed among contributors.

Please feel free to create new PRs against humble if you see fit.

@mhubii mhubii merged commit 6b761dc into humble Mar 4, 2024
1 check passed
@mhubii mhubii mentioned this pull request Mar 4, 2024
@mhubii mhubii deleted the dev-humble-docker branch March 4, 2024 11:29
This was referenced May 13, 2024
@mhubii mhubii mentioned this pull request Jun 11, 2024
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants