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

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

Fam Zheng <famz@redhat.com> writes:

> 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?

Yes I kinda avoided the complexity here. Do we have a programatic way of
finding this out or should we just assume we get based a resolvable path?

>
>> +    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()?

Ack.

>
>> +        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"?

It's really a NOP to behave in a mkdir -p type way. Python 3 provides an
exist_ok parameter but I assume we aren't ready to drop python 2 just
yet ;-)

>
>> +
>> +    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?

Yeah, also a bit hacky. In my original shell script it actually searched
the output of Debian's binfmt_update script to see where it was set to
search for the qemu binary from.

>
>> +            # 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.

Makes sense.

>
>>          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 "="?

Ack.

>
>> +        if args.qemu:
>> +            qbin=_find_user_binary(args.qemu)
>
> Ditto, and some more occasions above.

Ack.

>
>> +
>>          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


--
Alex Bennée