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 <dani...@collabora.com>
Reported-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
Fixes: 21b1f22eb056 ("wayland-egl: enhance the symbol test")
Cc: Emil Velikov <emil.veli...@collabora.com>
---
  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. <prefix>-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