Merge lp:~vorlon/germinate/lp.1025818 into lp:~cjwatson/germinate/trunk

[Steve Langasek]

Fix for germinate crashing on quantal livefs buildds, where LANG == C.

[Colin Watson (cjwatson)]

Thanks for this fix.

Could we use io.open instead in Python 3 (or just plain open; either
way, it takes an encoding parameter now), and also use "replace" error
handling rather than falling over any non-UTF-8 data that happens to be
there since text in Packages that isn't ASCII-only is mostly
informational anyway?

See germinate/seeds.py:AtomicFile for an example of handling the Python
2/3 differences here. It's a bit verbose, but I figure it can go away
soon enough anyway once I'm ready to drop Python 2 support from
germinate.

Any chance of a test case to make sure this doesn't regress in future?
Such a test case would also make it a lot easier to ensure that this
still works with both Python 2 and Python 3. It should be possible to
unit-test this rather than needing to run the full germinate script;
germinate/tests/test_archive.py exists already, and perhaps you could
use EnvironmentVarGuard from test.support/test.test_support (as in
ubiquity's tests) to temporarily force LC_ALL=C.

[Steve Langasek (vorlon)]

The code in germinate/seeds.py includes this comment:

                # io.open is available from Python 2.6, but we only use it with
                # Python 3 because it raises exceptions when passed bytes.

So where would bytes be passed from that this would happen? Would this actually be a problem for the case of Packages/Sources files?

Though it looks like we can use open() for python3 but not for python2, which would seem to be preferred if we can get away with it, so it's probably not really worth consolidating this into a single call and avoid the python2 special casing.

Will work up a test case and re-push.

[Colin Watson (cjwatson)]

On Wed, Jul 18, 2012 at 04:37:29PM -0000, Steve Langasek wrote:
> The code in germinate/seeds.py includes this comment:
>
> # io.open is available from Python 2.6, but we only use it with
> # Python 3 because it raises exceptions when passed bytes.
>
> So where would bytes be passed from that this would happen?

Example test failure if I remove that:

  Traceback (most recent call last):
    File "/home/cjwatson/src/ubuntu/germinate/germinate/germinate/tests/test_seeds.py", line 39, in test_creates_file
      test.write("string")
  TypeError: must be unicode, not str

str == bytes in Python 2, so using io.open would effectively be an API
change, requiring that everything be done using unicode objects. I want
to keep the Python 2 API compatible.

> Would this actually be a problem for the case of Packages/Sources files?

On reflection, this case is a bit different from that of
germinate.seeds, because the file objects here are entirely internal to
germinate; so that argument for not using io.open in Python 2 isn't
valid. That said, germinate still intends to be compatible with Python
2.6 for the sake of Launchpad, and io.open was apparently rather slow in
2.6, so I think it would still be better to avoid that.

[lp:~vorlon/germinate/lp.1025818]

524. By Steve Langasek on 2012-07-18

use 'replace' for handling non-UTF-8 data on input, and prefer open() to
codecs.open() for python3

525. By Steve Langasek on 2012-07-21

Update test to verify correct handling of UTF-8 data in Packages files.

[Steve Langasek (vorlon)]

test case and io.open handling pushed. How's that look, Colin?

[Colin Watson (cjwatson)]

Thanks, this looks good. The bytes.decode.encode business in the test case is unfortunate but I can see why you had to do it. There's just one problem, which is that using plain open in the Python 3 case causes pychecker to fail, as shown by 'python setup.py test' (you might not have seen this because pychecker is only run if it's installed):

  Processing module archive (germinate/archive.py)...
  germinate/archive.py:176: Function (open) doesn't support **kwArgs for name (encoding)
  germinate/archive.py:176: Function (open) doesn't support **kwArgs for name (errors)

This is really a pychecker bug in that it doesn't understand Python 3, but it's easy to work around by using the otherwise-synonymous io.open rather than open. I'll fix this up and merge.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2012-06-14 14:32:58 +0000
+++ debian/changelog	2012-07-21 19:41:19 +0000
@@ -1,3 +1,10 @@
+germinate (2.10) UNRELEASED; urgency=low
+
+  * Always open Packages files as UTF-8, regardless of the current locale.
+    LP: #1025818.
+
+ -- Steve Langasek <[e-mail]>  Tue, 17 Jul 2012 13:08:29 -0700
+
 germinate (2.9) unstable; urgency=low
 
   * Support both Python 2 and 3 directly rather than using 2to3.

=== modified file 'germinate/archive.py'
--- germinate/archive.py	2012-06-14 14:18:32 +0000
+++ germinate/archive.py	2012-07-21 19:41:19 +0000
@@ -33,6 +33,7 @@
 import tempfile
 import shutil
 import logging
+import codecs
 
 import apt_pkg
 
@@ -168,7 +169,11 @@
                         except OSError:
                             pass
 
-            return open(fullname, "r")
+            if sys.version_info[0] < 3:
+                return codecs.open(fullname, 'r', 'UTF-8', 'replace')
+            else:
+                return open(fullname, mode='r', encoding='UTF-8',
+                            errors='replace')
 
         tag_files = []
         for mirror in mirrors:

=== modified file 'germinate/tests/test_archive.py'
--- germinate/tests/test_archive.py	2012-01-16 13:58:10 +0000
+++ germinate/tests/test_archive.py	2012-07-21 19:41:19 +0000
@@ -57,12 +57,13 @@
         os.makedirs(source_dir)
         packages = gzip.GzipFile(os.path.join(binary_dir, "Packages.gz"), "w")
         try:
-            packages.write(textwrap.dedent("""\
+            packages.write(textwrap.dedent(b"""\
                 Package: test
                 Version: 1.0
                 Architecture: i386
+                Maintainer: \xc3\xba\xe1\xb8\x83\xc3\xba\xc3\xb1\xc5\xa7\xc5\xaf\x20\xc4\x91\xc9\x99\x76\xe1\xba\xbd\xc5\x82\xc3\xb5\xe1\xb9\x97\xc3\xa8\xc5\x97\xe1\xb9\xa1
+                """.decode("UTF-8")).encode("UTF-8"))
 
-                """).encode("UTF-8"))
         finally:
             packages.close()
         sources = gzip.GzipFile(os.path.join(source_dir, "Sources.gz"), "w")