[libvirt PATCH 3/4] ci: Expose CI_USER_LOGIN as a Makefile variable for users to use

[Andrea Bolognani]

On Wed, 2021-02-10 at 18:00 +0100, Erik Skultety wrote:
>  # We need the container process to run with current host IDs
>  # so that it can access the passed in build directory
> -CI_UID = $(shell id -u)
> -CI_GID = $(shell id -g)
> +CI_UID = $(shell id -u $(CI_USER_LOGIN))
> +CI_GID = $(shell id -g $(CI_USER_LOGIN))

Please quote uses of $(CI_USER_LOGIN) in the shell.

Also, since you're using the variable here, it would be IMHO cleaner
if you moved the block that contains its definition before this one.

>  # We also need the user's login and home directory to prepare the
>  # environment the way some programs expect it
> -CI_USER_LOGIN = $(shell echo "$$USER")
> -CI_USER_HOME = $(shell echo "$$HOME")
> +CI_USER_LOGIN = $(shell whoami)
> +CI_USER_HOME = $(shell eval echo "~$(CI_USER_LOGIN)")

This use of eval makes me a bit uncomfortable. Can we do

  CI_USER_HOME = $(shell getent passwd "$(CI_USER_LOGIN)" | cut -d: -f6)

instead?

> +	# In case we want to debug in the container, having root is actually
> +	# preferable, so reset the CI_PODMAN_ARGS and don't actually perform
> +	# any uid/gid mapping
> +	ifeq ($(CI_UID), 0)
> +		CI_PODMAN_ARGS=
> +	endif

Setting $(CI_PODMAN_ARGS) only to reset it moments later seems worse
than just doing

  ifneq ($(CI_UID, 0)
      CI_PODMAN_ARGS = \
          ...
          $(NULL)
  endif

The comment is also somewhat misleading: whether or not you're going
to debug in the container, whatever that means, is not really
relevant - the point is simply that performing these mappings is only
necessary when the container process is running as non-root.

>  	@echo "    CI_CLEAN=0          - do not delete '$(CI_SCRATCHDIR)' after completion"
>  	@echo "    CI_REUSE=1          - re-use existing '$(CI_SCRATCHDIR)' content"
> +	@echo "    CI_USER_LOGIN=      - which user should run in the container (default is $$USER)"
>  	@echo "    CI_ENGINE=auto      - container engine to use (podman, docker)"
>  	@echo "    CI_MESON_ARGS=      - extra arguments passed to meson"
>  	@echo "    CI_NINJA_ARGS=      - extra arguments passed to ninja"

We document the default, so this should be

  CI_USER_LOGIN=$$USER - which user should run in the container

And please document this after CI_ENGINE.

-- 
Andrea Bolognani / Red Hat / Virtualization