Fixed tags Makefile recipe

[rjlasko]

Hello.

I love Unison, and have been using it for the past 3 years. Since I like to be able to control my software build process + version, I build Unison from scratch whenever possible. To date, I have built Unison on many different platforms (OSx, FreeBSD, Debian, Ubuntu, Arch Linux, Synology Linux). However, in every build (except OSx, where the 'etags' executable exists), I have encountered the same (non-fatal) error at the very end of the build process:

make tags
make[1]: Entering directory '/tmp/src/unison-2.48.3'
if [ -f `which etags` ]; then \
    etags *.mli */*.mli *.ml */*.ml */*.m *.c */*.c *.txt \
          ; fi 
/bin/sh: 2: etags: not found
Makefile:347: recipe for target 'tags' failed
make[1]: [tags] Error 127 (ignored)
make[1]: Leaving directory '/tmp/src/unison-2.48.3'

Despite the build completing successfully, and everything running fine, this error has always bothered me, as the code indicates that the 'etags' executable is not necessary to complete the build of the unison executable. I've tracked down the error to this line of code in the source Makefile.

First, the 'which' command (which is not as portable as 'command' or 'type') will return nothing if the file is not found, which means that '[ -f "" ]' will be the command run, which is both valid and evaluates true. This combination is what yields the error above.

To fix the problem, we can instead interpret the (which, command, or type) executable generating some output to stdout as an indication that the directive exists or executable file is present on the PATH. Our saviour here is the syntax '[ -n "/x/y/z" ]', which tests if the string is not empty. In this case, '[ -n "/some/path/etags" ]' evaluates to true, and we know the command exists and we can proceed to run the etags command. If not, then we instead get '[ -n "" ]', which evaluates false, allowing us to skip.

I've tested this on Ubuntu 16.04 with BASH, and it works fine. Easy peasy. Hopefully you will accept this pull request, as it is pretty straightforward.

Thanks,
Rob

[ian-kelling]

I actually changed that line of code recently to deal with this issue. I only tested the it on debian, but it's working there.

'[ -f "" ]' will be the command run, which is both valid and evaluates true

No, that evaluates to false under dash and bash. The old code incorrectly evaluated to true. Try out the latest sources and let us know if it is still a problem.

[rjlasko]

Sure. [ -fwhich etags] syntax reduces to [ -f ]. What i wrote in the message was merely an attempt to denote the nature of the former code reducing the which etags evaulation down to an empty string. Despite this, we both agree that the result was that the expression incorrectly evaluated true.

I guess that if there was a second point that I was trying to address with my solution was that wrapping the which command with a file test seemed redundant, as the which command will already evaluate non-zero if the etags executable is not found in the path. The most simplistic solution here would be to use [ $(which etags) ].

In any case. I'm certain that your solution will work just fine, and look forward to no longer having to think about whether the build failed when seeing the etags error.

thanks!

[ian-kelling]

Thanks @rjlasco. I agree with you.