| Message ID | 1464272863-2285-2-git-send-email-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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]) + 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) + 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) + + 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") + # 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") 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 + if args.qemu: + qbin=_find_user_binary(args.qemu) + 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):
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(-)