Skip to content

Replace start-docker.sh with docker-compose.yml #422

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

Merged
merged 12 commits into from
Jun 11, 2022

Conversation

smartin015
Copy link
Contributor

Description

Per observations/discussion in #404, this PR replaces the original start-docker.sh script with a docker-compose.yml script. The same features are available in both, but using a compose file is less custom and easier to customize. It uses an anonymous volume - all changes in /root are persisted between runs.

Also fixes a couple of typos I found in the tutorials :)

@vatanaksoytezer please give this script a shot and let me know if there's anything you'd like me to change (e.g. if you'd prefer to also keep the script so folks have a fallback)

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@vatanaksoytezer vatanaksoytezer self-assigned this Jun 7, 2022
@gavanderhoorn
Copy link
Member

Does this still run xhost to update the permissions? Or is that no longer needed either?

@smartin015
Copy link
Contributor Author

Does this still run xhost to update the permissions? Or is that no longer needed either?

xhost should no longer be needed as ~/.Xauthority is mounted in (see https://stackoverflow.com/a/69505647). Please give it a try though - it works on my setup, but I haven't tried on a different PC.

Copy link
Contributor

@vatanaksoytezer vatanaksoytezer left a comment

Choose a reason for hiding this comment

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

Hello @smartin015 thank you so much for the PR, this is way nicer than scripting! I did test this and was able to get it to work with a few changes, that I mentioned below. I have a few nitpicky comments, appreciate if you can take them a look, otherwise, it's all good. Thank you so much!

@smartin015 smartin015 mentioned this pull request Jun 10, 2022
2 tasks
@vatanaksoytezer
Copy link
Contributor

vatanaksoytezer commented Jun 11, 2022

Thank you very much @smartin015 for the contribution, much appreciated! All looks good to me!

@vatanaksoytezer vatanaksoytezer merged commit 352061d into moveit:main Jun 11, 2022
@vatanaksoytezer vatanaksoytezer added backport-foxy This label signals Mergify to backport this PR to Foxy backport-galactic This label signals Mergify to backport this PR to Galactic backport-humble This label signals Mergify to backport this PR to Humble labels Jun 11, 2022
mergify bot pushed a commit that referenced this pull request Jun 11, 2022
(cherry picked from commit 352061d)

# Conflicts:
#	doc/how_to_guides/how_to_setup_docker_containers_in_ubuntu.rst
mergify bot pushed a commit that referenced this pull request Jun 11, 2022
(cherry picked from commit 352061d)

# Conflicts:
#	doc/how_to_guides/how_to_setup_docker_containers_in_ubuntu.rst
mergify bot pushed a commit that referenced this pull request Jun 11, 2022
(cherry picked from commit 352061d)

# Conflicts:
#	doc/how_to_guides/how_to_setup_docker_containers_in_ubuntu.rst
@smartin015 smartin015 deleted the docker_compose branch June 11, 2022 13:00
@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 13, 2022

With the latest docker-compose-plugin & nvidia-docker2 available for Focal, and the .yaml file contributed by this PR, I get could not connect to display :1 errors if I don't first run xhost + (or a more secure version of that command).

@smartin015: what sort of system are you running?


Edit: the system I tried this on doesn't appear to have $HOME/.Xauthority.

The $XAUTHORITY env var is set to /run/user/$(id -u)/gdm/Xauthority.

After changing the volume mount to read $XAUTHORITY:/root/.Xauthority things start to work without xhost +.

Having written that: I don't know whether this is a secure way of allowing access to the X server from the Docker container.

Comment on lines +41 to +42
environment:
DISPLAY: $DISPLAY
Copy link
Member

@gavanderhoorn gavanderhoorn Jun 13, 2022

Choose a reason for hiding this comment

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

@smartin015: I'm wondering if this works as intended.

According to the YAML spec (edit: merge keys only made it into 1.1 as a draft) and my testing, this does not override/merge environment into the environment map specified in x-base-gpu, which doesn't override/merge the environment map specified in x-base-cpu.

I've had to do something like this to get it to work:

x-base-cpu: &base-cpu
  ...
  environment: &base-cpu-env
    QT_X11_NO_MITSHM: 1
  ...

x-base-gpu: &base-gpu
  <<: *base-cpu
  ...
  environment: &base-gpu-env
    <<: *base-cpu-env
    NVIDIA_VISIBLE_DEVICES: all
    NVIDIA_DRIVER_CAPABILITIES: all

services:
  ...
  gpu:
    <<: *base-gpu
    ...
    environment:
      <<: *base-gpu-env
      DISPLAY: $DISPLAY

could you perhaps verify QT_X11_NO_MITSHM is being set in the gpu container @smartin015? env | sort didn't show it for me, unless I made the above changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad on testing somewhat I may have misused some settings while testing. I can confirm what @gavanderhoorn says here. The docker container launches fine but the display isn't working. This fix seems to set the correct environment variables as suggested, but unfortunately, the display wasn't working for me (ie: xeyes). I'm going to revert this change in a day if not resolved to keep non working stuff away from main.

Copy link
Contributor Author

@smartin015 smartin015 Jun 13, 2022

Choose a reason for hiding this comment

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

Apologies for the breakage - I opened #439 to remove the x-* templating since it may be a bit suspect. However, please do revert the change if you'd rather not try to fix forward. Better to have a working old path than a non-working new one :)

From a clean boot of my Ubuntu 22.04 computer, I ran the following with the new PR:

DOCKER_IMAGE=humble-source docker-compose run gpu /bin/bash
source install/setup.bash && ros2 run rviz2 rviz2

And it opens an rviz window as expected.

Running xhost command shows only SI:localuser:<my username>, so it seems that xhost isn't configured in any way beyond the defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-foxy This label signals Mergify to backport this PR to Foxy backport-galactic This label signals Mergify to backport this PR to Galactic backport-humble This label signals Mergify to backport this PR to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants