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

Feature/dockerise #99

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Feature/dockerise #99

wants to merge 23 commits into from

Conversation

niederha
Copy link

@niederha niederha commented Feb 2, 2023

Hello!

I made a docker infrastructure that allows you to run iiwa_ros. This is, in my opinion a cool feature that means you don't have to be stuck with a certain ubuntu version to run iiwa_ros, instead you can just run it and work in a container. It also helps with portability and reproducibility of robotics experiments.

I have tested it although not extensively. It works well, yet it might still have some bugs, I'll gladly keep an eye on this to fix them as they come during the following year.

I documented as best I could, imo, this is mature enough to be merged into master.

I'm not sure who I should assign this to. This is likely not perfect since I don't have a ton of experience with docker but I think it is good enough.

@niederha niederha self-assigned this Feb 2, 2023
@niederha niederha requested a review from domire8 February 7, 2023 11:31
Copy link

@domire8 domire8 left a comment

Choose a reason for hiding this comment

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

This seems to make sense 👍 a few minor comments but I think everything that you tested with all the different scripts and on a few different machines is good to go!

docker/Dockerfile Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
@niederha niederha removed their assignment Feb 15, 2023
@niederha
Copy link
Author

@matthias-mayr @costashatz @nash169 This PR is reviewed and ready for merged. I have the rights to do it but I'm not one of the official maintainers of the repo. Could I have the greenlight from one of you?

@costashatz
Copy link
Collaborator

@niederha thanks! I will have a check later during this week :) Overall it seems great..

…om AICA-docker + specified it in the install script
# Clone KUKA FRI (need to be root to clone private repo)
WORKDIR /tmp
USER root
RUN --mount=type=ssh git clone [email protected]:epfl-lasa/kuka_fri.git
Copy link
Contributor

@matthias-mayr matthias-mayr Feb 21, 2023

Choose a reason for hiding this comment

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

This would fail for everyone not having access (like me)
But I guess people can adapt the docker file to make it point to their own repo. Or pass an argument maybe?

Copy link
Author

Choose a reason for hiding this comment

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

Oh it was my understanding that this was a requirement to use this library. If that's not the case I can find a work around to skip it in case we can't access

Copy link
Contributor

Choose a reason for hiding this comment

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

It is required, you got that totally right. But I am not at EPFL and haven't gotten access to the repo.
I got the code instead and put it in our own repo. So for me the docker would not build right now.

I guess one option is to allow people to optionally pass their own repo address

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good point by @matthias-mayr . I want to make kuka_fri public though in the long term. I need to contact KUKA. So for now, we can make this an argument..

ARG HOST_GID=1000
ARG GIT_NAME=""
ARG GIT_EMAIL=""
ARG USE_SIMD=OFF
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: My understanding is that it creates the wrong impression that this is still an option.
The dependencies such as RBDyn are currently always without SIMD, so setting this true would result in crashes

@matthias-mayr
Copy link
Contributor

The files look fine to me. I didn't try running it yet.

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.

4 participants