[Mesa-dev] [PATCH] android: vulkan: add support for libmesa_vulkan_{util, wsi}

[Dylan Baker]

Quoting Mauro Rossi (2017-02-25 10:31:43)
[snip]
> diff --git a/src/vulkan/util/gen_enum_to_str.py b/src/vulkan/util/gen_enum_to_str.py
> index 4b6fdf3..5d2bc54 100644
> --- a/src/vulkan/util/gen_enum_to_str.py
> +++ b/src/vulkan/util/gen_enum_to_str.py
> @@ -22,6 +22,7 @@
>  """Create enum to string functions for vulking using vk.xml."""
>  
>  from __future__ import print_function
> +import argparse
>  import os
>  import textwrap
>  import xml.etree.cElementTree as et
> @@ -30,6 +31,14 @@ from mako.template import Template
>  
>  VK_XML = os.path.join(os.path.dirname(__file__), '..', 'registry', 'vk.xml')
>  
> +parser = argparse.ArgumentParser(formatter_class=argparse.RawTextHelpFormatter)
> +parser.add_argument('-o', '--output-path', help=textwrap.dedent('''\
> +   -o $(top_builddir)/src/vulkan/util for Linux builds
> +   -o $(intermediates)/util for Android builds'''),
> +   required=True)

Don't put makefile commands as the help line, either set it to something like
"where to put the output files", or just omit it.

> +argv = parser.parse_args()
> +dst_dir = argv.output_path

Can we call this "args" instead of "argv"? There is an argv in python
(sys.argv), and it's just asking to confuse people who expect C's ARGV.

> +
>  COPYRIGHT = textwrap.dedent(u"""\
>      * Copyright © 2017 Intel Corporation
>      *
> @@ -159,8 +168,8 @@ def xml_parser(filename):
>  
>  def main():
>      enums = xml_parser(VK_XML)
> -    for template, file_ in [(C_TEMPLATE, 'util/vk_enum_to_str.c'),
> -                            (H_TEMPLATE, 'util/vk_enum_to_str.h')]:
> +    for template, file_ in [(C_TEMPLATE, os.path.join(dst_dir,'vk_enum_to_str.c')),
> +                            (H_TEMPLATE, os.path.join(dst_dir,'vk_enum_to_str.h'))]:

I don't think there's anything wrong with just using 'os.path.join(args.output_path, ...)

Also, please put a space after the ',' in os.path.join

I'm not really qualified to review the android.mk, but with the above changes:
Reviewed-by: Dylan Baker <dylan at pnwbakers.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170227/2f06c9f6/attachment.sig>