Unify --docker-image handling

This avoids having to change multiple places when changing the handling of
this option.

Also gets rid of a few untested branches, to make sure that the --docker-image
handling is always tested.

Also, make sure --docker-extra-args is handled in a uniform way.

alibuild_helpers/args.py

@@ -6,6 +6,7 @@
 
 import re
 import os
+import shlex
 
 try:
   import commands
@@ -113,7 +114,7 @@ def doParseArgs():
   """)
   build_docker.add_argument("--docker", dest="docker", action="store_true",
                             help="Build inside a Docker container.")
-  build_docker.add_argument("--docker-image", dest="dockerImage", metavar="IMAGE", default=argparse.SUPPRESS,
+  build_docker.add_argument("--docker-image", dest="dockerImage", metavar="IMAGE", default=None,
                             help=("The Docker image to build inside of. Implies --docker. "
                                   "By default, an image is chosen based on the architecture."))
   build_docker.add_argument("--docker-extra-args", metavar="ARGLIST", default="",
@@ -217,7 +218,7 @@ def doParseArgs():
   """)
   deps_docker.add_argument("--docker", dest="docker", action="store_true",
                            help="Check for available system packages inside a Docker container.")
-  deps_docker.add_argument("--docker-image", dest="dockerImage", metavar="IMAGE",
+  deps_docker.add_argument("--docker-image", dest="dockerImage", metavar="IMAGE", default=None,
                            help=("The Docker image to use. Implies --docker. By default, an image "
                                  "is chosen based on the current or selected architecture."))
   deps_docker.add_argument("--docker-extra-args", default="", metavar="ARGLIST",
@@ -263,9 +264,13 @@ def doParseArgs():
   """)
   doctor_docker.add_argument("--docker", dest="docker", action="store_true",
                              help="Check for available system packages inside a Docker container.")
-  doctor_docker.add_argument("--docker-image", dest="dockerImage", metavar="IMAGE",
+  doctor_docker.add_argument("--docker-image", dest="dockerImage", metavar="IMAGE", default=None,
                              help=("The Docker image to use. Implies --docker. By default, an image "
                                    "is chosen based on the current or selected architecture."))
+  doctor_docker.add_argument("--docker-extra-args", metavar="ARGLIST", default="",
+                             help=("Command-line arguments to pass to 'docker run'. "
+                                   "Passed through verbatim -- separate multiple arguments "
+                                   "with spaces, and make sure quoting is correct! Implies --docker."))
 
   doctor_dirs = doctor_parser.add_argument_group(title="Customise aliBuild directories")
   doctor_dirs.add_argument("-C", "--chdir", metavar="DIR", dest="chdir", default=DEFAULT_CHDIR,
@@ -386,6 +391,25 @@ def finaliseArgs(args, parser):
     # stale git logs from previous invocations.
     cleanup_git_log(args.referenceSources)
 
+  if args.action in ("build", "doctor", "deps"):
+    if args.dockerImage or args.docker_extra_args:
+      args.docker = True
+
+    args.docker_extra_args = shlex.split(args.docker_extra_args)
+    args.docker_extra_args.append("--network=host")
+
+    if args.docker and args.architecture.startswith("osx"):
+      parser.error("cannot use `-a %s` and --docker" % args.architecture)
+
+    if args.docker and commands.getstatusoutput("which docker")[0]:
+      parser.error("cannot use --docker as docker executable is not found")
+
+    # If specified, used the docker image requested, otherwise, if running
+    # in docker the docker image is given by the first part of the
+    # architecture we want to build for.
+    if args.docker and not args.dockerImage:
+      args.dockerImage = "registry.cern.ch/alisw/%s-builder" % args.architecture.split("_")[0]
+
   if args.action == "build":
     args.configDir = args.configDir
 
@@ -400,21 +424,6 @@ def finaliseArgs(args, parser):
     if args.remoteStore or args.writeStore:
       args.noSystem = True
 
-    if "dockerImage" in args or args.docker_extra_args:
-      args.docker = True
-
-    if args.docker and args.architecture.startswith("osx"):
-      parser.error("cannot use `-a %s` and --docker" % args.architecture)
-
-    if args.docker and commands.getstatusoutput("which docker")[0]:
-      parser.error("cannot use --docker as docker executable is not found")
-
-    # If specified, used the docker image requested, otherwise, if running
-    # in docker the docker image is given by the first part of the
-    # architecture we want to build for.
-    if args.docker and not "dockerImage" in args:
-      args.dockerImage = "registry.cern.ch/alisw/%s-builder" % args.architecture.split("_")[0]
-
     if args.remoteStore.endswith("::rw") and args.writeStore:
       parser.error("cannot specify ::rw and --write-store at the same time")
 
@@ -428,7 +437,7 @@ def finaliseArgs(args, parser):
         args.develPrefix = basename(abspath(args.chdir))
       else:
         args.develPrefix = basename(dirname(abspath(args.configDir)))
-    if "dockerImage" in args:
+    if getattr(args, "docker", False):
       args.develPrefix = "%s-%s" % (args.develPrefix, args.architecture) if "develPrefix" in args else args.architecture
 
   if args.action == "init":

alibuild_helpers/build.py

@@ -335,7 +335,6 @@ def doBuild(args, parser):
     syncHelper = NoRemoteSync()
 
   packages = args.pkgname
-  dockerImage = args.dockerImage if "dockerImage" in args else None
   specs = {}
   buildOrder = []
   workDir = abspath(args.workDir)
@@ -376,7 +375,7 @@ def doBuild(args, parser):
 
   install_wrapper_script("git", workDir)
 
-  with DockerRunner(dockerImage, ["--network=host"]) as getstatusoutput_docker:
+  with DockerRunner(args.dockerImage, args.docker_extra_args) as getstatusoutput_docker:
     my_gzip = "pigz" if getstatusoutput_docker("which pigz")[0] == 0 else "gzip"
     my_tar = ("tar --ignore-failed-read"
               if getstatusoutput_docker("tar --ignore-failed-read -cvvf "
@@ -1082,10 +1081,10 @@ def doBuild(args, parser):
         "{mirrorVolume} {develVolumes} {additionalEnv} {additionalVolumes} "
         "{overrideSource} {extraArgs} {image} bash -ex /build.sh"
       ).format(
-        image=quote(dockerImage),
+        image=quote(args.dockerImage),
         workdir=quote(abspath(args.workDir)),
         scriptDir=quote(scriptDir),
-        extraArgs=args.docker_extra_args if "docker_extra_args" in args else "",
+        extraArgs=" ".join(map(quote, args.docker_extra_args)),
         overrideSource="-e SOURCE0_DIR_OVERRIDE=/" if source.startswith("/") else "",
         additionalEnv=" ".join(
           "-e {}={}".format(var, quote(value)) for var, value in buildEnvironment),

alibuild_helpers/deps.py

@@ -13,16 +13,11 @@ def doDeps(args, parser):
   if not args.outgraph:
     parser.error("Specify a PDF output file with --outgraph")
 
-  # In case we are using Docker
-  dockerImage = args.dockerImage if "dockerImage" in args else ""
-  if args.docker and not dockerImage:
-    dockerImage = "registry.cern.ch/alisw/%s-builder" % args.architecture.split("_")[0]
-
   # Resolve all the package parsing boilerplate
   specs = {}
   defaultsReader = lambda: readDefaults(args.configDir, args.defaults, parser.error, args.architecture)
   (err, overrides, taps) = parseDefaults(args.disable, defaultsReader, debug)
-  with DockerRunner(dockerImage, ["--network=host"]) as getstatusoutput_docker:
+  with DockerRunner(args.dockerImage, args.docker_extra_args) as getstatusoutput_docker:
     systemPackages, ownPackages, failed, validDefaults = \
       getPackageList(packages                = [args.package],
                      specs                   = specs,

alibuild_helpers/doctor.py

@@ -107,11 +107,6 @@ def doDoctor(args, parser):
   if err:
     homebrew_replacement = "brew() { true; }; "
 
-
-  dockerImage = args.dockerImage if "dockerImage" in args else ""
-  if args.docker and not dockerImage:
-    dockerImage = "registry.cern.ch/alisw/%s-builder" % args.architecture.split("_")[0]
-
   logger.setLevel(logging.BANNER)
   if args.debug:
     logger.setLevel(logging.DEBUG)
@@ -141,7 +136,7 @@ def performValidateDefaults(spec):
       error("%s", msg)
     return (ok,msg,valid)
 
-  with DockerRunner(dockerImage, ["--network=host"]) as getstatusoutput_docker:
+  with DockerRunner(args.dockerImage, args.docker_extra_args) as getstatusoutput_docker:
     fromSystem, own, failed, validDefaults = \
       getPackageList(packages                = packages,
                      specs                   = specs,

tests/test_args.py

@@ -59,9 +59,10 @@ class FakeExit(Exception):
   ((), "build --force-unknown-architecture zlib --no-remote-store --remote-store rsync://test.local/", [("noSystem", False), ("remoteStore", "")]),
   ((), "build zlib --architecture slc7_x86-64"                                         , [("noSystem", True), ("preferSystem", False), ("remoteStore", "https://s3.cern.ch/swift/v1/alibuild-repo")]),
   ((), "build zlib --architecture ubuntu1804_x86-64"                                   , [("noSystem", False), ("preferSystem", False), ("remoteStore", "")]),
-  ((), "build zlib -a slc7_x86-64 --docker-image registry.cern.ch/alisw/slc7-builder"  , [("docker", True), ("dockerImage", "registry.cern.ch/alisw/slc7-builder")]),
+  ((), "build zlib -a slc7_x86-64"                                                     , [("docker", False), ("dockerImage", None), ("docker_extra_args", ["--network=host"])]),
   ((), "build zlib -a slc7_x86-64 --docker-image registry.cern.ch/alisw/some-builder"  , [("docker", True), ("dockerImage", "registry.cern.ch/alisw/some-builder")]),
   ((), "build zlib -a slc7_x86-64 --docker"                                            , [("docker", True), ("dockerImage", "registry.cern.ch/alisw/slc7-builder")]),
+  ((), "build zlib -a slc7_x86-64 --docker-extra-args=--foo"                           , [("docker", True), ("dockerImage", "registry.cern.ch/alisw/slc7-builder"), ("docker_extra_args", ["--foo", "--network=host"])]),
   ((), "build zlib --devel-prefix -a slc7_x86-64 --docker"                             , [("docker", True), ("dockerImage", "registry.cern.ch/alisw/slc7-builder"), ("develPrefix", "%s-slc7_x86-64" % os.path.basename(os.getcwd()))]),
   ((), "build zlib --devel-prefix -a slc7_x86-64 --docker-image someimage"             , [("docker", True), ("dockerImage", "someimage"), ("develPrefix", "%s-slc7_x86-64" % os.path.basename(os.getcwd()))]),
   ((), "--debug build --force-unknown-architecture --defaults o2 O2"                   , [("debug", True), ("action",  "build"), ("defaults", "o2"), ("pkgname", ["O2"])]),

tests/test_build.py

@@ -248,6 +248,8 @@ def test_coverDoBuild(self, mock_debug, mock_glob, mock_sys, mock_git_git):
             writeStore="",
             referenceSources="/sw/MIRROR",
             docker=False,
+            dockerImage=None,
+            docker_extra_args=["--network=host"],
             architecture="osx_x86-64",
             workDir="/sw",
             pkgname=["root"],

tests/test_deps.py

@@ -61,6 +61,8 @@ def depsOpen(fn, mode):
                          configDir="/dist",
                          debug=False,
                          docker=False,
+                         dockerImage=None,
+                         docker_extra_args=["--network=host"],
                          preferSystem=[],
                          noSystem=True,
                          architecture="slc7_x86-64",

tests/test_doctor.py

@@ -83,6 +83,8 @@ def resetOut():
     args = Namespace(workDir="/work",
                      configDir="/dist",
                      docker=False,
+                     dockerImage=None,
+                     docker_extra_args=["--network=host"],
                      debug=False,
                      preferSystem=[],
                      noSystem=False,