Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Emil Velikov]

On 19 March 2018 at 13:39, Daniel Stone  wrote:
> Hi Emil,
>
> On 19 March 2018 at 13:27, Emil Velikov  wrote:
>> On 19 March 2018 at 13:20, Daniel Stone  wrote:
>>> Me neither really, but it seemed best for consistency with the rest of
>>> the file which used test rather than [.
>>
>> Sounds fine either way - but the "test ||" -> "if test" changes seems 
>> spurious.
>> If they stand out so much, guess one could have pointed it out?
>
> Not so much spurious as just broken? The final line, as written, will
> only exit if _both_ added and removed are non-empty. If one but not
> the other is non-empty, then the test will exit success[0].
>
Right s/||/&&/ should address that

> I tried to think of a rewrite which would work, but in the end decided
> making it explicit was the least error-prone thing to do, since this
> one managed to slip past review with no-one noticing.
>
Fair point.

Thanks!
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Simon McVittie]

On Mon, 19 Mar 2018 at 13:39:30 +, Daniel Stone wrote:
> On 19 March 2018 at 13:27, Emil Velikov  wrote:
> >> I'd not realised AC_PROG_NM didn't set a full path. Brilliant. Maybe
> >> replace test -x "$NM" with $NM -V >/dev/null 2>&1? Or just trusting it
> >> works if it's non-empty is fine too.
> >>
> > I'd go with non-empty - everything else seems like an overkill.

If you ever want the more full version for something else, I think

command -v "$command" >/dev/null

is the canonical way to ask a POSIX shell "could I invoke $command?".
It exits successfully if $command is a function, an alias, a shell
built-in, the basename of an executable in $PATH, an executable found
by relative or absolute path, an alias or a shell reserved word.

smcv
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Daniel Stone]

Hi Emil,

On 19 March 2018 at 13:27, Emil Velikov  wrote:
> On 19 March 2018 at 13:20, Daniel Stone  wrote:
>> Me neither really, but it seemed best for consistency with the rest of
>> the file which used test rather than [.
>
> Sounds fine either way - but the "test ||" -> "if test" changes seems 
> spurious.
> If they stand out so much, guess one could have pointed it out?

Not so much spurious as just broken? The final line, as written, will
only exit if _both_ added and removed are non-empty. If one but not
the other is non-empty, then the test will exit success[0].

I tried to think of a rewrite which would work, but in the end decided
making it explicit was the least error-prone thing to do, since this
one managed to slip past review with no-one noticing.

 +if ! test -x "$NM"; then
 +   echo "nm binary \"$NM\" does not exist"
 +   exit 99
   fi
>>>
>>> Here, however, you are introducing an -x test, which is not good for all the
>>> people (including packagers) that set NM to e.g. -nm (so not the
>>> full path).
>>
>> I'd not realised AC_PROG_NM didn't set a full path. Brilliant. Maybe
>> replace test -x "$NM" with $NM -V >/dev/null 2>&1? Or just trusting it
>> works if it's non-empty is fine too.
>>
> I'd go with non-empty - everything else seems like an overkill.

Sure, I'll respin.

Cheers,
Daniel

[0]:
strictly:~% cat foo.sh
#!/bin/sh -e

ADDED_YES="added"
ADDED_NO=""
REMOVED_YES="removed"
REMOVED_NO=""

test ! -n "$ADDED_NO" || test ! -n "$REMOVED_YES"
echo "no-yes passed"

test ! -n "$ADDED_YES" || test ! -n "$REMOVED_NO"
echo "yes-no passed"

test ! -n "$ADDED_NO" || test ! -n "$REMOVED_NO"
echo "both-no passed"

test ! -n "$ADDED_YES" || test ! -n "$REMOVED_YES"
echo "both-yes passed"
strictly:~% ./foo.sh
no-yes passed
yes-no passed
both-no passed
zsh: exit 1 ./foo.sh
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Emil Velikov]

On 19 March 2018 at 13:20, Daniel Stone  wrote:
> Hi Quentin,
> Thanks for the quick review!
>
> On 19 March 2018 at 13:06, Quentin Glidic
>  wrote:
>> On 3/19/18 1:31 PM, Daniel Stone wrote:
>>> diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
>>> index c47026b2..d1a4a6be 100755
>>> --- a/egl/wayland-egl-symbols-check
>>> +++ b/egl/wayland-egl-symbols-check
>>> @@ -1,11 +1,17 @@
>>>   #!/bin/sh
>>>   set -eu
>>>   +RET=0
>>>   LIB=${WAYLAND_EGL_LIB}
>>>   -if [ ! -f "$LIB" ]; then
>>> -   echo "The test binary \"$LIB\" does no exist"
>>> -   exit 1
>>> +if ! test -f "$LIB"; then
>>> +   echo "Test binary \"$LIB\" does not exist"
>>> +   exit 99
>>> +fi
>>
>> Not a big fan of replacing [!-f] with !test-f but it’s a cosmetic issue so
>> not a big deal.
>
> Me neither really, but it seemed best for consistency with the rest of
> the file which used test rather than [.
>
Sounds fine either way - but the "test ||" -> "if test" changes seems spurious.
If they stand out so much, guess one could have pointed it out?

>>> +if ! test -x "$NM"; then
>>> +   echo "nm binary \"$NM\" does not exist"
>>> +   exit 99
>>>   fi
>>
>> Here, however, you are introducing an -x test, which is not good for all the
>> people (including packagers) that set NM to e.g. -nm (so not the
>> full path).
>
> I'd not realised AC_PROG_NM didn't set a full path. Brilliant. Maybe
> replace test -x "$NM" with $NM -V >/dev/null 2>&1? Or just trusting it
> works if it's non-empty is fine too.
>
I'd go with non-empty - everything else seems like an overkill.

HTH
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Daniel Stone]

Hi Quentin,
Thanks for the quick review!

On 19 March 2018 at 13:06, Quentin Glidic
 wrote:
> On 3/19/18 1:31 PM, Daniel Stone wrote:
>> diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
>> index c47026b2..d1a4a6be 100755
>> --- a/egl/wayland-egl-symbols-check
>> +++ b/egl/wayland-egl-symbols-check
>> @@ -1,11 +1,17 @@
>>   #!/bin/sh
>>   set -eu
>>   +RET=0
>>   LIB=${WAYLAND_EGL_LIB}
>>   -if [ ! -f "$LIB" ]; then
>> -   echo "The test binary \"$LIB\" does no exist"
>> -   exit 1
>> +if ! test -f "$LIB"; then
>> +   echo "Test binary \"$LIB\" does not exist"
>> +   exit 99
>> +fi
>
> Not a big fan of replacing [!-f] with !test-f but it’s a cosmetic issue so
> not a big deal.

Me neither really, but it seemed best for consistency with the rest of
the file which used test rather than [.

>> +if ! test -x "$NM"; then
>> +   echo "nm binary \"$NM\" does not exist"
>> +   exit 99
>>   fi
>
> Here, however, you are introducing an -x test, which is not good for all the
> people (including packagers) that set NM to e.g. -nm (so not the
> full path).

I'd not realised AC_PROG_NM didn't set a full path. Brilliant. Maybe
replace test -x "$NM" with $NM -V >/dev/null 2>&1? Or just trusting it
works if it's non-empty is fine too.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Quentin Glidic]

On 3/19/18 1:31 PM, Daniel Stone wrote:

The previous rewrite of the wayland-egl ABI checker introduced checks
for removed symbols as well as added symbols, but broke some failure
conditions. Add an explict return-code variable set in failure paths,
rather than chaining or conditions.

If we cannot find the binary or nm, we regard this as an error
condition, rather than test failure.

Signed-off-by: Daniel Stone 
Reported-by: Pekka Paalanen 
Fixes: 21b1f22eb056 ("wayland-egl: enhance the symbol test")
Cc: Emil Velikov 
---
  egl/wayland-egl-symbols-check | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
index c47026b2..d1a4a6be 100755
--- a/egl/wayland-egl-symbols-check
+++ b/egl/wayland-egl-symbols-check
@@ -1,11 +1,17 @@
  #!/bin/sh
  set -eu
  
+RET=0

  LIB=${WAYLAND_EGL_LIB}
  
-if [ ! -f "$LIB" ]; then

-   echo "The test binary \"$LIB\" does no exist"
-   exit 1
+if ! test -f "$LIB"; then
+   echo "Test binary \"$LIB\" does not exist"
+   exit 99
+fi


Not a big fan of replacing [!-f] with !test-f but it’s a cosmetic issue 
so not a big deal.




+
+if ! test -x "$NM"; then
+   echo "nm binary \"$NM\" does not exist"
+   exit 99
  fi


Here, however, you are introducing an -x test, which is not good for all 
the people (including packagers) that set NM to e.g. -nm (so not 
the full path).




  AVAIL_FUNCS="$($NM -D --format=bsd --defined-only $LIB | awk '{print $3}')"


Maybe just checking for -n "$NM" then -n "$AVAIL_FUNCS" would be enough?


Cheers,



@@ -32,7 +38,11 @@ NEW_ABI=$(echo "$AVAIL_FUNCS" | while read func; do
  echo $func
  done)
  
-test ! -n "$NEW_ABI" || echo "New ABI detected - If intentional, update the test."; echo "$NEW_ABI"

+if test -n "$NEW_ABI"; then
+   echo "New ABI detected - If intentional, update the test."
+   echo "$NEW_ABI"
+   RET=1
+fi
  
  REMOVED_ABI=$(echo "$REQ_FUNCS" | while read func; do

  echo "$AVAIL_FUNCS" | grep -q "^$func$" && continue
@@ -40,5 +50,10 @@ REMOVED_ABI=$(echo "$REQ_FUNCS" | while read func; do
  echo $func
  done)
  
-test ! -n "$REMOVED_ABI" || echo "ABI break detected - Required symbol(s) no longer exported!"; echo "$REMOVED_ABI"

-test ! -n "$NEW_ABI" || test ! -n "$REMOVED_ABI"
+if test -n "$REMOVED_ABI"; then
+   echo "ABI break detected - Required symbol(s) no longer exported!"
+   echo "$REMOVED_ABI"
+   RET=1
+fi
+
+exit $RET




--

Quentin “Sardem FF7” Glidic
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure

[Daniel Stone]

The previous rewrite of the wayland-egl ABI checker introduced checks
for removed symbols as well as added symbols, but broke some failure
conditions. Add an explict return-code variable set in failure paths,
rather than chaining or conditions.

If we cannot find the binary or nm, we regard this as an error
condition, rather than test failure.

Signed-off-by: Daniel Stone 
Reported-by: Pekka Paalanen 
Fixes: 21b1f22eb056 ("wayland-egl: enhance the symbol test")
Cc: Emil Velikov 
---
 egl/wayland-egl-symbols-check | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
index c47026b2..d1a4a6be 100755
--- a/egl/wayland-egl-symbols-check
+++ b/egl/wayland-egl-symbols-check
@@ -1,11 +1,17 @@
 #!/bin/sh
 set -eu
 
+RET=0
 LIB=${WAYLAND_EGL_LIB}
 
-if [ ! -f "$LIB" ]; then
-   echo "The test binary \"$LIB\" does no exist"
-   exit 1
+if ! test -f "$LIB"; then
+   echo "Test binary \"$LIB\" does not exist"
+   exit 99
+fi
+
+if ! test -x "$NM"; then
+   echo "nm binary \"$NM\" does not exist"
+   exit 99
 fi
 
 AVAIL_FUNCS="$($NM -D --format=bsd --defined-only $LIB | awk '{print $3}')"
@@ -32,7 +38,11 @@ NEW_ABI=$(echo "$AVAIL_FUNCS" | while read func; do
 echo $func
 done)
 
-test ! -n "$NEW_ABI" || echo "New ABI detected - If intentional, update the 
test."; echo "$NEW_ABI"
+if test -n "$NEW_ABI"; then
+   echo "New ABI detected - If intentional, update the test."
+   echo "$NEW_ABI"
+   RET=1
+fi
 
 REMOVED_ABI=$(echo "$REQ_FUNCS" | while read func; do
 echo "$AVAIL_FUNCS" | grep -q "^$func$" && continue
@@ -40,5 +50,10 @@ REMOVED_ABI=$(echo "$REQ_FUNCS" | while read func; do
 echo $func
 done)
 
-test ! -n "$REMOVED_ABI" || echo "ABI break detected - Required symbol(s) no 
longer exported!"; echo "$REMOVED_ABI"
-test ! -n "$NEW_ABI" || test ! -n "$REMOVED_ABI"
+if test -n "$REMOVED_ABI"; then
+   echo "ABI break detected - Required symbol(s) no longer exported!"
+   echo "$REMOVED_ABI"
+   RET=1
+fi
+
+exit $RET
-- 
2.16.2

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel