[gentoo-portage-dev] Re: [PATCH v2] repoman: declare '-x', '--xmlparse' command line options obsolete

2016-05-23 Thread Duncan
Alexander Berntsen posted on Mon, 23 May 2016 12:56:06 +0200 as excerpted:

> When submitting v2-patches, please submit them in-reply-to=the message
> ID of the original thread.

It isn't mandatory here and many ignore it, but many readers (like me) 
and more importantly reviewers find a short, often one-line description 
of what changed between versions useful as well.  A multi-revision patch 
will thus have a mini-changelog of what happened at each revision.  While 
not mandatory here, it is considered so on many lists including most linux 
kernel related lists.

> The patch looks OK, but I don't recall us using the "OBSOLETE" phrase in
> any documentation. Does anybody know any phrase we should be using
> instead, and why we should be using that phrase instead?
> 
> Maybe we should pick a term ("OBSOLETE" is fine by me), and make it
> "official" for these situations in the future.

No argument with obsolete here, but as long as the option is still 
allowed (even if ignored) for backward compatibility, isn't "deprecated" 
the usual term?

Then "obsolete" can be reserved for continued listing (for historical 
reasons...) after the option is no longer allowed (whether it directly 
triggers an error or simply isn't processed at all, thus likely 
triggering an indirect error due to incorrect parsing of other options 
and parameters on the command line).

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman




Re: [gentoo-portage-dev] [PATCH 2/2] pym/portage/util/locale.py: add a C module to help check locale

2016-05-23 Thread Michał Górny
On Mon, 23 May 2016 08:08:18 -0400
"Anthony G. Basile"  wrote:

> On 5/23/16 2:44 AM, Michał Górny wrote:
> > On Sun, 22 May 2016 13:04:40 -0400
> > "Anthony G. Basile"  wrote:
> >   
> >> From: "Anthony G. Basile" 
> >>
> >> The current method to check for a sane system locale is to use python's
> >> ctypes.util.find_library() to construct a full library path to the
> >> system libc.so and pass that path to ctypes.CDLL() so we can call
> >> toupper() and tolower() directly.  However, this gets bogged down in
> >> implementation details and fails with musl.
> >>
> >> We work around this design flaw in ctypes with a small python module
> >> written in C which provides thin wrappers to toupper() and tolower(),
> >> and only fall back on the current ctypes-based check when this module
> >> is not available.
> >>
> >> This has been tested on glibc, uClibc and musl systems.
> >>
> >> X-Gentoo-bug: 571444
> >> X-Gentoo-bug-url: https://bugs.gentoo.org/show_bug.cgi?id=571444
> >>
> >> Signed-off-by: Anthony G. Basile 
> >> ---
> >>  pym/portage/util/locale.py   | 32 ++-
> >>  setup.py |  6 ++-
> >>  src/portage_c_convert_case.c | 94 
> >> 
> >>  3 files changed, 121 insertions(+), 11 deletions(-)
> >>  create mode 100644 src/portage_c_convert_case.c
> >>
> >> diff --git a/pym/portage/util/locale.py b/pym/portage/util/locale.py
> >> index 2a15ea1..85ddd2b 100644
> >> --- a/pym/portage/util/locale.py
> >> +++ b/pym/portage/util/locale.py
> >> @@ -11,6 +11,7 @@ from __future__ import absolute_import, unicode_literals
> >>  import locale
> >>  import logging
> >>  import os
> >> +import sys
> >>  import textwrap
> >>  import traceback
> >>  
> >> @@ -34,18 +35,26 @@ def _check_locale(silent):
> >>"""
> >>The inner locale check function.
> >>"""
> >> -
> >> -  libc_fn = find_library("c")
> >> -  if libc_fn is None:
> >> -  return None
> >> -  libc = LoadLibrary(libc_fn)
> >> -  if libc is None:
> >> -  return None
> >> +  try:
> >> +  from portage_c_convert_case import _c_toupper, _c_tolower
> >> +  libc_tolower = _c_tolower
> >> +  libc_toupper = _c_toupper  
> > 
> > Now I'm being picky... but if you named the functions toupper()
> > and tolower(), you could actually import the whole module as 'libc'
> > and have less code!  
> 
> I see what you're saying, and its tempting because its elegant, but I'm
> afraid of a clash of names.  I've got a bad feeling this will get us
> into trouble later.
> 
> Let me play with this and see what happens.

I don't think this will be problematic since things like this happen
in Python all the time ;-). And after all, C function names can be
different than Python function names.

> > Also it would be nice to actually make the module more generic. There
> > are more places where we use CDLL, and all of them could eventually be
> > supported by the module (unshare() would be much better done in C, for
> > example).  
> 
> Yeah I get your point here.  Let me convince myself first.

I've got a killer argument: right now we hardcode constants from Linux
headers in the Python code!

Not that I'm asking you to actually add code for that as well. Just
rename the module to something more generic like portage.util.libc ;-).

> >> +  except ImportError:
> >> +  writemsg_level("!!! Unable to import 
> >> portage_c_convert_case\n!!!\n",
> >> +  level=logging.WARNING, noiselevel=-1)  
> > 
> > Do we really want to warn verbosely about this? I think it'd be
> > a pretty common case for people running the git checkout.  
> 
> This should stay.  Its good to know that the module is not being
> imported and silently falling back on the ctypes stuff.
> 
> 1) its only going to happen in the rare occasion that you're using
> something like a turkish locale and can't import the module.

Wrong. This happens before the check is done, so it will be output
every time Portage is started, also with good locale.

> 2) people who do a git checkout should add
> PYTHONPATH=build/lib.linux-x86_64-3.4 to their env to test the module.
> I can add something to testpath.  Users will have to be instructed to
> run `./setup build` and then the script shoudl read something like this
> 
> unamem=$(uname -m)
> 
> pythonversion=$(python --version 2>&1 | cut -c8-)
> pythonversion=${pythonversion%\.*}
> 
> portagedir=$(dirname ${BASH_SOURCE[0]})
> 
> export PATH="${portagedir}/bin:${PATH}"
> 
> export
> PYTHONPATH="${portagedir}/build/lib.linux-${unamem}-${pythonversion}:${portagedir}/pym:${PYTHONPATH:+:}${PYTHONPATH}"
> 
> export PYTHONWARNINGS=d,i::ImportWarning
> 
> 
> BTW, the original code must have a bug in it.  It reads
> 
> export PYTHONPATH=PYTHONPATH="$(dirname
> $BASH_SOURCE[0])/pym:${PYTHONPATH:+:}${PYTHONPATH}"
> 
> The double PYTHONPATH=PYTHONPATH= can't be right.

You 

Re: [gentoo-portage-dev] [PATCH 2/2] pym/portage/util/locale.py: add a C module to help check locale

2016-05-23 Thread Anthony G. Basile
On 5/23/16 2:44 AM, Michał Górny wrote:
> On Sun, 22 May 2016 13:04:40 -0400
> "Anthony G. Basile"  wrote:
> 
>> From: "Anthony G. Basile" 
>>
>> The current method to check for a sane system locale is to use python's
>> ctypes.util.find_library() to construct a full library path to the
>> system libc.so and pass that path to ctypes.CDLL() so we can call
>> toupper() and tolower() directly.  However, this gets bogged down in
>> implementation details and fails with musl.
>>
>> We work around this design flaw in ctypes with a small python module
>> written in C which provides thin wrappers to toupper() and tolower(),
>> and only fall back on the current ctypes-based check when this module
>> is not available.
>>
>> This has been tested on glibc, uClibc and musl systems.
>>
>> X-Gentoo-bug: 571444
>> X-Gentoo-bug-url: https://bugs.gentoo.org/show_bug.cgi?id=571444
>>
>> Signed-off-by: Anthony G. Basile 
>> ---
>>  pym/portage/util/locale.py   | 32 ++-
>>  setup.py |  6 ++-
>>  src/portage_c_convert_case.c | 94 
>> 
>>  3 files changed, 121 insertions(+), 11 deletions(-)
>>  create mode 100644 src/portage_c_convert_case.c
>>
>> diff --git a/pym/portage/util/locale.py b/pym/portage/util/locale.py
>> index 2a15ea1..85ddd2b 100644
>> --- a/pym/portage/util/locale.py
>> +++ b/pym/portage/util/locale.py
>> @@ -11,6 +11,7 @@ from __future__ import absolute_import, unicode_literals
>>  import locale
>>  import logging
>>  import os
>> +import sys
>>  import textwrap
>>  import traceback
>>  
>> @@ -34,18 +35,26 @@ def _check_locale(silent):
>>  """
>>  The inner locale check function.
>>  """
>> -
>> -libc_fn = find_library("c")
>> -if libc_fn is None:
>> -return None
>> -libc = LoadLibrary(libc_fn)
>> -if libc is None:
>> -return None
>> +try:
>> +from portage_c_convert_case import _c_toupper, _c_tolower
>> +libc_tolower = _c_tolower
>> +libc_toupper = _c_toupper
> 
> Now I'm being picky... but if you named the functions toupper()
> and tolower(), you could actually import the whole module as 'libc'
> and have less code!

I see what you're saying, and its tempting because its elegant, but I'm
afraid of a clash of names.  I've got a bad feeling this will get us
into trouble later.

Let me play with this and see what happens.

> 
> Also it would be nice to actually make the module more generic. There
> are more places where we use CDLL, and all of them could eventually be
> supported by the module (unshare() would be much better done in C, for
> example).

Yeah I get your point here.  Let me convince myself first.

> 
>> +except ImportError:
>> +writemsg_level("!!! Unable to import 
>> portage_c_convert_case\n!!!\n",
>> +level=logging.WARNING, noiselevel=-1)
> 
> Do we really want to warn verbosely about this? I think it'd be
> a pretty common case for people running the git checkout.

This should stay.  Its good to know that the module is not being
imported and silently falling back on the ctypes stuff.

1) its only going to happen in the rare occasion that you're using
something like a turkish locale and can't import the module.

2) people who do a git checkout should add
PYTHONPATH=build/lib.linux-x86_64-3.4 to their env to test the module.
I can add something to testpath.  Users will have to be instructed to
run `./setup build` and then the script shoudl read something like this

unamem=$(uname -m)

pythonversion=$(python --version 2>&1 | cut -c8-)
pythonversion=${pythonversion%\.*}

portagedir=$(dirname ${BASH_SOURCE[0]})

export PATH="${portagedir}/bin:${PATH}"

export
PYTHONPATH="${portagedir}/build/lib.linux-${unamem}-${pythonversion}:${portagedir}/pym:${PYTHONPATH:+:}${PYTHONPATH}"

export PYTHONWARNINGS=d,i::ImportWarning


BTW, the original code must have a bug in it.  It reads

export PYTHONPATH=PYTHONPATH="$(dirname
$BASH_SOURCE[0])/pym:${PYTHONPATH:+:}${PYTHONPATH}"

The double PYTHONPATH=PYTHONPATH= can't be right.

> 
>> +libc_fn = find_library("c")
>> +if libc_fn is None:
>> +return None
>> +libc = LoadLibrary(libc_fn)
>> +if libc is None:
>> +return None
>> +libc_tolower = libc.tolower
>> +libc_toupper = libc.toupper
>>  
>>  lc = list(range(ord('a'), ord('z')+1))
>>  uc = list(range(ord('A'), ord('Z')+1))
>> -rlc = [libc.tolower(c) for c in uc]
>> -ruc = [libc.toupper(c) for c in lc]
>> +rlc = [libc_tolower(c) for c in uc]
>> +ruc = [libc_toupper(c) for c in lc]
>>  
>>  if lc != rlc or uc != ruc:
>>  if silent:
>> @@ -62,7 +71,10 @@ def _check_locale(silent):
>>  "as LC_CTYPE in make.conf.")
>>  msg = [l for l in textwrap.wrap(msg, 70)]
>>  

Re: [gentoo-portage-dev] [PATCH v2] repoman: declare '-x', '--xmlparse' command line options obsolete

2016-05-23 Thread Alexander Berntsen
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

When submitting v2-patches, please submit them in-reply-to=the message
ID of the original thread.

The patch looks OK, but I don't recall us using the "OBSOLETE" phrase
in any documentation. Does anybody know any phrase we should be using
instead, and why we should be using that phrase instead?

Maybe we should pick a term ("OBSOLETE" is fine by me), and make it
"official" for these situations in the future.
- -- 
Alexander
berna...@gentoo.org
https://secure.plaimi.net/~alexander
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCgAGBQJXQuHFAAoJENQqWdRUGk8B74sP/3gMywA7AWshVzsyzEA2PRf2
LNaW0vb98zSq5DbRuqN5624i83g20wk2GHNm/8ZqS1yuTrjNmJ3XjXJriQwCIryL
cJTvz0FxrOdGcmT7/SWTI3vT0P6yOA5s+rrNp8jBPqeDQFAcy8eZbTDa8A9KtWmZ
w6ZQ+ht3mQqLt3VkADDbzqX5/w1szi8isQXkFFKUn4en54A9KQsmsFWRqKYNpUPK
7HXSTHhvPtSyoYxjBxKVd+chbDHVFbIqVWuuJfoSro3RiKDnCjzxYjuqcP8mRxfp
s9O0LCynAwWBrTQwBJd0FRPp5Y95vRENCP+18XnT4cI3S+2TdJAptW+MfZrAXKz/
9OKJfuwkdvpOqdNtpk0mmbe19CIiCEFiZXP+wC+8JtyuEE+gwfUJaPbz5MFNLqRo
TLPHvjphrtmyvasa8ZxocB7mF9uCwZSaPbxf0jr80axvBcVZPREY/a15SbOJliZI
mBzqxto1mX6idWONMLIhWwwwV0SMEkOTxl4Osh/De2FbnSW47rOQxJdXs0jXQXW/
KOlVX7h7PzQ7KqS9lDpHJsyRMseRPMq3BTfwW+pMs3qwKEDg2Ge4p1E8aGv7Bjj8
LoPvPuGV2Vy51ZzowMqHxQzYTXdnHq33TDI8M/1qaJaIdrhSsGbmcoJt2YQuXPkE
d+6/eMbY0YERDJ6FTofe
=eAJu
-END PGP SIGNATURE-



Re: [gentoo-portage-dev] [PATCH 2/2] pym/portage/util/locale.py: add a C module to help check locale

2016-05-23 Thread Michał Górny
On Sun, 22 May 2016 13:04:40 -0400
"Anthony G. Basile"  wrote:

> From: "Anthony G. Basile" 
> 
> The current method to check for a sane system locale is to use python's
> ctypes.util.find_library() to construct a full library path to the
> system libc.so and pass that path to ctypes.CDLL() so we can call
> toupper() and tolower() directly.  However, this gets bogged down in
> implementation details and fails with musl.
> 
> We work around this design flaw in ctypes with a small python module
> written in C which provides thin wrappers to toupper() and tolower(),
> and only fall back on the current ctypes-based check when this module
> is not available.
> 
> This has been tested on glibc, uClibc and musl systems.
> 
> X-Gentoo-bug: 571444
> X-Gentoo-bug-url: https://bugs.gentoo.org/show_bug.cgi?id=571444
> 
> Signed-off-by: Anthony G. Basile 
> ---
>  pym/portage/util/locale.py   | 32 ++-
>  setup.py |  6 ++-
>  src/portage_c_convert_case.c | 94 
> 
>  3 files changed, 121 insertions(+), 11 deletions(-)
>  create mode 100644 src/portage_c_convert_case.c
> 
> diff --git a/pym/portage/util/locale.py b/pym/portage/util/locale.py
> index 2a15ea1..85ddd2b 100644
> --- a/pym/portage/util/locale.py
> +++ b/pym/portage/util/locale.py
> @@ -11,6 +11,7 @@ from __future__ import absolute_import, unicode_literals
>  import locale
>  import logging
>  import os
> +import sys
>  import textwrap
>  import traceback
>  
> @@ -34,18 +35,26 @@ def _check_locale(silent):
>   """
>   The inner locale check function.
>   """
> -
> - libc_fn = find_library("c")
> - if libc_fn is None:
> - return None
> - libc = LoadLibrary(libc_fn)
> - if libc is None:
> - return None
> + try:
> + from portage_c_convert_case import _c_toupper, _c_tolower
> + libc_tolower = _c_tolower
> + libc_toupper = _c_toupper

Now I'm being picky... but if you named the functions toupper()
and tolower(), you could actually import the whole module as 'libc'
and have less code!

Also it would be nice to actually make the module more generic. There
are more places where we use CDLL, and all of them could eventually be
supported by the module (unshare() would be much better done in C, for
example).

> + except ImportError:
> + writemsg_level("!!! Unable to import 
> portage_c_convert_case\n!!!\n",
> + level=logging.WARNING, noiselevel=-1)

Do we really want to warn verbosely about this? I think it'd be
a pretty common case for people running the git checkout.

> + libc_fn = find_library("c")
> + if libc_fn is None:
> + return None
> + libc = LoadLibrary(libc_fn)
> + if libc is None:
> + return None
> + libc_tolower = libc.tolower
> + libc_toupper = libc.toupper
>  
>   lc = list(range(ord('a'), ord('z')+1))
>   uc = list(range(ord('A'), ord('Z')+1))
> - rlc = [libc.tolower(c) for c in uc]
> - ruc = [libc.toupper(c) for c in lc]
> + rlc = [libc_tolower(c) for c in uc]
> + ruc = [libc_toupper(c) for c in lc]
>  
>   if lc != rlc or uc != ruc:
>   if silent:
> @@ -62,7 +71,10 @@ def _check_locale(silent):
>   "as LC_CTYPE in make.conf.")
>   msg = [l for l in textwrap.wrap(msg, 70)]
>   msg.append("")
> - chars = lambda l: ''.join(chr(x) for x in l)
> + if sys.version_info.major >= 3:

Portage uses hexversion for comparisons. Please be consistent.

> + chars = lambda l: ''.join(chr(x) for x in l)
> + else:
> + chars = lambda l: ''.join(chr(x).decode('utf-8', 
> 'replace') for x in l)

This looks like an unrelated change. Was the original code buggy? If
this is the case, then please fix it in a separate commit.

>   if uc != ruc:
>   msg.extend([
>   "  %s -> %s" % (chars(lc), chars(ruc)),
> diff --git a/setup.py b/setup.py
> index 25429bc..8b6b408 100755
> --- a/setup.py
> +++ b/setup.py
> @@ -47,7 +47,11 @@ x_scripts = {
>  # Dictionary custom modules written in C/C++ here.  The structure is
>  #   key   = module name
>  #   value = list of C/C++ source code, path relative to top source directory
> -x_c_helpers = {}
> +x_c_helpers = {
> + 'portage_c_convert_case' : [
> + 'src/portage_c_convert_case.c',
> + ],
> +}
>  
>  class x_build(build):
>   """ Build command with extra build_man call. """
> diff --git a/src/portage_c_convert_case.c b/src/portage_c_convert_case.c
> new file mode 100644
> index 000..f60b0c2
> --- /dev/null
> +++ b/src/portage_c_convert_case.c
> @@ -0,0 +1,94 @@
> +/* Copyright 2005-2016 Gentoo Foundation
> + *