Bazaar

Code review comment for lp:~gz/bzr/setlocale_on_posix_only_631350

setlocale_on_posix_only_631350
Merge into 2.2

Revision history for this message

John A Meinel (jameinel) wrote on 2010-09-07:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 9/7/2010 7:23 AM, Martin [gz] wrote:
> Martin [gz] has proposed merging lp:~gz/bzr/setlocale_on_posix_only_631350 into lp:bzr/2.2.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #631350 Python 2.6 @ win32: print non-ascii characters to console produce borked output
> https://bugs.launchpad.net/bugs/631350
>
>
> This branch fixes a serious and highly confusing issue with non-ascii terminal output Alexander hit using the bzr form the 2.2 windows installer. The immediate cause is the upgrade there to Python 2.6, but the underlying issue is a behaviour change involving setlocale the newer windows C runtime used.
>
> The C locale needs to care about encodings because it determines the behaviour of various functions that deal in bytes, mostly in output of localised text like day names in strftime. One of the categories also deals with character types and case conversion so deals with bytes input, a poor (or old) man's unicodedata.
>
> The problem appears to have arisen from some VS coder thinking it would be a good idea to use this locale setting to decode any bytes written to the (unicode) console, rather than following the previous practice of using the console output encoding to determine what they mean. Piping is a bytes->bytes operation and is not affected.
>
> As there is little benefit to trying to use what little localisation the C language provides outside it's native land, and great scope for breakage, this changes the startup script to only call setlocale on posix systems. The Python folks have been working around locale support for years, see for instance <http://bugs.python.org/issue774665> so there's not actually much left affected by it. However, as LC_CTYPE is the only problem category here, an alternative would be to still set everything else on all platforms. But then we'd really want to record the codepage we get back from LC_TIME so we know how to decode what we get back from strftime and so on.
>
> I've got some ideas on how to test this, the problem is it involves writing to the console, which is generally a shared resource between the test runner and the test framework. It's possible to create a new console to doodle in, but flashing up new windows in testing isn't great either.

This seems fine to me, but:

1) Definitely needs a NEWS entry
2) Why did you move the location of the call?

John
=:->

merge: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyGgIAACgkQJdeBCYSNAAOKXgCgyBoJRVl3JeFIe5swO8cLfQO9
BPEAoI65uDWHIlrt5P8saIzuMsP2JxzP
=dnwQ
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 9/7/2010 7:23 AM, Martin [gz] wrote:
> Martin [gz] has proposed merging lp:~gz/bzr/setlocale_on_posix_only_631350 into lp:bzr/2.2.
> 
> Requested reviews:
>   bzr-core (bzr-core)
> Related bugs:
>   #631350 Python 2.6 @ win32: print non-ascii characters to console produce borked output
>   https://bugs.launchpad.net/bugs/631350
> 
> 
> This branch fixes a serious and highly confusing issue with non-ascii terminal output Alexander hit using the bzr form the 2.2 windows installer. The immediate cause is the upgrade there to Python 2.6, but the underlying issue is a behaviour change involving setlocale the newer windows C runtime used.
> 
> The C locale needs to care about encodings because it determines the behaviour of various functions that deal in bytes, mostly in output of localised text like day names in strftime. One of the categories also deals with character types and case conversion so deals with bytes input, a poor (or old) man's unicodedata.
> 
> The problem appears to have arisen from some VS coder thinking it would be a good idea to use this locale setting to decode any bytes written to the (unicode) console, rather than following the previous practice of using the console output encoding to determine what they mean. Piping is a bytes->bytes operation and is not affected.
> 
> As there is little benefit to trying to use what little localisation the C language provides outside it's native land, and great scope for breakage, this changes the startup script to only call setlocale on posix systems. The Python folks have been working around locale support for years, see for instance <http://bugs.python.org/issue774665> so there's not actually much left affected by it. However, as LC_CTYPE is the only problem category here, an alternative would be to still set everything else on all platforms. But then we'd really want to record the codepage we get back from LC_TIME so we know how to decode what we get back from strftime and so on.
> 
> I've got some ideas on how to test this, the problem is it involves writing to the console, which is generally a shared resource between the test runner and the test framework. It's possible to create a new console to doodle in, but flashing up new windows in testing isn't great either.

This seems fine to me, but:

1) Definitely needs a NEWS entry
2) Why did you move the location of the call?

John
=:->

merge: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyGgIAACgkQJdeBCYSNAAOKXgCgyBoJRVl3JeFIe5swO8cLfQO9
BPEAoI65uDWHIlrt5P8saIzuMsP2JxzP
=dnwQ
-----END PGP SIGNATURE-----

review: Approve

« Back to merge proposal