Copy rosdoc2 sources into docker image instead of bind-mounting them read-only. #1030
Conversation
We don't currently distribute this as a deb and I am not in the mood to make it a user package. I might follow up by adding arguments to skip installation of dependencies to push those into apt.
Our ability to do out-of-tree builds by re-enabling a deprecated pip feature has been removed. Normally avoiding the copy here might save us a container cache miss but since we build the rosdoc2 images with `--force-rm` the intermediate containers are removed and thus every build is re-run anyway.
Whether or not keeping the ubuntu user when it exists as uid 1000 is the way forward we shouldn't hard-code paths we can get from the environment.
| @@ -64,7 +63,7 @@ def main(argv=sys.argv[1:]): | |||
| env['PATH'] = '' | |||
| else: | |||
| env['PATH'] += ':' | |||
| env['PATH'] += '/home/buildfarm/.local/bin' | |||
| env['PATH'] += os.path.join(env['HOME'], '.local/bin') | |||
There was a problem hiding this comment.
This was creating problems for my nuclearsandwich/uid-override patches which are required if your host user id is 1000 as the ubuntu:noble image now has an ubuntu user there.
| @@ -178,7 +180,6 @@ else: | |||
| ' --rm ' + | |||
| ' --cidfile=$WORKSPACE/docker_doc/docker.cid' + | |||
| ' -v $WORKSPACE/ros_buildfarm:/tmp/ros_buildfarm:ro' + | |||
| ' -v $WORKSPACE/rosdoc2:/tmp/rosdoc2:ro' + | |||
There was a problem hiding this comment.
This mount is no longer useful as we're copying the contents into the image.
| @@ -167,6 +167,8 @@ else: | |||
| 'sleep 1', | |||
| '', | |||
| 'echo "# BEGIN SECTION: Build Dockerfile - doc"', | |||
| '# copy rosdoc2 into image build context directory', | |||
| 'cp -r $WORKSPACE/rosdoc2 $WORKSPACE/docker_doc/rosdoc2', | |||
There was a problem hiding this comment.
In order to be available in the docker context to be copied into the image the rosdoc2 source directory needs to be in the docker context subdirectory.
|
It does indeed fix the test. Now I have to decide if an alternative is preferred. |
There was a problem hiding this comment.
I think that it would make sense to just do the regular mount and drop the read only.
In line 94 it's rmed before cloning so there shouldn't be a risk of persistence between runs
'echo "# BEGIN SECTION: Clone rosdoc2"',
'rm -fr rosdoc2',
'python3 -u $WORKSPACE/ros_buildfarm/scripts/wrapper/git.py clone --depth 1 https://github.com/ros-infrastructure/rosdoc2.git rosdoc2',
Also I noticed there's another instance of mounting rosdoc2 into a container. that would also want to be synchronized either way we go.
There was a problem hiding this comment.
I don't have a strong opinion either way whether we bind mount r/w or take this solution where we copy. I'll let you and @tfoote decide.
Otherwise, thanks for looking into this! It is a huge help.
|
Closing in favor of #1031. |
This is being explored as a fix for #1029.
The deprecated option that we were relying on to allow rosdoc2 to be built from a read-only source directory has been removed and I did not find a suitable PEP517 builder with an out-of-tree option.
I probably could have just cut
:rofrom the docker run invocation and called it a day and I may open that PR to see how it compares. I was trying to avoid modifying the system host's rosdoc2 clone, but given that most of this occurs in the bootstrap task I don't think that these portions of the workspace are preserved at all between job runs so that concern may be groundless.For this solution, I copied in the source directory first to the docker build context and then into the image itself before installing it with
--break-system-packageswhich seems to be required even when installing into a local user directory.One thing I don't like about this approach and the mixing of pip and break-system-packages is the fact that dependencies unsatisfied when
pip install . ...is invoked will be pulled in from pip along with our local project. I would like to mitigate this with the installation of dependencies from apt before pip is invoked with an option not to also install dependencies. Whether I do this with a full rosdep setup or by eye is TBD.