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.ben...@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
