On 19 March 2018 at 13:20, Daniel Stone <dan...@fooishbar.org> wrote: > Hi Quentin, > Thanks for the quick review! > > On 19 March 2018 at 13:06, Quentin Glidic > <sardemff7+wayl...@sardemff7.net> 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. <prefix>-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