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.

Sign up for GitHub

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

Abuild keygen fixes #52

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

Abuild keygen fixes #52

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53db2c1
dabuild: mount gitconfig in container
clandmeter Dec 28, 2019
4b844ae
dabuild: create mount directories with correct permissions
clandmeter Dec 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions dabuild.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@ else
esac
fi

## check $DABUILD_PACKAGES is a writable directory
ABUILD_PACKAGES=${DABUILD_PACKAGES:-${PWD%/aports/*}/packages/${ABUILD_VERSION}}

mkdir -p $ABUILD_PACKAGES
# docker will create mount points as root if not available so create them first
# use uid 1000 as this is the uid of the builder user in the container
install -d -o 1000 -g 1000 "$ABUILD_PACKAGES" "$HOME"/.abuild
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops -- thanks for catching that I don't check "$HOME/".abuild exists.

Not sure about creating them with [UG]ID=1000 though. Maybe better to invoke docker run ... below with --user "$(id -u)":"$(id -g)"? (I'm only on D4M here and it looks like some magic is being done to map through to my host user account in either case, so can't easily test.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is setting explicitly 1000 a problem? the first user is always assigned 1000?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, i was thinking only from a point of view from the container, its probably not smart to set 1000 if the current host user has a different uid (which is possible). I guess we need to rethink how to manage those bind mounts or instead use docker volumes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes-- mapping between the uid spaces in container and on host is always a fiddle.

AIUI, in general there are uids and gids defined in the container, some (typically, most) with corresponding user and group names; and there are uids and gids similarly on the host. None of these need to overlap. When the image is built, I create the builder(1000):builder(1000) user and that will be the user by which commands are executed by default.

At this point, the script is running on the host so should (IMO) be creating files/directories using the default host's uid:gid (ie., no -o or -g switches to install).

When we docker run then ideally we would create files using the uid:gid of the user on the host. But the CLI mechanism for doing that (AIUI) is to use the numeric IDs by passing in --user $(id -u):$(id -g). Unfortunately, that may prevent use of sudo inside the container because there is a good chance that the uid that ends up being used will not exist in /etc/passwd in the container and so sudo will refuse to run.

Perhaps a compromise would be to leave the uid in the container as-is (ie., builder(1000)) but to add something like --group-add $(id -G | sed 's/ / --group-add /g')" to the docker run ... command line -- assuming directories are created with g+w then at least file creation from inside the container will work while still letting us use sudo inside the container...?

(Unfortunately I can't test any of this as D4M does magic to make the mappings work behind my back -- on Mac/Windows it's extra complicated because there are effectively three layers of namespace involved: the actual host (Mac/Windows), the Linux VM, container.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Or we could just give up using bind mounts at all, and use named volumes for everything. But extracting built packages becomes trickier then. Maybe that doesn't matter though?)


## check $DABUILD_PACKAGES is a writable directory
if [ ! \( -d "$ABUILD_PACKAGES" -a -w "$ABUILD_PACKAGES" \) ]; then
die "Error: invalid or unwritable packages directory specified!" \
"ABUILD_PACKAGES = '$ABUILD_PACKAGES'" \
Expand All @@ -81,6 +84,11 @@ if [ -w "/var/cache/distfiles" ]; then
ABUILD_VOLUMES="$ABUILD_VOLUMES -v /var/cache/distfiles:/var/cache/distfiles"
fi

# pass over gitconfig for abuild-keygen
if [ -f "$HOME/.gitconfig" ]; then
ABUILD_VOLUMES="$ABUILD_VOLUMES -v $HOME/.gitconfig:/home/builder/.gitconfig"
fi

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM. (Didn't realise abuild-keygen used git and needed .gitconfig...)

if [ "$DABUILD_CACHE" = "true" ]; then
for v in %%ABUILD_VOLUMES%% ; do
vol=abuild-$ABUILD_VERSION-$DABUILD_ARCH-${v//\//_}
Expand Down