[Merge] lp:~mvo/ubuntu-release-upgrader/lp1309447 into lp:ubuntu-release-upgrader

[Barry Warsaw]

On Apr 22, 2014, at 12:17 PM, Michael Vogt wrote:

>Add robustness for systems that run in C or unsupported locale.
>
>This will add extra robustness when a sources.list file has utf8 comments but the release-upgrader is
>- run in C locale
>- run in a unsupported locale
>
>It should not do any harm otherwise.

=== modified file 'DistUpgrade/DistUpgradeMain.py'
--- DistUpgrade/DistUpgradeMain.py	2014-04-14 16:39:29 +0000
+++ DistUpgrade/DistUpgradeMain.py	2014-04-22 10:32:08 +0000
> @@ -28,6 +28,7 @@
>  import atexit
>  import gettext
>  import glob
> +import locale
>  import logging
>  import os
>  import shutil
> @@ -218,6 +219,21 @@
>          localedir = os.path.join(os.getcwd(), "mo")
>          gettext.bindtextdomain("ubuntu-release-upgrader", localedir)
>  
> +    # if the system runs in the C locale, use C.UTF-8 for utf-8 
> +    # python defaults (LP: #1309447)
> +    if (os.environ.get("LANG", None) in [None, "C"] and
> +        locale.getpreferredencoding() in ["ascii", "ANSI_X3.4-1968"]):
> +        os.putenv("LC_ALL", "C.UTF-8")

I would probably use tuples as the RHS operand for the `in` operation.  It's
just a style thing, but I usually prefer tuples for constants like this
because they're a little cheaper to create.  Feel free to ignore this since it
won't have any practical difference in performance.

> +
> +    # try setting locale and fallback to C.UTF-8 (LP: #1309447)
> +    try:
> +        locale.setlocale(locale.LC_ALL, "")
> +    except locale.Error:
> +        # use a utf8 default if the user has a unsupported locale
> +        locale.setlocale(locale.LC_CTYPE, "C.UTF-8")
> +    except Exception:
> +        logging.exception("setlocale() failed")

I wonder if you want to continue on if setlocale() fails.  OT1H, users may not
be savvy enough to know how to fix a problem with setlocale(), so *if* you
wanted to bail here, it would be good to provide some hints as to how to fix
or workaround their problem (e.g. "fix your locale by doing XYZ").  OTOH, if
failing here is really unexpected, then letting the exception percolate up or
sys.exit(1) after logging the exception might be better than soldiering on.
If you don't exit, then you might end up with the original bug this patch is
fixing, but I guess that would only happen if they *also* had non-ascii
characters in their sources.list.

> +
>      # create view and app objects
>      view = setup_view(options, config, logdir)

While looking at the bug description in LP: #1309447, I pulled up the
apt_clone.py file.  I wanted to see how `f` was opened such that its
.readlines() method would try to ascii-decode the contents.  I noticed that at
line 250, while the NamedTemporaryFile is opened in binary mode, the
sources.list file is opened in text mode, but with the default encoding.
That's probably not right.  An explicit `encoding='utf-8'` or explicitly
opening in binary mode and manually decoding it is probably a better way to
go, and likely has relevance on this bug.

I'll also note the comment at line 255.  I guess we're "after Ubuntu 14.04"
now. :)

-- 
https://code.launchpad.net/~mvo/ubuntu-release-upgrader/lp1309447/+merge/216692
Your team Ubuntu Core Development Team is subscribed to branch lp:ubuntu-release-upgrader.