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

[Pekka Paalanen]

On Mon, 19 Mar 2018 11:59:25 -0500
Derek Foreman  wrote:

> On 2018-03-19 11:20 AM, Eric Engestrom wrote:
> > On Monday, 2018-03-19 16:10:57 +, Daniel Stone wrote:  
> >> Hi,
> >>
> >> On 19 March 2018 at 16:08, Eric Engestrom  
> >> wrote:  
> >>> On Monday, 2018-03-19 15:13:14 +, Daniel Stone wrote:  
>  +if ! test -f "$LIB"; then
>  + echo "Test binary \"$LIB\" does not exist"
>  + exit 99
>  +fi
>  +
>  +if ! test -n "$NM"; then
>  + echo "nm environment variable not set"
>  + exit 99  
> >>>
> >>> 99? Were you looking for the "skip this test" 77?  
> >>
> >> I did mean 99 'some kind of inexplicable internal error happened'
> >> rather than 77 skip, but I have no strong opinion on it and am happy
> >> to change to whatever is suggested.  
> > 
> > I guess "don't have the tools to test this, skipping" would be fine, but
> > I'm not really involved in the wayland project so my opinion isn't the
> > one that matters the most :P  
> 
> Additional review is valuable, thanks. :)
> 
> > "I have no strong feelings one way or the other"  
> 
> In the absence of strong feelings, I've pushed this as-is.
> 
> Along with the recent version of the "pass nm path to check script" patch.

Hi,

I think 99 is the right one to use. It's an ABI breakage check, we
definitely don't want that to be optional.


Thanks,
pq


pgp_f9PCVw92g.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Derek Foreman]

On 2018-03-19 11:20 AM, Eric Engestrom wrote:

On Monday, 2018-03-19 16:10:57 +, Daniel Stone wrote:

Hi,

On 19 March 2018 at 16:08, Eric Engestrom  wrote:

On Monday, 2018-03-19 15:13:14 +, Daniel Stone wrote:

+if ! test -f "$LIB"; then
+ echo "Test binary \"$LIB\" does not exist"
+ exit 99
+fi
+
+if ! test -n "$NM"; then
+ echo "nm environment variable not set"
+ exit 99


99? Were you looking for the "skip this test" 77?


I did mean 99 'some kind of inexplicable internal error happened'
rather than 77 skip, but I have no strong opinion on it and am happy
to change to whatever is suggested.


I guess "don't have the tools to test this, skipping" would be fine, but
I'm not really involved in the wayland project so my opinion isn't the
one that matters the most :P


Additional review is valuable, thanks. :)


"I have no strong feelings one way or the other"


In the absence of strong feelings, I've pushed this as-is.

Along with the recent version of the "pass nm path to check script" patch.

Thanks,
Derek



Cheers,
Daniel

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



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

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

[Eric Engestrom]

On Monday, 2018-03-19 16:10:57 +, Daniel Stone wrote:
> Hi,
> 
> On 19 March 2018 at 16:08, Eric Engestrom  wrote:
> > On Monday, 2018-03-19 15:13:14 +, Daniel Stone wrote:
> >> +if ! test -f "$LIB"; then
> >> + echo "Test binary \"$LIB\" does not exist"
> >> + exit 99
> >> +fi
> >> +
> >> +if ! test -n "$NM"; then
> >> + echo "nm environment variable not set"
> >> + exit 99
> >
> > 99? Were you looking for the "skip this test" 77?
> 
> I did mean 99 'some kind of inexplicable internal error happened'
> rather than 77 skip, but I have no strong opinion on it and am happy
> to change to whatever is suggested.

I guess "don't have the tools to test this, skipping" would be fine, but
I'm not really involved in the wayland project so my opinion isn't the
one that matters the most :P

"I have no strong feelings one way or the other"

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

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

[Daniel Stone]

Hi,

On 19 March 2018 at 16:08, Eric Engestrom  wrote:
> On Monday, 2018-03-19 15:13:14 +, Daniel Stone wrote:
>> +if ! test -f "$LIB"; then
>> + echo "Test binary \"$LIB\" does not exist"
>> + exit 99
>> +fi
>> +
>> +if ! test -n "$NM"; then
>> + echo "nm environment variable not set"
>> + exit 99
>
> 99? Were you looking for the "skip this test" 77?

I did mean 99 'some kind of inexplicable internal error happened'
rather than 77 skip, but I have no strong opinion on it and am happy
to change to whatever is suggested.

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

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

[Eric Engestrom]

On Monday, 2018-03-19 15:13:14 +, 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.
> 
> v2: Don't test if we can execute $NM.
> 
> 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..70fe1f4c 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 -n "$NM"; then
> + echo "nm environment variable not set"
> + exit 99

99? Were you looking for the "skip this test" 77?

>  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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland] 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.

v2: Don't test if we can execute $NM.

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..70fe1f4c 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 -n "$NM"; then
+   echo "nm environment variable not set"
+   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