[RFC,1/2] tests/docker/docker.py: support --qemu option

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>
---
 tests/docker/docker.py | 58 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 6 deletions(-)

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

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

On Tue, 05/31 16:23, Alex Bennée wrote:
> >> +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?

As said below, let's assume the user provides an absolute path or a relative
path against the working directory, so we don't need to worry about path
guessing. The script caller should have more information.

Fam