[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 1/2] tests/docker/docker.py: support --qemu
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [RFC PATCH 1/2] tests/docker/docker.py: support --qemu option |
Date: |
Tue, 31 May 2016 16:23:38 +0100 |
User-agent: |
mu4e 0.9.17; emacs 25.0.94.4 |
Fam Zheng <address@hidden> 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 <address@hidden>
>
> 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
Re: [Qemu-devel] [RFC PATCH 0/2] Support building qemu-user powered docker test images, Fam Zheng, 2016/05/27