Re: [PULL 1/1] tests/docker: ensure container command is probed at most once

["Alex Bennée" <alex.bennee@linaro.org>]

Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com> writes:

> From: Daniel P. Berrangé <berrange@redhat.com>
>
> The '--engine' arg accepts either 'podman' or 'docker', which is
> not sufficiently granular to map directly to a command. This
> means that  docker.py still has to then probe the exact command
> to use.
>
> Meanwhile the 'probe' command prints out the full command to use
> but this cannot be passed back to docker.py to avoid probing
> again, so the caching is only useful in the few case where we
> run a container directly bypassing docker.py.
>
> Address this by replacing --engine with --command for docker.py.
>
> This in turn requires the --container-engine configure arg to be
> replaced with --container-command.
>
> With these changes the container command is probed at most once
> during configure and never again, while running in an unconfigured
> tree will still probe on demand.
>
> Tested-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Link: https://lore.kernel.org/qemu-devel/20260716102515.1503404-1-berrange@redhat.com
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>

I've bisected to this as breaking --disable-containers:

  ../../configure --target-list=aarch64-linux-user,loongarch64-linux-user --disable-containers
  ...
    Cross compilers                       
      aarch64                         : aarch64-linux-gnu-gcc
      loongarch64                     : cc --cc loongarch64-unknown-linux-gnu-gcc -i qemu/debian-loongarch-cross -s /home/alex.bennee/lsrc/qemu.git --

So some mangling with the container command line has happened.

> ---
>  configure                     | 25 +++++++------
>  tests/docker/Makefile.include | 13 ++++---
>  tests/docker/docker.py        | 67 ++++++++++++-----------------------
>  3 files changed, 40 insertions(+), 65 deletions(-)
>
> diff --git a/configure b/configure
> index d8bc10060e1..6ddfe4b5342 100755
> --- a/configure
> +++ b/configure
> @@ -172,7 +172,7 @@ fi
>  # some defaults, based on the host environment
>  
>  # default parameters
> -container_engine="auto"
> +container_command=""
>  cpu=""
>  cross_compile="no"
>  cross_prefix=""
> @@ -734,7 +734,7 @@ for opt do
>    ;;
>    --disable-containers) use_containers="no"
>    ;;
> -  --container-engine=*) container_engine="$optarg"
> +  --container-command=*) container_command="$optarg"
>    ;;
>    --rust-target-triple=*) rust_target_triple="$optarg"
>    ;;
> @@ -869,7 +869,7 @@ Advanced options (experts only):
>    --enable-debug           enable common debug build options
>    --cpu=CPU                Build for host CPU [$cpu]
>    --disable-containers     don't use containers for cross-building
> -  --container-engine=TYPE  which container engine to use [$container_engine]
> +  --container-command=CMD  which container command to use [autodetect]
>    --gdb=GDB-path           gdb to use for gdbstub tests [$gdb_bin]
>    --wasm64-32bit-address-limit Restrict wasm64 address space to 32-bit (default
>                                 is to use the whole 64-bit range).
> @@ -1291,12 +1291,12 @@ fi
>  ##########################################
>  # functions to probe cross compilers
>  
> -runc="no"
> -if test $use_containers = "yes" && (has "docker" || has "podman"); then
> -    runc=$($python "$source_path"/tests/docker/docker.py --engine "$container_engine" probe)
> -    if test "$runc" != "no"; then
> -        docker_py="$python $source_path/tests/docker/docker.py --engine $container_engine"
> -    fi
> +if test "$container_command" = ""; then
> +    container_command=$($python "$source_path"/tests/docker/docker.py probe)
> +    test "$container_command" = "no" && container_command=""
> +fi
> +if test $use_containers = "yes" && test "$container_command" != ""; then
> +    docker_py="$python $source_path/tests/docker/docker.py --command $container_command"

else
  container_command=""
fi

?

>  
>  # cross compilers defaults, can be overridden with --cross-cc-ARCH
> @@ -1415,7 +1415,7 @@ probe_target_compiler() {
>    esac
>  
>    for host in $container_hosts; do
> -    test "$runc" != no || continue
> +    test "$container_command" != "" || continue
>      test "$host" = "$cpu" || continue
>      case $target_arch in
>        # debian-all-test-cross architectures
> @@ -1736,9 +1736,8 @@ echo all: >> $config_host_mak
>  echo "SRC_PATH=$source_path" >> $config_host_mak
>  echo "TARGET_DIRS=$target_list" >> $config_host_mak
>  echo "GDB=$gdb_bin" >> $config_host_mak
> -if test "$runc" != no; then
> -    echo "RUNC=$runc" >> $config_host_mak
> -    echo "CONTAINER_ENGINE=$container_engine" >> $config_host_mak
> +if test "$container_command" != ""; then
> +    echo "CONTAINER_COMMAND=$container_command" >> $config_host_mak
>  fi
>  echo "SUBDIRS=$subdirs" >> $config_host_mak
>  if test "$rust" != disabled; then
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index 4725c39807b..0adddb6a5c6 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -16,9 +16,8 @@ DOCKER_DEFAULT_REGISTRY := registry.gitlab.com/qemu-project/qemu
>  endif
>  DOCKER_REGISTRY := $(if $(REGISTRY),$(REGISTRY),$(DOCKER_DEFAULT_REGISTRY))
>  
> -CONTAINER_ENGINE = auto
> -DOCKER_SCRIPT=$(SRC_PATH)/tests/docker/docker.py --engine $(CONTAINER_ENGINE)
> -RUNC ?= $(shell $(DOCKER_SCRIPT) probe)
> +CONTAINER_COMMAND ?= $(shell $(SRC_PATH)/tests/docker/docker.py probe)
> +DOCKER_SCRIPT=$(SRC_PATH)/tests/docker/docker.py --command "$(CONTAINER_COMMAND)"
>  
>  CUR_TIME := $(shell date +%Y-%m-%d-%H.%M.%S.$$$$)
>  DOCKER_SRC_COPY := $(BUILD_DIR)/docker-src.$(CUR_TIME)
> @@ -41,7 +40,7 @@ docker-qemu-src: $(DOCKER_SRC_COPY)
>  # General rule for building docker images.
>  docker-image-%: $(DOCKER_FILES_DIR)/%.docker
>  	  $(call quiet-command,			\
> -		DOCKER_BUILDKIT=1 $(RUNC) build		\
> +		DOCKER_BUILDKIT=1 $(CONTAINER_COMMAND) build \
>  		$(if $(DOCKER_V),,--quiet)		\
>  		$(if $(NOCACHE),--no-cache,		\
>  			$(if $(DOCKER_REGISTRY),--cache-from $(DOCKER_REGISTRY)/qemu/$*)) \
> @@ -152,7 +151,7 @@ $(foreach i,$(filter-out $(DOCKER_PARTIAL_IMAGES),$(DOCKER_IMAGES)), \
>  )
>  
>  docker:
> -	@echo 'Build QEMU and run tests inside $(RUNC) containers'
> +	@echo 'Build QEMU and run tests inside $(CONTAINER_COMMAND) containers'
>  	@echo
>  	@echo 'Available targets:'
>  	@echo
> @@ -219,10 +218,10 @@ docker-run: docker-qemu-src
>  			$(IMAGE) --executable $(EXECUTABLE),		\
>  			"  COPYING $(EXECUTABLE) to $(IMAGE)"))
>  	$(call quiet-command,						\
> -		$(RUNC) run 						\
> +		$(CONTAINER_COMMAND) run 				\
>  			--rm						\
>  			$(if $(NOUSER),,				\
> -				$(if $(filter docker,$(RUNC)),		\
> +				$(if $(filter docker,$(CONTAINER_COMMAND)), \
>  					-u $(UID),			\
>  					--userns keep-id		\
>  				)					\
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index 9e18b984f45..d2f39b5645b 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -35,27 +35,6 @@
>  
>  DEVNULL = open(os.devnull, 'wb')
>  
> -class EngineEnum(enum.IntEnum):
> -    AUTO = 1
> -    DOCKER = 2
> -    PODMAN = 3
> -
> -    def __str__(self):
> -        return self.name.lower()
> -
> -    def __repr__(self):
> -        return str(self)
> -
> -    @staticmethod
> -    def argparse(s):
> -        try:
> -            return EngineEnum[s.upper()]
> -        except KeyError:
> -            return s
> -
> -
> -USE_ENGINE = EngineEnum.AUTO
> -
>  def _bytes_checksum(bytes):
>      """Calculate a digest string unique to the text content"""
>      return hashlib.sha1(bytes).hexdigest()
> @@ -73,12 +52,11 @@ def _file_checksum(filename):
>  
>  def _guess_engine_command():
>      """ Guess a working engine command or raise exception if not found"""
> -    commands = []
> -
> -    if USE_ENGINE in [EngineEnum.AUTO, EngineEnum.PODMAN]:
> -        commands += [["podman"], ["podman-remote"], ["podman", "--remote"]]
> -    if USE_ENGINE in [EngineEnum.AUTO, EngineEnum.DOCKER]:
> -        commands += [["docker"], ["sudo", "-n", "docker"]]
> +    commands = [["podman"],
> +                ["podman-remote"],
> +                ["podman", "--remote"],
> +                ["docker"],
> +                ["sudo", "-n", "docker"]]
>      for cmd in commands:
>          try:
>              # 'version' is not sufficient to prove a working binary
> @@ -222,8 +200,11 @@ def _dockerfile_verify_flat(df):
>  
>  class Docker(object):
>      """ Running Docker commands """
> -    def __init__(self):
> -        self._command = _guess_engine_command()
> +    def __init__(self, commandstr=None):
> +        if commandstr is None:
> +            self._command = _guess_engine_command()
> +        else:
> +            self._command = commandstr.split(" ")
>  
>          if ("docker" in self._command and
>              "TRAVIS" not in os.environ and
> @@ -411,8 +392,8 @@ def args(self, parser):
>                              help="Run container using the current user's uid")
>  
>      def run(self, args, argv):
> -        return Docker().run(argv, args.keep, quiet=args.quiet,
> -                            as_user=args.run_as_current_user)
> +        return Docker(args.command).run(argv, args.keep, quiet=args.quiet,
> +                                        as_user=args.run_as_current_user)
>  
>  
>  class BuildCommand(SubCommand):
> @@ -445,7 +426,7 @@ def run(self, args, argv):
>          dockerfile = _read_dockerfile(args.dockerfile)
>          tag = args.tag
>  
> -        dkr = Docker()
> +        dkr = Docker(args.command)
>          if "--no-cache" not in argv and \
>             dkr.image_matches_dockerfile(tag, dockerfile):
>              if not args.quiet:
> @@ -512,7 +493,7 @@ def args(self, parser):
>                              help="Docker registry")
>  
>      def run(self, args, argv):
> -        dkr = Docker()
> +        dkr = Docker(args.command)
>          dkr.command(cmd="pull", quiet=args.quiet,
>                      argv=["%s/%s" % (args.registry, args.tag)])
>          dkr.command(cmd="tag", quiet=args.quiet,
> @@ -590,7 +571,7 @@ def run(self, args, argv):
>          tmp.seek(0)
>  
>          # Run the build with our tarball context
> -        dkr = Docker()
> +        dkr = Docker(args.command)
>          dkr.update_image(args.tag, tmp, quiet=args.quiet)
>  
>          return 0
> @@ -601,7 +582,7 @@ class CleanCommand(SubCommand):
>      name = "clean"
>  
>      def run(self, args, argv):
> -        Docker().clean()
> +        Docker(args.command).clean()
>          return 0
>  
>  
> @@ -610,7 +591,7 @@ class ImagesCommand(SubCommand):
>      name = "images"
>  
>      def run(self, args, argv):
> -        return Docker().command("images", argv, args.quiet)
> +        return Docker(args.command).command("images", argv, args.quiet)
>  
>  
>  class ProbeCommand(SubCommand):
> @@ -619,7 +600,7 @@ class ProbeCommand(SubCommand):
>  
>      def run(self, args, argv):
>          try:
> -            docker = Docker()
> +            docker = Docker(args.command)
>              print(" ".join(docker._command))
>          except Exception:
>              print("no")
> @@ -651,18 +632,16 @@ def run(self, args, argv):
>                  cmd += ["-v", "%s:%s:ro,z" % (p, p)]
>          cmd += [args.image, args.cc]
>          cmd += argv
> -        return Docker().run(cmd, False, quiet=args.quiet,
> -                            as_user=True)
> +        return Docker(args.command).run(cmd, False, quiet=args.quiet,
> +                                        as_user=True)
>  
>  
>  def main():
> -    global USE_ENGINE
> -
>      parser = argparse.ArgumentParser(description="A Docker helper",
>                                       usage="%s <subcommand> ..." %
>                                       os.path.basename(sys.argv[0]))
> -    parser.add_argument("--engine", type=EngineEnum.argparse, choices=list(EngineEnum),
> -                        help="specify which container engine to use")
> +    parser.add_argument("--command",
> +                        help="specify which container engine command to use")
>      subparsers = parser.add_subparsers(title="subcommands", help=None)
>      for cls in SubCommand.__subclasses__():
>          cmd = cls()
> @@ -671,8 +650,6 @@ def main():
>          cmd.args(subp)
>          subp.set_defaults(cmdobj=cmd)
>      args, argv = parser.parse_known_args()
> -    if args.engine:
> -        USE_ENGINE = args.engine
>      return args.cmdobj.run(args, argv)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro