-
Notifications
You must be signed in to change notification settings - Fork 269
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
Fix docker nightly build #2643
base: gz-sim9
Are you sure you want to change the base?
Fix docker nightly build #2643
Conversation
Signed-off-by: Maksim Derbasov <[email protected]>
Signed-off-by: Maksim Derbasov <[email protected]>
Would it additionally make sense to introduce a CI job to make sure that this docker file can build on, for instance, a nightly basis? |
Not a big master of GitHub actions, could try I suppose |
Maybe in another PR? It not fits very well widely spread pipelines AFAIS. Actually I planned to make fix for annoying bug #2506 which affects us. (and we use older version in which I'd like to propagate fix too). |
Absolutely, would just be a nice addition to keep these dockerfiles fresh and not regressing.
Good with me. |
Ok. The last mystery, what happened in Jenkins. Looks like something goes wrong with couple of python tests. I can't try restart build there, don't have a permissions. |
the failing python tests are likely due to osrf/homebrew-simulation#2834 which is currently affecting all gz packages. So not the fault of this PR |
Thank you for your explanation. What is the next step for this PR? |
yeah, once this is merged, you can cherry pick the changes for backports or use mergify, e.g. add the comment |
Ok, will try) Thank you! |
docker/Dockerfile.base
Outdated
@@ -1,4 +1,4 @@ | |||
FROM ubuntu:focal | |||
FROM ubuntu:jammy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The officially supported platform for gz-sim9 is Ubuntu:noble. https://gazebosim.org/docs/latest/install/#supported-platforms
docker/Dockerfile.nightly
Outdated
@@ -10,7 +10,7 @@ COPY docker/scripts/enable_nightly.sh scripts/enable_nightly.sh | |||
RUN scripts/enable_nightly.sh | |||
|
|||
RUN apt-get update \ | |||
&& apt-get install -y \ | |||
&& apt-get install --no-install-recommends -y \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you verified this works? My recollection is that some of our packages, such as gz-*-python
and gz-*-cli
are suggested/recommended packages and my not install if we add --no-install-recommends
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried to make it smaller, it's working, but yes, some tools, like 'gz topic' maybe missing. I'll remove this line
@@ -17,16 +17,17 @@ sudo apt-get install --no-install-recommends -y \ | |||
cppcheck \ | |||
curl \ | |||
git \ | |||
g++-8 \ | |||
pkg-config \ | |||
g++11 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just install the default version
g++11 \ | |
g++ \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Just tried to make changes more in a spirit of previous style
Same fro Clang I suppose
docker/README.md
Outdated
@@ -73,21 +75,21 @@ distribution using debians. | |||
image of Gazebo Garden: | |||
|
|||
``` | |||
./build.bash gz-garden ./Dockerfile.gz | |||
./build.bash gz ./Dockerfile.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instruction above says the first argument must be the name of the Gazebo distribution. Should this change be reverted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess I used auto-replacement
I'll check everything and send update
Signed-off-by: Maksim Derbasov <[email protected]>
docker/run.bash
Outdated
@@ -40,7 +31,6 @@ docker run -it \ | |||
-v "/etc/localtime:/etc/localtime:ro" \ | |||
-v "/dev/input:/dev/input" \ | |||
--rm \ | |||
--gpus all \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after this changes this script started working fine on a Ubuntu 22.04 host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is needed to get GPU acceleration when using NVIDIA GPUs. Maybe leave it as is and add a statement below to remove it if not using a GPU?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll roll-back. Not sure how to dispatch automatically it for a GPU/noGPU cases. In my environment (VirtualBox) it leads to error:
docker: Error response from daemon: could not select device driver "" with capabilities: [[gpu]].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. I have a few more comments.
docker/run.bash
Outdated
@@ -40,7 +31,6 @@ docker run -it \ | |||
-v "/etc/localtime:/etc/localtime:ro" \ | |||
-v "/dev/input:/dev/input" \ | |||
--rm \ | |||
--gpus all \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is needed to get GPU acceleration when using NVIDIA GPUs. Maybe leave it as is and add a statement below to remove it if not using a GPU?
@@ -18,17 +18,8 @@ ARGS=("$@") | |||
# This is necessary so Gazebo can create a context for OpenGL rendering | |||
# (even headless). | |||
XAUTH=/tmp/.docker.xauth | |||
if [ ! -f $XAUTH ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, previous code not working, here is example on a system with empty /tmp
:
xauth: /tmp/.docker.xauth not writable, changes will be ignored
xauth: (argv):1: unable to read any entries from file "(stdin)"
chmod: changing permissions of '/tmp/.docker.xauth': Operation not permitted
Authorization required, but no authorization protocol specified
and previous code somehow creates /tmp/.docker.xauth
as a directory
docker/README.md
Outdated
@@ -75,7 +75,7 @@ distribution using debians. | |||
image of Gazebo Garden: | |||
|
|||
``` | |||
./build.bash gz ./Dockerfile.gz | |||
./build.bash gz-ionic ./Dockerfile.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but now we need to update the instruction right above to say "Gazebo Ionic"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll roll-back it here to garden and lines below too, due to build.bash
not working properly with Ionic AFAIU. It executes Dockerfile.gz
, which is based on nvidia/opengl:1.2-glvnd-devel-ubuntu20.04
image.
Signed-off-by: Maksim Derbasov <[email protected]>
🦟 Bug fix
Fixes #
Summary
Clean build by instruction in
docker/README.md
not working. Base image builds fine, but the next step is failing.Ubuntu Focal (20.04) is too old to build
gz-sim9
branch. It doesn't contain required GZ packages.GCC8 is a very old compiler, I believe installation and replacing a default compiler was done for Ubuntu 18 (with default GCC7). Switched to GCC11 (default for Ubuntu 22). IMHO I'd prefer not to bind to compiler version in this case, but I made it as close as possible
Minor improvements in Readme and image
AFAIU
docker/run.bash
script not working well on modern systems, but it's out of scope of this questionSlightly out of scope, but couple of months ago tried same for a
gz-sim8
branch and had problem with 2 missing gz libraries. If this patch will be accepted, I'll do similar changes on that branch tooChecklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.