Re: [PATCH wayland 2/2] wayland-egl: Make symbol test fail on failure
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
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
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
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
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
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
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