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

[Emil Velikov]

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