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

[Daniel Stone]

Hi Emil,

On 19 March 2018 at 13:27, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 19 March 2018 at 13:20, Daniel Stone <daniel at fooishbar.org> 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. <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.

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