Re: [Qemu-devel] [RFC PATCH 1/2] tests/docker/docker.py: support --qemu option

[Fam Zheng <famz@redhat.com>]

On Thu, 05/26 15:27, Alex Bennée wrote:
> When passed the name of a qemu-$arch binary we copy it and any linked
> libraries into the docker build context. These can then be included by a
> dockerfile with the line:
> 
>   # Copy all of context into container
>   ADD . /
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Looks good in general except a few nitpicks below, most important one being the
binary path lookup.

> ---
>  tests/docker/docker.py | 58 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index fe73de7..e9242f3 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -20,6 +20,8 @@ import atexit
>  import uuid
>  import argparse
>  import tempfile
> +import re
> +from shutil import copyfile
>  
>  def _text_checksum(text):
>      """Calculate a digest string unique to the text content"""
> @@ -37,6 +39,27 @@ def _guess_docker_command():
>      raise Exception("Cannot find working docker command. Tried:\n%s" % \
>                      commands_txt)
>  
> +def _find_user_binary(binary_name):
> +    """ Find a binary in the QEMU source tree. Used for finding qemu-$arch."""
> +    top = os.path.abspath("%s/../../.." % sys.argv[0])

What if this is an out of tree build?

> +    linux_user = [ x for x in os.listdir(top) if x.endswith("-linux-user") ]
> +    for x in linux_user:
> +        check_path = "%s/%s/%s" % (top, x, binary_name)

os.path.join()?

> +        if os.path.isfile(check_path):
> +            print ("found %s" % check_path)
> +            return check_path
> +    return None
> +
> +def _copy_with_mkdir(src, root_dir, sub_path):
> +    """Copy src into root_dir, creating sub_path as needed."""
> +    full_path = "%s/%s" % (root_dir, sub_path)
> +    try:
> +        os.makedirs(full_path)
> +    except OSError:
> +        print "skipping %s" % (full_path)

Print the error message too? Also do you want to "return"?

> +
> +    copyfile(src, "%s/%s" % (full_path, os.path.basename(src)))
> +
>  class Docker(object):
>      """ Running Docker commands """
>      def __init__(self):
> @@ -86,18 +109,36 @@ class Docker(object):
>          labels = json.loads(resp)[0]["Config"].get("Labels", {})
>          return labels.get("com.qemu.dockerfile-checksum", "")
>  
> -    def build_image(self, tag, dockerfile, df_path, quiet=True, argv=None):
> +    def build_image(self, tag, dockerfile, quiet=True, qemu=None, argv=None):
>          if argv == None:
>              argv = []
> +
> +        # Create a temporary docker context to build in
> +        tmp_dir = tempfile.mkdtemp(prefix="docker_build")
> +
> +        # Copy the dockerfile into our work space
>          tmp = dockerfile + "\n" + \
>                "LABEL com.qemu.dockerfile-checksum=%s" % \
>                _text_checksum(dockerfile)
> -        dirname = os.path.dirname(df_path)
> -        tmp_df = tempfile.NamedTemporaryFile(dir=dirname)
> +        tmp_df = tempfile.NamedTemporaryFile(dir=tmp_dir, suffix=".docker")
>          tmp_df.write(tmp)
>          tmp_df.flush()
> +
> +        # Do we want to copy QEMU into here?
> +        if qemu:
> +            _copy_with_mkdir(qemu, tmp_dir, "/usr/bin")

Looks like "/usr/bin" is combined with tmp_dir as "FOO//usr/bin", superfluous?

> +            # do ldd bit here
> +            ldd_output = subprocess.check_output(["ldd", qemu])
> +            for l in ldd_output.split("\n"):
> +                s = re.search("(/.*/)(\S*)", l)
> +                if s and len(s.groups())==2:
> +                    so_path=s.groups()[0]
> +                    so_lib=s.groups()[1]
> +                    _copy_with_mkdir("%s/%s" % (so_path, so_lib),
> +                                     tmp_dir, so_path)
> +
>          self._do(["build", "-t", tag, "-f", tmp_df.name] + argv + \
> -                 [dirname],
> +                 [tmp_dir],
>                   quiet=quiet)
>  
>      def image_matches_dockerfile(self, tag, dockerfile):
> @@ -148,6 +189,7 @@ class BuildCommand(SubCommand):
>      """ Build docker image out of a dockerfile. Arguments: <tag> <dockerfile>"""
>      name = "build"
>      def args(self, parser):
> +        parser.add_argument("--qemu", help="Include qemu-user binaries")

Can I ask for a rename of this and the variable names in this patch, to a more
generic name (to reflect that it is inherently orthorgonal to the type of the
binary we are copying)? How about:

           parser.add_argument("--executable-inject", "-e",
                               help="""Specify a binary that will be copied to the
                               container together with all its dependent
                               libraries""")

And I think it is reasonable to expect the user (or the calling Makefile) to
designate a working absolute or relative path, instead of looking up it
ourselves.

>          parser.add_argument("tag",
>                              help="Image Tag")
>          parser.add_argument("dockerfile",
> @@ -157,14 +199,18 @@ class BuildCommand(SubCommand):
>          dockerfile = open(args.dockerfile, "rb").read()
>          tag = args.tag
>  
> +        # find qemu binary
> +        qbin=None

Add whitespaces around "="?

> +        if args.qemu:
> +            qbin=_find_user_binary(args.qemu)

Ditto, and some more occasions above.

> +
>          dkr = Docker()
>          if dkr.image_matches_dockerfile(tag, dockerfile):
>              if not args.quiet:
>                  print "Image is up to date."
>              return 0
>  
> -        dkr.build_image(tag, dockerfile, args.dockerfile,
> -                        quiet=args.quiet, argv=argv)
> +        dkr.build_image(tag, dockerfile, quiet=args.quiet, qemu=qbin, argv=argv)
>          return 0
>  
>  class CleanCommand(SubCommand):
> -- 
> 2.7.4
> 

Thanks,

Fam