[Mesa-dev] [PATCH shader-db] check_dependencies: refactor to a python script

[Dylan Baker]

On Sat, Oct 10, 2015 at 06:45:08PM +1100, Rhys Kidd wrote:
> Deliver consistency with all other shader-db scripts, which are Python scripts.
> 
> No change in features or output strings.
> 
> Passed pep8, except for two comment lines suggesting commands to add
> dependencies to the [require] section of *.shader_test files.
> 
> Although not a performance critical feature, equivalent performance to
> Perl script other than process_directories() recursive directory traversal.
> 
> os.scandir(item) would be significantly faster than os.walk(item), however
> its use would introduce a minimum dependency on Python 3.5 which is preferably
> avoided at this time.
> 
> Signed-off-by: Rhys Kidd <rhyskidd at gmail.com>
> ---
>  check_dependencies.pl | 107 --------------------------------------------------
>  check_dependencies.py |  82 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 107 deletions(-)
>  delete mode 100755 check_dependencies.pl
>  create mode 100755 check_dependencies.py
> 
> diff --git a/check_dependencies.pl b/check_dependencies.pl
> deleted file mode 100755
> index 3e49f7f..0000000
> --- a/check_dependencies.pl
> +++ /dev/null
> @@ -1,107 +0,0 @@
> -#!/usr/bin/perl
> -#
> -# Copyright © 2014 Intel Corporation
> -#
> -# Permission is hereby granted, free of charge, to any person obtaining a
> -# copy of this software and associated documentation files (the "Software"),
> -# to deal in the Software without restriction, including without limitation
> -# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> -# and/or sell copies of the Software, and to permit persons to whom the
> -# Software is furnished to do so, subject to the following conditions:
> -#
> -# The above copyright notice and this permission notice (including the next
> -# paragraph) shall be included in all copies or substantial portions of the
> -# Software.
> -#
> -# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> -# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> -# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> -# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> -# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> -# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> -# IN THE SOFTWARE.
> -
> -# For checking that shader_test's dependencies are correct.
> -#
> -# Run with
> -# 	./check_dependencies.pl shaders/
> -#
> -# And then run a command like these to add dependencies to the [require]
> -# section:
> -#
> -# find shaders/ -name '*.shader_test' -exec grep -l '#version 120' {} + | xargs sed -i -e 's/GLSL >= 1.10/GLSL >= 1.20/'
> -# find shaders/ -name '*.shader_test' -exec grep -l '#extension GL_ARB_texture_rectangle : require' {} + | xargs sed -i -e 's/GLSL >= 1.20/GLSL >= 1.20\nGL_ARB_texture_rectangle/'
> -
> -use strict;
> -use File::Find;
> -
> -die("Not enough arguments: specify a directory\n") if ($#ARGV < 0);
> -
> -# The array_diff function is copied from the Array::Utils package and contains
> -# this copyright:
> -#
> -# This module is Copyright (c) 2007 Sergei A. Fedorov.
> -# All rights reserved.
> -#
> -# You may distribute under the terms of either the GNU General Public
> -# License or the Artistic License, as specified in the Perl README file.
> -sub array_diff(\@\@) {
> -	my %e = map { $_ => undef } @{$_[1]};
> -	return @{[ ( grep { (exists $e{$_}) ? ( delete $e{$_} ) : ( 1 ) } @{ $_[0] } ), keys %e ] };
> -}
> -
> -my @shader_test;
> -
> -sub wanted {
> -	push(@shader_test, $File::Find::name) if (/\.shader_test$/);
> -}
> -
> -finddepth(\&wanted,  @ARGV);
> -
> -my $fail = 0;
> -
> -foreach my $shader_test (@shader_test) {
> -	my $expected;
> -	my $actual;
> -	my @expected_ext;
> -	my @actual_ext;
> -
> -	open(my $fh, "<", $shader_test)
> -		or die("cannot open < $shader_test: $!\n");
> -	
> -	while (<$fh>) {
> -		chomp;
> -
> -		if (/^GLSL >= (\d)\.(\d\d)/) {
> -			$expected = $1 * 100 + $2;
> -		}
> -		if (/^\s*#\s*version\s+(\d{3})/) {
> -			$actual = $1 if $actual == undef;
> -			$actual = $1 if $actual < $1;
> -		}
> -
> -		if (/^(GL_\S+)/) {
> -			next if ($1 eq "GL_ARB_fragment_program" ||
> -				 $1 eq "GL_ARB_vertex_program");
> -			push(@expected_ext, $1);
> -		}
> -		if (/^\s*#\s*extension\s+(GL_\S+)\s*:\s*require/) {
> -			push(@actual_ext, $1);
> -		}
> -	}
> -
> -	close($fh);
> -
> -	if ($actual != undef && $expected != $actual) {
> -		print "$shader_test requested $expected, but requires $actual\n";
> -		$fail = 1;
> -	}
> -
> -	my @extension = array_diff(@expected_ext, @actual_ext);
> -	foreach my $extension (@extension) {
> -		print "$shader_test extension $extension mismatch\n";
> -		$fail = 1;
> -	}
> -}
> -
> -exit($fail);
> diff --git a/check_dependencies.py b/check_dependencies.py
> new file mode 100755
> index 0000000..c1f7860
> --- /dev/null
> +++ b/check_dependencies.py
> @@ -0,0 +1,82 @@
> +#!/usr/bin/env python3
> +
> +# For checking that shader_test's dependencies are correct.
> +#
> +# Run with
> +# 	./check_dependencies shaders/
> +#
> +# And then run a command like these to add dependencies to the [require]
> +# section:
> +#
> +# find shaders/ -name '*.shader_test' -exec grep -l '#version 120' {} + | xargs sed -i -e 's/GLSL >= 1.10/GLSL >= 1.20/'
> +# find shaders/ -name '*.shader_test' -exec grep -l '#extension GL_ARB_texture_rectangle : require' {} + | xargs sed -i -e 's/GLSL >= 1.20/GLSL >= 1.20\nGL_ARB_texture_rectangle/
> +
> +import os
> +import re
> +import sys
> +
> +
> +def process_directories(item, filetype):
> +    if os.path.isfile(item):
> +        yield item
> +    else:
> +        for dirpath, _, filenames in os.walk(item):
> +            for fname in filenames:
> +                if filetype in fname and fname.endswith(filetype):
> +                    yield os.path.join(dirpath, fname)
> +
> +
> +def main():
> +    fail = 0
> +
> +    if len(sys.argv) < 2:
> +        print("Not enough arguments: specify a directory")
> +        sys.exit(1)
> +
> +    re_find_GLSL = re.compile(r'GLSL >= (\d).(\d\d)')
> +    re_find_version = re.compile(r'\s*#\s*version\s+(\d{3})')
> +
> +    re_find_GL = re.compile(r'(GL_\S+)')
> +    re_find_ext = re.compile(r'\s*#\s*extension\s+(GL_\S+)\s*:\s*require')
> +
> +    for shader_test in process_directories(sys.argv[1], '.shader_test'):
> +        expected = 0
> +        actual = 0
> +        expected_ext = []
> +        actual_ext = []
> +
> +        with open(shader_test, 'r') as fh:
> +            for line in fh:
> +                match = re_find_GLSL.match(line)
> +                if match:
> +                    expected = int(match.group(1) + match.group(2))
> +
> +                match = re_find_version.match(line)
> +                if match:
> +                    actual = max(actual, int(match.group(1)))
> +
> +                match = re_find_GL.match(line)
> +                if match:
> +                    if match.group(1) != "GL_ARB_fragment_program" \
> +                       and match.group(1) != "GL_ARB_vertex_program":
> +                        expected_ext.append(match.group(1))
> +
> +                match = re_find_ext.match(line)
> +                if match:
> +                    actual_ext.append(match.group(1))
> +
> +        if (actual is not 0) and (expected is not actual):

This is very subtly wrong, because you're using 'is'.

'is', as I'm sure you're aware, is true if a and b are a reference to
the same object. Int's are interesting, because of the way CPython
stores them your code will work with small ints (though the assumption
could be broken by a C extension, I think), but can break at odd times
in odd ways with larger ints.

From https://docs.python.org/3.4/c-api/long.html (also applies to 2.7):
The current implementation keeps an array of integer objects for all
integers between -5 and 256, when you create an int in that range you
actually just get back a reference to the existing object. So it should
be possible to change the value of 1.

tl;dr: for ints always use '==' or '!=' not 'is' or 'is not'

> +            print("%s requested %s, but requires %s" %
> +                  (shader_test, expected, actual))
> +            fail = 1
> +
> +        for extension in set(expected_ext) ^ set(actual_ext):
> +            print("%s extension %s mismatch" % (shader_test, extension))
> +            fail = 1
> +
> +    sys.exit(fail)
> +
> +
> +if __name__ == "__main__":
> +    main()
> +
> -- 
> 2.1.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151012/cd752d58/attachment-0001.sig>