[gentoo-portage-dev] [PATCH] xpak-helper: rewrite to rely more on argparse

[gentoo-portage-dev] [PATCH] xpak-helper: rewrite to rely more on argparse

[Mike Frysinger]

The current code implements a lot of ad-hoc argument parsing when it
could simply let the argparse module do it all for it.  This makes the
code easier to understand and extend in the process.
---
 bin/xpak-helper.py | 68 ++++++++++++++++++++----------------------------------
 1 file changed, 25 insertions(+), 43 deletions(-)

diff --git a/bin/xpak-helper.py b/bin/xpak-helper.py
index 8c965ec..1b2883d 100755
--- a/bin/xpak-helper.py
+++ b/bin/xpak-helper.py
@@ -2,68 +2,50 @@
 # Copyright 2009-2014 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
+"""Helper to manage xpak archives"""
+
 import argparse
 import sys
 import portage
 portage._internal_caller = True
 from portage import os
 
-def command_recompose(args):
-
-	usage = "usage: recompose <binpkg_path> <metadata_dir>\n"
 
-	if len(args) != 2:
-		sys.stderr.write(usage)
-		sys.stderr.write("2 arguments are required, got %s\n" % len(args))
-		return 1
+def command_recompose(parser, opts):
+	"""Create an xpak archive from a directory"""
+	binpkg_path = opts.binpkg_path
+	metadata_dir = opts.metadata_dir
 
-	binpkg_path, metadata_dir = args
-
-	if not os.path.isfile(binpkg_path):
-		sys.stderr.write(usage)
-		sys.stderr.write("Argument 1 is not a regular file: '%s'\n" % \
-			binpkg_path)
-		return 1
+	if os.path.exists(binpkg_path) and not os.path.isfile(binpkg_path):
+		parser.error('binpkg_path is not a regular file: %s' % binpkg_path)
 
 	if not os.path.isdir(metadata_dir):
-		sys.stderr.write(usage)
-		sys.stderr.write("Argument 2 is not a directory: '%s'\n" % \
-			metadata_dir)
-		return 1
+		parser.error('metadata_dir is not a directory: %s' % metadata_dir)
 
 	t = portage.xpak.tbz2(binpkg_path)
 	t.recompose(metadata_dir)
 	return os.EX_OK
 
-def main(argv):
 
-	if argv and isinstance(argv[0], bytes):
-		for i, x in enumerate(argv):
-			argv[i] = portage._unicode_decode(x, errors='strict')
+def get_parser():
+	"""Return the command line parser"""
+	parser = argparse.ArgumentParser(description=__doc__)
+	subparsers = parser.add_subparsers()
 
-	valid_commands = ('recompose',)
-	description = "Perform metadata operations on a binary package."
-	usage = "usage: %s COMMAND [args]" % \
-		os.path.basename(argv[0])
+	subparser = subparsers.add_parser('recompose',
+		help=command_recompose.__doc__)
+	subparser.set_defaults(func=command_recompose)
+	subparser.add_argument('binpkg_path', help='Path to output xpak')
+	subparser.add_argument('metadata_dir', help='Path to input dir')
 
-	parser = argparse.ArgumentParser(description=description, usage=usage)
-	options, args = parser.parse_known_args(argv[1:])
+	return parser
 
-	if not args:
-		parser.error("missing command argument")
 
-	command = args[0]
-
-	if command not in valid_commands:
-		parser.error("invalid command: '%s'" % command)
-
-	if command == 'recompose':
-		rval = command_recompose(args[1:])
-	else:
-		raise AssertionError("invalid command: '%s'" % command)
+def main(argv):
+	parser = get_parser()
+	opts = parser.parse_args(argv)
+	opts.func(parser, opts)
 
-	return rval
 
-if __name__ == "__main__":
-	rval = main(sys.argv[:])
-	sys.exit(rval)
+if __name__ == '__main__':
+	sys.exit(main(sys.argv[1:]))
-- 
2.5.2

Re: [gentoo-portage-dev] [PATCH] xpak-helper: rewrite to rely more on argparse

[Zac Medico]

On 10/31/2015 10:23 PM, Mike Frysinger wrote:
> The current code implements a lot of ad-hoc argument parsing when it
> could simply let the argparse module do it all for it.  This makes the
> code easier to understand and extend in the process.
> ---
>  bin/xpak-helper.py | 68 ++++++++++++++++++++----------------------------------
>  1 file changed, 25 insertions(+), 43 deletions(-)
> 
> diff --git a/bin/xpak-helper.py b/bin/xpak-helper.py
> index 8c965ec..1b2883d 100755
> --- a/bin/xpak-helper.py
> +++ b/bin/xpak-helper.py
[snip]
> -def main(argv):
>  
> -	if argv and isinstance(argv[0], bytes):
> -		for i, x in enumerate(argv):
> -			argv[i] = portage._unicode_decode(x, errors='strict')

You've dropped the _unicode_decode call.

In order to handle python3 with arguments containing UTF-8 characters
(in ${PKGDIR}) and a mis-matched sys.getfilesystemencoding() value, it's
safest to decode the arguments like chmod-lite.py does. We should create
a function for this code which is also duplicated in install.py:

if sys.hexversion >= 0x3000000:
    # We can't trust that the filesystem encoding (locale dependent)
    # correctly matches the arguments, so use surrogateescape to
    # pass through the original argv bytes for Python 3.
    fs_encoding = sys.getfilesystemencoding()
    files = [x.encode(fs_encoding, 'surrogateescape') for x in files]
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] xpak-helper: rewrite to rely more on argparse

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On 01 Nov 2015 09:36, Zac Medico wrote:
> On 10/31/2015 10:23 PM, Mike Frysinger wrote:
> > The current code implements a lot of ad-hoc argument parsing when it
> > could simply let the argparse module do it all for it.  This makes the
> > code easier to understand and extend in the process.
> > ---
> >  bin/xpak-helper.py | 68 ++++++++++++++++++++----------------------------------
> >  1 file changed, 25 insertions(+), 43 deletions(-)
> > 
> > diff --git a/bin/xpak-helper.py b/bin/xpak-helper.py
> > index 8c965ec..1b2883d 100755
> > --- a/bin/xpak-helper.py
> > +++ b/bin/xpak-helper.py
> [snip]
> > -def main(argv):
> >  
> > -	if argv and isinstance(argv[0], bytes):
> > -		for i, x in enumerate(argv):
> > -			argv[i] = portage._unicode_decode(x, errors='strict')
> 
> You've dropped the _unicode_decode call.

i did on purpose.  i should have mentioned that.

> In order to handle python3 with arguments containing UTF-8 characters
> (in ${PKGDIR}) and a mis-matched sys.getfilesystemencoding() value, it's
> safest to decode the arguments like chmod-lite.py does.

it seems wrong that we have incomplete coverage here.
some tools do it and some do not.

> We should create
> a function for this code which is also duplicated in install.py:

you mean portage._decode_argv ?

what if we create a new module like "commandline" that provides an
ArgumentParser interface that takes care of this for us ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] xpak-helper: rewrite to rely more on argparse

[Zac Medico]

On 11/02/2015 08:52 AM, Mike Frysinger wrote:
> On 01 Nov 2015 09:36, Zac Medico wrote:
>> In order to handle python3 with arguments containing UTF-8 characters
>> (in ${PKGDIR}) and a mis-matched sys.getfilesystemencoding() value, it's
>> safest to decode the arguments like chmod-lite.py does.
> 
> it seems wrong that we have incomplete coverage here.
> some tools do it and some do not.

Yeah, complete coverage would be nice. Most if not all of the python
helpers that are called from the ebuild environment already use this
method to decode filename arguments (dohtml.py was only fixed recently,
for bug 561846).

>> We should create
>> a function for this code which is also duplicated in install.py:
> 
> you mean portage._decode_argv ?

Yes, I forgot about that function.

> what if we create a new module like "commandline" that provides an
> ArgumentParser interface that takes care of this for us ?
> -mike

That sounds good. After it decodes the arguments, it should decode them
as UTF-8 with errors='strict', and exit the program immediately if it
triggers a UnicodeDecodeError.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] xpak-helper: rewrite to rely more on argparse

[Zac Medico]

On 11/02/2015 09:06 AM, Zac Medico wrote:
> On 11/02/2015 08:52 AM, Mike Frysinger wrote:
>> On 01 Nov 2015 09:36, Zac Medico wrote:
>>> In order to handle python3 with arguments containing UTF-8 characters
>>> (in ${PKGDIR}) and a mis-matched sys.getfilesystemencoding() value, it's
>>> safest to decode the arguments like chmod-lite.py does.
>>
>> it seems wrong that we have incomplete coverage here.
>> some tools do it and some do not.
> 
> Yeah, complete coverage would be nice. Most if not all of the python
> helpers that are called from the ebuild environment already use this
> method to decode filename arguments (dohtml.py was only fixed recently,
> for bug 561846).
> 
>>> We should create
>>> a function for this code which is also duplicated in install.py:
>>
>> you mean portage._decode_argv ?
> 
> Yes, I forgot about that function.
> 
>> what if we create a new module like "commandline" that provides an
>> ArgumentParser interface that takes care of this for us ?
>> -mike
> 
> That sounds good. After it decodes the arguments, it should decode them
> as UTF-8 with errors='strict', and exit the program immediately if it
> triggers a UnicodeDecodeError.
> 

I mean, after it _encodes_ them with surrogateescape, it should
immediately decode them as UTF-8 with errors='strict'. That way, we'll
have unicode strings to pass to argparse (we don't want to pass raw
bytes to argparse).
-- 
Thanks,
Zac