[Mesa-dev] glx/tests: Fix bash-specific code in dispatch-index-check

Submitted by Aaron Watry on Feb. 25, 2017, 4:03 a.m.

Details

Message ID 20170225040336.19603-1-awatry@gmail.com
State New
Headers show
Series "glx/tests: Fix bash-specific code in dispatch-index-check" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Aaron Watry Feb. 25, 2017, 4:03 a.m.
Using <<< for variable redirection is bash-specific behavior.
Ubuntu redirects sh -> dash, so this was erroring out.

Also, the initial error that led me to this was that srcdir is null when running make check
so I just copied something similar to what the optimization-test script does.
---
 src/glx/tests/dispatch-index-check | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/glx/tests/dispatch-index-check b/src/glx/tests/dispatch-index-check
index 78464b8..ee1b9ee 100755
--- a/src/glx/tests/dispatch-index-check
+++ b/src/glx/tests/dispatch-index-check
@@ -1,24 +1,31 @@ 
 #!/bin/sh
 
+if [ -z "$srcdir" ]; then
+   scriptdir=`dirname "$0"`
+else
+   scriptdir=$srcdir
+fi
+
+
 # extract enum definition
 dispatch_list=$(sed '/__GLXdispatchIndex/,/__GLXdispatchIndex/!d' \
-  "$srcdir"/../g_glxglvnddispatchindices.h)
+  "$scriptdir"/../g_glxglvnddispatchindices.h)
 
 # extract values inside of enum
-dispatch_list=$(sed '1d;$d' <<< "$dispatch_list")
+dispatch_list=$(printf "$dispatch_list" | sed '1d;$d')
 
 # remove indentation
-dispatch_list=$(sed 's/^\s\+//' <<< "$dispatch_list")
+dispatch_list=$(printf "$dispatch_list" | sed 's/^\s\+//')
 
 # extract function names
-dispatch_list=$(sed 's/DI_//;s/,//' <<< "$dispatch_list")
+dispatch_list=$(printf "$dispatch_list" | sed 's/DI_//;s/,//')
 
 # same for commented functions, we want to keep them sorted too
-dispatch_list=$(sed 's#// ##;s/ implemented by [a-z]\+//' <<< "$dispatch_list")
+dispatch_list=$(printf "$dispatch_list" | sed 's#// ##;s/ implemented by [a-z]\+//')
 
 # remove LAST_INDEX, as it will not be in alphabetical order
-dispatch_list=$(sed '/LAST_INDEX/d' <<< "$dispatch_list")
+dispatch_list=$(printf "$dispatch_list" | sed '/LAST_INDEX/d')
 
-sorted=$(LC_ALL=C sort <<< "$dispatch_list")
+sorted=$(LC_ALL=C printf "$dispatch_list" | sort)
 
 test "$dispatch_list" = "$sorted"

Comments

On Fri, Feb 24, 2017 at 8:03 PM, Aaron Watry <awatry@gmail.com> wrote:
> Using <<< for variable redirection is bash-specific behavior.
> Ubuntu redirects sh -> dash, so this was erroring out.
>
> Also, the initial error that led me to this was that srcdir is null when running make check
> so I just copied something similar to what the optimization-test script does.
> ---
>  src/glx/tests/dispatch-index-check | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/src/glx/tests/dispatch-index-check b/src/glx/tests/dispatch-index-check
> index 78464b8..ee1b9ee 100755
> --- a/src/glx/tests/dispatch-index-check
> +++ b/src/glx/tests/dispatch-index-check
> @@ -1,24 +1,31 @@
>  #!/bin/sh
>
> +if [ -z "$srcdir" ]; then
> +   scriptdir=`dirname "$0"`
> +else
> +   scriptdir=$srcdir
> +fi
> +
> +
>  # extract enum definition
>  dispatch_list=$(sed '/__GLXdispatchIndex/,/__GLXdispatchIndex/!d' \
> -  "$srcdir"/../g_glxglvnddispatchindices.h)
> +  "$scriptdir"/../g_glxglvnddispatchindices.h)
>
>  # extract values inside of enum
> -dispatch_list=$(sed '1d;$d' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed '1d;$d')
>
>  # remove indentation
> -dispatch_list=$(sed 's/^\s\+//' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed 's/^\s\+//')
>
>  # extract function names
> -dispatch_list=$(sed 's/DI_//;s/,//' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed 's/DI_//;s/,//')
>
>  # same for commented functions, we want to keep them sorted too
> -dispatch_list=$(sed 's#// ##;s/ implemented by [a-z]\+//' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed 's#// ##;s/ implemented by [a-z]\+//')
>
>  # remove LAST_INDEX, as it will not be in alphabetical order
> -dispatch_list=$(sed '/LAST_INDEX/d' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed '/LAST_INDEX/d')
>
> -sorted=$(LC_ALL=C sort <<< "$dispatch_list")
> +sorted=$(LC_ALL=C printf "$dispatch_list" | sort)
>
>  test "$dispatch_list" = "$sorted"
> --
> 2.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Fixes: 3cc33e764011 ("glx: add GLXdispatchIndex sort check")

Tested-by: Vinson Lee <vlee@freedesktop.org>
On Friday, 2017-02-24 22:03:36 -0600, Aaron Watry wrote:
> Using <<< for variable redirection is bash-specific behavior.
> Ubuntu redirects sh -> dash, so this was erroring out.
> 
> Also, the initial error that led me to this was that srcdir is null when running make check
> so I just copied something similar to what the optimization-test script does.
> ---
>  src/glx/tests/dispatch-index-check | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/src/glx/tests/dispatch-index-check b/src/glx/tests/dispatch-index-check
> index 78464b8..ee1b9ee 100755
> --- a/src/glx/tests/dispatch-index-check
> +++ b/src/glx/tests/dispatch-index-check
> @@ -1,24 +1,31 @@
>  #!/bin/sh
>  
> +if [ -z "$srcdir" ]; then
> +   scriptdir=`dirname "$0"`
> +else
> +   scriptdir=$srcdir
> +fi
> +
> +
>  # extract enum definition
>  dispatch_list=$(sed '/__GLXdispatchIndex/,/__GLXdispatchIndex/!d' \
> -  "$srcdir"/../g_glxglvnddispatchindices.h)
> +  "$scriptdir"/../g_glxglvnddispatchindices.h)

No need to create a new var that just copies the old one :)

>  
>  # extract values inside of enum
> -dispatch_list=$(sed '1d;$d' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed '1d;$d')

Never use a variable you have no control over as the format string for
printf! Use `printf '%s' "$var"` instead.

I just pushed a1e5e55989 ("check: mark two tests are requiring bash")
which fixes this by simply asking for bash in the shebang, which was
what my original patch did, and was changed just before pushing because
of a review comment that turned out to be wrong :)

Cheers,
  Eric

>  
>  # remove indentation
> -dispatch_list=$(sed 's/^\s\+//' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed 's/^\s\+//')
>  
>  # extract function names
> -dispatch_list=$(sed 's/DI_//;s/,//' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed 's/DI_//;s/,//')
>  
>  # same for commented functions, we want to keep them sorted too
> -dispatch_list=$(sed 's#// ##;s/ implemented by [a-z]\+//' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed 's#// ##;s/ implemented by [a-z]\+//')
>  
>  # remove LAST_INDEX, as it will not be in alphabetical order
> -dispatch_list=$(sed '/LAST_INDEX/d' <<< "$dispatch_list")
> +dispatch_list=$(printf "$dispatch_list" | sed '/LAST_INDEX/d')
>  
> -sorted=$(LC_ALL=C sort <<< "$dispatch_list")
> +sorted=$(LC_ALL=C printf "$dispatch_list" | sort)
>  
>  test "$dispatch_list" = "$sorted"
> -- 
> 2.9.3
>
On 26 February 2017 at 13:51, Eric Engestrom <eric@engestrom.ch> wrote:
> On Friday, 2017-02-24 22:03:36 -0600, Aaron Watry wrote:
>> Using <<< for variable redirection is bash-specific behavior.
>> Ubuntu redirects sh -> dash, so this was erroring out.
>>
>> Also, the initial error that led me to this was that srcdir is null when running make check
>> so I just copied something similar to what the optimization-test script does.
>> ---
>>  src/glx/tests/dispatch-index-check | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/glx/tests/dispatch-index-check b/src/glx/tests/dispatch-index-check
>> index 78464b8..ee1b9ee 100755
>> --- a/src/glx/tests/dispatch-index-check
>> +++ b/src/glx/tests/dispatch-index-check
>> @@ -1,24 +1,31 @@
>>  #!/bin/sh
>>
>> +if [ -z "$srcdir" ]; then
>> +   scriptdir=`dirname "$0"`
>> +else
>> +   scriptdir=$srcdir
>> +fi
>> +
>> +
>>  # extract enum definition
>>  dispatch_list=$(sed '/__GLXdispatchIndex/,/__GLXdispatchIndex/!d' \
>> -  "$srcdir"/../g_glxglvnddispatchindices.h)
>> +  "$scriptdir"/../g_glxglvnddispatchindices.h)
>
> No need to create a new var that just copies the old one :)
>
>>
>>  # extract values inside of enum
>> -dispatch_list=$(sed '1d;$d' <<< "$dispatch_list")
>> +dispatch_list=$(printf "$dispatch_list" | sed '1d;$d')
>
> Never use a variable you have no control over as the format string for
> printf! Use `printf '%s' "$var"` instead.
>
> I just pushed a1e5e55989 ("check: mark two tests are requiring bash")
> which fixes this by simply asking for bash in the shebang, which was
> what my original patch did, and was changed just before pushing because
> of a review comment that turned out to be wrong :)
>
Yes, my bad on that one - guess I shouldn't trust checkbashisms/zsh that much.

I won't object if we can make these tests 'generic' sh - patches welcome ;-)
Emil
On Sun, Feb 26, 2017 at 7:51 AM, Eric Engestrom <eric@engestrom.ch> wrote:

> On Friday, 2017-02-24 22:03:36 -0600, Aaron Watry wrote:
> > Using <<< for variable redirection is bash-specific behavior.
> > Ubuntu redirects sh -> dash, so this was erroring out.
> >
> > Also, the initial error that led me to this was that srcdir is null when
> running make check
> > so I just copied something similar to what the optimization-test script
> does.
> > ---
> >  src/glx/tests/dispatch-index-check | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/glx/tests/dispatch-index-check
> b/src/glx/tests/dispatch-index-check
> > index 78464b8..ee1b9ee 100755
> > --- a/src/glx/tests/dispatch-index-check
> > +++ b/src/glx/tests/dispatch-index-check
> > @@ -1,24 +1,31 @@
> >  #!/bin/sh
> >
> > +if [ -z "$srcdir" ]; then
> > +   scriptdir=`dirname "$0"`
> > +else
> > +   scriptdir=$srcdir
> > +fi
> > +
> > +
> >  # extract enum definition
> >  dispatch_list=$(sed '/__GLXdispatchIndex/,/__GLXdispatchIndex/!d' \
> > -  "$srcdir"/../g_glxglvnddispatchindices.h)
> > +  "$scriptdir"/../g_glxglvnddispatchindices.h)
>
> No need to create a new var that just copies the old one :)
>
>
Fair enough.  I just didn't want this script to screw up anything in the
future that sourced this script in by modifying externally declared
variables that we knew about.


> >
> >  # extract values inside of enum
> > -dispatch_list=$(sed '1d;$d' <<< "$dispatch_list")
> > +dispatch_list=$(printf "$dispatch_list" | sed '1d;$d')
>
> Never use a variable you have no control over as the format string for
> printf! Use `printf '%s' "$var"` instead.
>

Yeah, that's my bad. I basically did a SO search for ways to de-bashify the
'<<<' operator, and one of the first results used printf $var.  In
retrospect, your version is much more appropriate.

I'm generally a bash guy, so just declaring the scripts as actually being
bash scripts works for me as well.  Make check works again for me, and I
agree with Vinson that it'd be nice to translate this to pure sh, but for
now, this at least works.


>
> I just pushed a1e5e55989 ("check: mark two tests are requiring bash")
> which fixes this by simply asking for bash in the shebang, which was
> what my original patch did, and was changed just before pushing because
> of a review comment that turned out to be wrong :)
>
> Cheers,
>   Eric
>
> >
> >  # remove indentation
> > -dispatch_list=$(sed 's/^\s\+//' <<< "$dispatch_list")
> > +dispatch_list=$(printf "$dispatch_list" | sed 's/^\s\+//')
> >
> >  # extract function names
> > -dispatch_list=$(sed 's/DI_//;s/,//' <<< "$dispatch_list")
> > +dispatch_list=$(printf "$dispatch_list" | sed 's/DI_//;s/,//')
> >
> >  # same for commented functions, we want to keep them sorted too
> > -dispatch_list=$(sed 's#// ##;s/ implemented by [a-z]\+//' <<<
> "$dispatch_list")
> > +dispatch_list=$(printf "$dispatch_list" | sed 's#// ##;s/ implemented
> by [a-z]\+//')
> >
> >  # remove LAST_INDEX, as it will not be in alphabetical order
> > -dispatch_list=$(sed '/LAST_INDEX/d' <<< "$dispatch_list")
> > +dispatch_list=$(printf "$dispatch_list" | sed '/LAST_INDEX/d')
> >
> > -sorted=$(LC_ALL=C sort <<< "$dispatch_list")
> > +sorted=$(LC_ALL=C printf "$dispatch_list" | sort)
> >
> >  test "$dispatch_list" = "$sorted"
> > --
> > 2.9.3
> >
>