[Qemu-devel] [PATCH 0/6] docker: Port to Python 3

[Qemu-devel] [PATCH 0/6] docker: Port to Python 3

[Eduardo Habkost]

This series makes tests/docker/docker.py compatible with both
Python 2 and Python 3, and adds type annotation to make
maintenance easier in the future.

A note about dockerfile encoding
--------------------------------

One decision I made while working on this was to open dockerfiles
in text mode instead of binary mode, to make the code simpler and
safer.

This means we won't support dockerfiles that are not valid utf-8
data, but I see that as a feature and not a bug.  :)

Opening dockerfiles in binary mode and treating its contents as
byte sequences instead of text is possible if we really want to,
but I don't think it would be worth the extra code complexity.

Eduardo Habkost (6):
  docker: Use BytesIO instead of StringIO
  docker: Always return int on run()
  docker: Add type annotations
  docker: Use os.environ.items() instead of .iteritems()
  docker: Make _get_so_libs() work on Python 3
  docker: Open dockerfiles in text mode

 tests/docker/docker.py | 115 ++++++++++++++++++++++++++++-------------
 1 file changed, 79 insertions(+), 36 deletions(-)


base-commit: bd4e4a387aa733e40270a7406c7d111f2292de65
prerequisite-patch-id: 83051ebcf718afae38540902b60a0f8e9f91c174
prerequisite-patch-id: 1a35c71f2a58523de78e3ea2e44c5ef1f84bcc4a
prerequisite-patch-id: 5206b4c5a6797ea17eb763da6203e1881d379f2c
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 1/6] docker: Use BytesIO instead of StringIO

[Eduardo Habkost]

The file passed as argument to TarFile.addfile() must be a binary
file, so BytesIO is more appropriate than StringIO.

This is necessary to make the code work on Python 3.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/docker/docker.py | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 8e13f18e6c..0de7662146 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -24,10 +24,7 @@ import tempfile
 import re
 import signal
 from tarfile import TarFile, TarInfo
-try:
-    from StringIO import StringIO
-except ImportError:
-    from io import StringIO
+from io import BytesIO
 from shutil import copy, rmtree
 from pwd import getpwuid
 from datetime import datetime,timedelta
@@ -372,13 +369,13 @@ class UpdateCommand(SubCommand):
                 tmp_tar.add(os.path.realpath(l), arcname=l)
 
         # Create a Docker buildfile
-        df = StringIO()
-        df.write("FROM %s\n" % args.tag)
-        df.write("ADD . /\n")
-        df.seek(0)
+        df = BytesIO()
+        df.write(b"FROM %s\n" % args.tag.encode())
+        df.write(b"ADD . /\n")
 
         df_tar = TarInfo(name="Dockerfile")
-        df_tar.size = len(df.buf)
+        df_tar.size = df.tell()
+        df.seek(0)
         tmp_tar.addfile(df_tar, fileobj=df)
 
         tmp_tar.close()
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 2/6] docker: Always return int on run()

[Eduardo Habkost]

We'll add type annotations to the run() methods, so add 'return'
statements to all the functions so the type checker won't
complain.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/docker/docker.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 0de7662146..e3bfa1cc9e 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -418,7 +418,7 @@ class ProbeCommand(SubCommand):
         except Exception:
             print("no")
 
-        return
+        return 0
 
 
 class CcCommand(SubCommand):
@@ -503,6 +503,7 @@ class CheckCommand(SubCommand):
                     print ("Image less than %d minutes old" % (args.olderthan))
                 return 0
 
+        return 0
 
 def main():
     parser = argparse.ArgumentParser(description="A Docker helper",
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 3/6] docker: Add type annotations

[Eduardo Habkost]

Add type annotations that indicate how the code works today, to
make the conversion to Python 3 easier and safer.

With these type annotations, "mypy -2" is not reporting any
issues, but "mypy" in Python 3 mode reports a few problems:

tests/docker/docker.py:233: error: Argument 1 to "_text_checksum" has incompatible type "str"; expected "bytes"
tests/docker/docker.py:358: error: "_Environ[str]" has no attribute "iteritems"
tests/docker/docker.py:360: error: Argument 3 to "build_image" of "Docker" has incompatible type "bytes"; expected "str"

These problems will be addressed by the following commits.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/docker/docker.py | 44 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index e3bfa1cc9e..db6b463b92 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -29,6 +29,12 @@ from shutil import copy, rmtree
 from pwd import getpwuid
 from datetime import datetime,timedelta
 
+try:
+    from typing import List, Union, Tuple
+except ImportError:
+    # needed only to make type annotations work
+    pass
+
 
 FILTERED_ENV_NAMES = ['ftp_proxy', 'http_proxy', 'https_proxy']
 
@@ -37,13 +43,16 @@ DEVNULL = open(os.devnull, 'wb')
 
 
 def _text_checksum(text):
+    # type: (bytes) -> str
     """Calculate a digest string unique to the text content"""
     return hashlib.sha1(text).hexdigest()
 
 def _file_checksum(filename):
+    # type: (str) -> str
     return _text_checksum(open(filename, 'rb').read())
 
 def _guess_docker_command():
+    # type: () -> List[str]
     """ Guess a working docker command or raise exception if not found"""
     commands = [["docker"], ["sudo", "-n", "docker"]]
     for cmd in commands:
@@ -60,6 +69,7 @@ def _guess_docker_command():
                     commands_txt)
 
 def _copy_with_mkdir(src, root_dir, sub_path='.'):
+    # type: (str, str, str) -> None
     """Copy src into root_dir, creating sub_path as needed."""
     dest_dir = os.path.normpath("%s/%s" % (root_dir, sub_path))
     try:
@@ -73,6 +83,7 @@ def _copy_with_mkdir(src, root_dir, sub_path='.'):
 
 
 def _get_so_libs(executable):
+    # type: (str) -> List[str]
     """Return a list of libraries associated with an executable.
 
     The paths may be symbolic links which would need to be resolved to
@@ -94,6 +105,7 @@ def _get_so_libs(executable):
     return libs
 
 def _copy_binary_with_libs(src, dest_dir):
+    # type: (str, str) -> None
     """Copy a binary executable and all its dependant libraries.
 
     This does rely on the host file-system being fairly multi-arch
@@ -108,11 +120,13 @@ def _copy_binary_with_libs(src, dest_dir):
             _copy_with_mkdir(l , dest_dir, so_path)
 
 def _read_qemu_dockerfile(img_name):
+    # type: (str) -> str
     df = os.path.join(os.path.dirname(__file__), "dockerfiles",
                       img_name + ".docker")
     return open(df, "r").read()
 
 def _dockerfile_preprocess(df):
+    # type: (str) -> str
     out = ""
     for l in df.splitlines():
         if len(l.strip()) == 0 or l.startswith("#"):
@@ -194,11 +208,16 @@ class Docker(object):
         labels = json.loads(resp)[0]["Config"].get("Labels", {})
         return labels.get("com.qemu.dockerfile-checksum", "")
 
-    def build_image(self, tag, docker_dir, dockerfile,
-                    quiet=True, user=False, argv=None, extra_files_cksum=[]):
-        if argv == None:
-            argv = []
-
+    def build_image(self,
+                    tag,                 # type: str
+                    docker_dir,          # type: str
+                    dockerfile,          # type: str
+                    quiet=True,          # type: bool
+                    user=False,          # type: bool
+                    argv=[],             # type: List[str]
+                    extra_files_cksum=[] # List[Tuple[str, bytes]]
+                    ):
+        # type(...) -> None
         tmp_df = tempfile.NamedTemporaryFile(dir=docker_dir, suffix=".docker")
         tmp_df.write(dockerfile)
 
@@ -249,7 +268,8 @@ class Docker(object):
 
 class SubCommand(object):
     """A SubCommand template base class"""
-    name = None # Subcommand name
+    # Subcommand name
+    name = None # type: str
     def shared_args(self, parser):
         parser.add_argument("--quiet", action="store_true",
                             help="Run quietly unless an error occured")
@@ -258,6 +278,7 @@ class SubCommand(object):
         """Setup argument parser"""
         pass
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         """Run command.
         args: parsed argument by argument parser.
         argv: remaining arguments from sys.argv.
@@ -271,6 +292,7 @@ class RunCommand(SubCommand):
         parser.add_argument("--keep", action="store_true",
                             help="Don't remove image when command completes")
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         return Docker().run(argv, args.keep, quiet=args.quiet)
 
 class BuildCommand(SubCommand):
@@ -294,6 +316,7 @@ class BuildCommand(SubCommand):
                             help="Dockerfile name")
 
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         dockerfile = open(args.dockerfile, "rb").read()
         tag = args.tag
 
@@ -321,7 +344,7 @@ class BuildCommand(SubCommand):
 
             # Copy any extra files into the Docker context. These can be
             # included by the use of the ADD directive in the Dockerfile.
-            cksum = []
+            cksum = []  # type: List[Tuple[bytes, str]]
             if args.include_executable:
                 # FIXME: there is no checksum of this executable and the linked
                 # libraries, once the image built any change of this executable
@@ -352,6 +375,7 @@ class UpdateCommand(SubCommand):
                             help="Executable to copy")
 
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         # Create a temporary tarball with our whole build context and
         # dockerfile for the update
         tmp = tempfile.NamedTemporaryFile(suffix="dckr.tar.gz")
@@ -394,6 +418,7 @@ class CleanCommand(SubCommand):
     """Clean up docker instances"""
     name = "clean"
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         Docker().clean()
         return 0
 
@@ -401,6 +426,7 @@ class ImagesCommand(SubCommand):
     """Run "docker images" command"""
     name = "images"
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         return Docker().command("images", argv, args.quiet)
 
 
@@ -409,6 +435,7 @@ class ProbeCommand(SubCommand):
     name = "probe"
 
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         try:
             docker = Docker()
             if docker._command[0] == "docker":
@@ -437,6 +464,7 @@ class CcCommand(SubCommand):
                             reading sources""")
 
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         if argv and argv[0] == "--":
             argv = argv[1:]
         cwd = os.getcwd()
@@ -468,6 +496,7 @@ class CheckCommand(SubCommand):
                             help="number of minutes")
 
     def run(self, args, argv):
+        # type: (argparse.Namespace, List[str]) -> int
         tag = args.tag
 
         dkr = Docker()
@@ -506,6 +535,7 @@ class CheckCommand(SubCommand):
         return 0
 
 def main():
+    # type: () -> int
     parser = argparse.ArgumentParser(description="A Docker helper",
             usage="%s <subcommand> ..." % os.path.basename(sys.argv[0]))
     subparsers = parser.add_subparsers(title="subcommands", help=None)
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 4/6] docker: Use os.environ.items() instead of .iteritems()

[Eduardo Habkost]

Mapping.iteritems() doesn't exist in Python 3.

Note that Mapping.items() exists in both Python 3 and Python 2,
but it returns a list (and not an iterator) in Python 2.  The
existing code will work on both cases, though.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/docker/docker.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index db6b463b92..bc34bd872b 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -355,7 +355,7 @@ class BuildCommand(SubCommand):
                 cksum += [(filename, _file_checksum(filename))]
 
             argv += ["--build-arg=" + k.lower() + "=" + v
-                        for k, v in os.environ.iteritems()
+                        for k, v in os.environ.items()
                         if k.lower() in FILTERED_ENV_NAMES]
             dkr.build_image(tag, docker_dir, dockerfile,
                             quiet=args.quiet, user=args.user, argv=argv,
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 5/6] docker: Make _get_so_libs() work on Python 3

[Eduardo Habkost]

The "ldd" output is a byte sequence, not a string.  Use bytes
literals while handling the output, and use os.fsdecode() on the
resulting file paths before returning.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/docker/docker.py | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index bc34bd872b..f58af8e894 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -41,6 +41,15 @@ FILTERED_ENV_NAMES = ['ftp_proxy', 'http_proxy', 'https_proxy']
 
 DEVNULL = open(os.devnull, 'wb')
 
+def _fsdecode(name):
+    # type: (bytes) -> str
+    """Decode filename to str, try to use os.fsdecode() if available"""
+    if hasattr(os, 'fsdecode'):
+        # Python 3
+        return os.fsdecode(name) # type: ignore
+    else:
+        # Python 2.7
+        return name # type: ignore
 
 def _text_checksum(text):
     # type: (bytes) -> str
@@ -90,15 +99,15 @@ def _get_so_libs(executable):
     ensure theright data is copied."""
 
     libs = []
-    ldd_re = re.compile(r"(/.*/)(\S*)")
+    ldd_re = re.compile(b"(/.*/)(\S*)")
     try:
         ldd_output = subprocess.check_output(["ldd", executable])
-        for line in ldd_output.split("\n"):
+        for line in ldd_output.split(b"\n"):
             search = ldd_re.search(line)
             if search and len(search.groups()) == 2:
                 so_path = search.groups()[0]
                 so_lib = search.groups()[1]
-                libs.append("%s/%s" % (so_path, so_lib))
+                libs.append(_fsdecode(b"%s/%s" % (so_path, so_lib)))
     except subprocess.CalledProcessError:
         print("%s had no associated libraries (static build?)" % (executable))
 
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 6/6] docker: Open dockerfiles in text mode

[Eduardo Habkost]

Instead of treating dockerfile contents as byte sequences, always
open dockerfiles in text mode and treat it as text.

This is not strictly required to make the script compatible with
Python 3, but it's a simpler and safer way than opening
dockerfiles in binary mode and decoding the data data later.

To make the code compatible with both Python 2 and 3, use
io.open(), which accepts a 'encoding' argument on both versions.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/docker/docker.py | 46 ++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index f58af8e894..412a031c1c 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -23,6 +23,7 @@ import argparse
 import tempfile
 import re
 import signal
+import io
 from tarfile import TarFile, TarInfo
 from io import BytesIO
 from shutil import copy, rmtree
@@ -30,7 +31,7 @@ from pwd import getpwuid
 from datetime import datetime,timedelta
 
 try:
-    from typing import List, Union, Tuple
+    from typing import List, Union, Tuple, Text
 except ImportError:
     # needed only to make type annotations work
     pass
@@ -52,13 +53,13 @@ def _fsdecode(name):
         return name # type: ignore
 
 def _text_checksum(text):
-    # type: (bytes) -> str
+    # type: (Text) -> str
     """Calculate a digest string unique to the text content"""
-    return hashlib.sha1(text).hexdigest()
+    return hashlib.sha1(text.encode('utf-8')).hexdigest()
 
 def _file_checksum(filename):
     # type: (str) -> str
-    return _text_checksum(open(filename, 'rb').read())
+    return _text_checksum(io.open(filename, 'r', encoding='utf-8').read())
 
 def _guess_docker_command():
     # type: () -> List[str]
@@ -129,14 +130,14 @@ def _copy_binary_with_libs(src, dest_dir):
             _copy_with_mkdir(l , dest_dir, so_path)
 
 def _read_qemu_dockerfile(img_name):
-    # type: (str) -> str
+    # type: (Text) -> str
     df = os.path.join(os.path.dirname(__file__), "dockerfiles",
                       img_name + ".docker")
-    return open(df, "r").read()
+    return io.open(df, "r", encoding='utf-8').read()
 
 def _dockerfile_preprocess(df):
-    # type: (str) -> str
-    out = ""
+    # type: (Text) -> Text
+    out = u""
     for l in df.splitlines():
         if len(l.strip()) == 0 or l.startswith("#"):
             continue
@@ -149,7 +150,7 @@ def _dockerfile_preprocess(df):
             inlining = _read_qemu_dockerfile(l[len(from_pref):])
             out += _dockerfile_preprocess(inlining)
             continue
-        out += l + "\n"
+        out += l + u"\n"
     return out
 
 class Docker(object):
@@ -220,32 +221,37 @@ class Docker(object):
     def build_image(self,
                     tag,                 # type: str
                     docker_dir,          # type: str
-                    dockerfile,          # type: str
+                    dockerfile,          # type: Text
                     quiet=True,          # type: bool
                     user=False,          # type: bool
                     argv=[],             # type: List[str]
                     extra_files_cksum=[] # List[Tuple[str, bytes]]
                     ):
         # type(...) -> None
-        tmp_df = tempfile.NamedTemporaryFile(dir=docker_dir, suffix=".docker")
+        tmp_ndf = tempfile.NamedTemporaryFile(dir=docker_dir, suffix=".docker")
+        # on Python 2.7, NamedTemporaryFile doesn't support encoding parameter,
+        # so reopen it in text mode:
+        tmp_df = io.open(tmp_ndf.name, mode='w+t', encoding='utf-8')
         tmp_df.write(dockerfile)
 
         if user:
             uid = os.getuid()
             uname = getpwuid(uid).pw_name
-            tmp_df.write("\n")
-            tmp_df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
+            tmp_df.write(u"\n")
+            tmp_df.write(u"RUN id %s 2>/dev/null || useradd -u %d -U %s" %
                          (uname, uid, uname))
 
-        tmp_df.write("\n")
-        tmp_df.write("LABEL com.qemu.dockerfile-checksum=%s" %
-                     _text_checksum(_dockerfile_preprocess(dockerfile)))
+        dockerfile = _dockerfile_preprocess(dockerfile)
+
+        tmp_df.write(u"\n")
+        tmp_df.write(u"LABEL com.qemu.dockerfile-checksum=%s" %
+                     _text_checksum(dockerfile))
         for f, c in extra_files_cksum:
-            tmp_df.write("LABEL com.qemu.%s-checksum=%s" % (f, c))
+            tmp_df.write(u"LABEL com.qemu.%s-checksum=%s" % (f, c))
 
         tmp_df.flush()
 
-        self._do_check(["build", "-t", tag, "-f", tmp_df.name] + argv + \
+        self._do_check(["build", "-t", tag, "-f", tmp_ndf.name] + argv + \
                        [docker_dir],
                        quiet=quiet)
 
@@ -326,7 +332,7 @@ class BuildCommand(SubCommand):
 
     def run(self, args, argv):
         # type: (argparse.Namespace, List[str]) -> int
-        dockerfile = open(args.dockerfile, "rb").read()
+        dockerfile = io.open(args.dockerfile, "r", encoding='utf-8').read()
         tag = args.tag
 
         dkr = Docker()
@@ -519,7 +525,7 @@ class CheckCommand(SubCommand):
                 print("Need a dockerfile for tag:%s" % (tag))
                 return 1
 
-            dockerfile = open(args.dockerfile, "rb").read()
+            dockerfile = io.open(args.dockerfile, "r", encoding='utf-8').read()
 
             if dkr.image_matches_dockerfile(tag, dockerfile):
                 if not args.quiet:
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 4/6] docker: Use os.environ.items() instead of .iteritems()

[Philippe Mathieu-Daudé]

On 06/26/2018 11:14 PM, Eduardo Habkost wrote:
> Mapping.iteritems() doesn't exist in Python 3.
> 
> Note that Mapping.items() exists in both Python 3 and Python 2,
> but it returns a list (and not an iterator) in Python 2.  The
> existing code will work on both cases, though.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/docker/docker.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index db6b463b92..bc34bd872b 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -355,7 +355,7 @@ class BuildCommand(SubCommand):
>                  cksum += [(filename, _file_checksum(filename))]
>  
>              argv += ["--build-arg=" + k.lower() + "=" + v
> -                        for k, v in os.environ.iteritems()
> +                        for k, v in os.environ.items()
>                          if k.lower() in FILTERED_ENV_NAMES]
>              dkr.build_image(tag, docker_dir, dockerfile,
>                              quiet=args.quiet, user=args.user, argv=argv,
> 

Re: [Qemu-devel] [PATCH 6/6] docker: Open dockerfiles in text mode

[Eric Blake]

On 06/26/2018 09:14 PM, Eduardo Habkost wrote:
> Instead of treating dockerfile contents as byte sequences, always
> open dockerfiles in text mode and treat it as text.
> 
> This is not strictly required to make the script compatible with
> Python 3, but it's a simpler and safer way than opening
> dockerfiles in binary mode and decoding the data data later.

s/data data/data/

> 
> To make the code compatible with both Python 2 and 3, use
> io.open(), which accepts a 'encoding' argument on both versions.

How does this compare to the recent change to the QAPI generators in 
commit de685ae5e?  Should we be trying to use similar mechanisms in both 
places?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   tests/docker/docker.py | 46 ++++++++++++++++++++++++------------------
>   1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index f58af8e894..412a031c1c 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -23,6 +23,7 @@ import argparse
>   import tempfile
>   import re
>   import signal
> +import io
>   from tarfile import TarFile, TarInfo
>   from io import BytesIO
>   from shutil import copy, rmtree
> @@ -30,7 +31,7 @@ from pwd import getpwuid
>   from datetime import datetime,timedelta
>   
>   try:
> -    from typing import List, Union, Tuple
> +    from typing import List, Union, Tuple, Text
>   except ImportError:
>       # needed only to make type annotations work
>       pass
> @@ -52,13 +53,13 @@ def _fsdecode(name):
>           return name # type: ignore
>   
>   def _text_checksum(text):
> -    # type: (bytes) -> str
> +    # type: (Text) -> str
>       """Calculate a digest string unique to the text content"""
> -    return hashlib.sha1(text).hexdigest()
> +    return hashlib.sha1(text.encode('utf-8')).hexdigest()
>   
>   def _file_checksum(filename):
>       # type: (str) -> str
> -    return _text_checksum(open(filename, 'rb').read())
> +    return _text_checksum(io.open(filename, 'r', encoding='utf-8').read())
>   
>   def _guess_docker_command():
>       # type: () -> List[str]
> @@ -129,14 +130,14 @@ def _copy_binary_with_libs(src, dest_dir):
>               _copy_with_mkdir(l , dest_dir, so_path)
>   
>   def _read_qemu_dockerfile(img_name):
> -    # type: (str) -> str
> +    # type: (Text) -> str
>       df = os.path.join(os.path.dirname(__file__), "dockerfiles",
>                         img_name + ".docker")
> -    return open(df, "r").read()
> +    return io.open(df, "r", encoding='utf-8').read()
>   
>   def _dockerfile_preprocess(df):
> -    # type: (str) -> str
> -    out = ""
> +    # type: (Text) -> Text
> +    out = u""
>       for l in df.splitlines():
>           if len(l.strip()) == 0 or l.startswith("#"):
>               continue
> @@ -149,7 +150,7 @@ def _dockerfile_preprocess(df):
>               inlining = _read_qemu_dockerfile(l[len(from_pref):])
>               out += _dockerfile_preprocess(inlining)
>               continue
> -        out += l + "\n"
> +        out += l + u"\n"
>       return out
>   
>   class Docker(object):
> @@ -220,32 +221,37 @@ class Docker(object):
>       def build_image(self,
>                       tag,                 # type: str
>                       docker_dir,          # type: str
> -                    dockerfile,          # type: str
> +                    dockerfile,          # type: Text
>                       quiet=True,          # type: bool
>                       user=False,          # type: bool
>                       argv=[],             # type: List[str]
>                       extra_files_cksum=[] # List[Tuple[str, bytes]]
>                       ):
>           # type(...) -> None
> -        tmp_df = tempfile.NamedTemporaryFile(dir=docker_dir, suffix=".docker")
> +        tmp_ndf = tempfile.NamedTemporaryFile(dir=docker_dir, suffix=".docker")
> +        # on Python 2.7, NamedTemporaryFile doesn't support encoding parameter,
> +        # so reopen it in text mode:
> +        tmp_df = io.open(tmp_ndf.name, mode='w+t', encoding='utf-8')
>           tmp_df.write(dockerfile)
>   
>           if user:
>               uid = os.getuid()
>               uname = getpwuid(uid).pw_name
> -            tmp_df.write("\n")
> -            tmp_df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
> +            tmp_df.write(u"\n")
> +            tmp_df.write(u"RUN id %s 2>/dev/null || useradd -u %d -U %s" %
>                            (uname, uid, uname))
>   
> -        tmp_df.write("\n")
> -        tmp_df.write("LABEL com.qemu.dockerfile-checksum=%s" %
> -                     _text_checksum(_dockerfile_preprocess(dockerfile)))
> +        dockerfile = _dockerfile_preprocess(dockerfile)
> +
> +        tmp_df.write(u"\n")
> +        tmp_df.write(u"LABEL com.qemu.dockerfile-checksum=%s" %
> +                     _text_checksum(dockerfile))
>           for f, c in extra_files_cksum:
> -            tmp_df.write("LABEL com.qemu.%s-checksum=%s" % (f, c))
> +            tmp_df.write(u"LABEL com.qemu.%s-checksum=%s" % (f, c))
>   
>           tmp_df.flush()
>   
> -        self._do_check(["build", "-t", tag, "-f", tmp_df.name] + argv + \
> +        self._do_check(["build", "-t", tag, "-f", tmp_ndf.name] + argv + \
>                          [docker_dir],
>                          quiet=quiet)
>   
> @@ -326,7 +332,7 @@ class BuildCommand(SubCommand):
>   
>       def run(self, args, argv):
>           # type: (argparse.Namespace, List[str]) -> int
> -        dockerfile = open(args.dockerfile, "rb").read()
> +        dockerfile = io.open(args.dockerfile, "r", encoding='utf-8').read()
>           tag = args.tag
>   
>           dkr = Docker()
> @@ -519,7 +525,7 @@ class CheckCommand(SubCommand):
>                   print("Need a dockerfile for tag:%s" % (tag))
>                   return 1
>   
> -            dockerfile = open(args.dockerfile, "rb").read()
> +            dockerfile = io.open(args.dockerfile, "r", encoding='utf-8').read()
>   
>               if dkr.image_matches_dockerfile(tag, dockerfile):
>                   if not args.quiet:
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] docker: Open dockerfiles in text mode

[Eduardo Habkost]

On Wed, Jun 27, 2018 at 07:51:01AM -0500, Eric Blake wrote:
> On 06/26/2018 09:14 PM, Eduardo Habkost wrote:
> > Instead of treating dockerfile contents as byte sequences, always
> > open dockerfiles in text mode and treat it as text.
> > 
> > This is not strictly required to make the script compatible with
> > Python 3, but it's a simpler and safer way than opening
> > dockerfiles in binary mode and decoding the data data later.
> 
> s/data data/data/

Thanks.

> 
> > 
> > To make the code compatible with both Python 2 and 3, use
> > io.open(), which accepts a 'encoding' argument on both versions.
> 
> How does this compare to the recent change to the QAPI generators in commit
> de685ae5e?  Should we be trying to use similar mechanisms in both places?

We could do the same and use io.open(..., encoding='utf-8')
unconditionally on QAPI too, but it may be harder because of its
usage of insinstance() and the 'str' type[1] everywhere.

One solution I considered on QAPI was using
'__future__.unicode_literals' and the 'builtins.str' type from
python-future, but I don't think QAPI's specific case justify
adding a new build dependency on Python 2.7 hosts.

Adding type annotations to the QAPI code may help us sort out the
mess more easily.

[1] 'str' on Python 2.7 is more similar to the 'bytes' type on
    Python 3, but io.open(..., encoding=...) on Python 2.7
    returns 'unicode' objects (that that are more similar to the
    native 'str' type from Python 3).

-- 
Eduardo