IanDarwin / pdfshow

Deliver/present talks from PDFs - A simple PDF presenter oriented towards slideshow decks in PDF form
Other
8 stars 4 forks source link

Error building package on Linux #44

Closed j75 closed 3 years ago

j75 commented 3 years ago

I think the mkinstaller script requires bash, otherwise the following message is issued (on Ubuntu 20.04):

./mkinstaller: 7: function: not found

However, running the script as bash ./mkinstaller ... works smoothly, so I would suggest to modify the first line of the script to something like #!/bin/bash or #!/usr/bin/env bash.

Please also note that passing the script to a script-checker program (such as shellcheck) displays some warnings:

In mkinstaller line 41:
shift `expr $OPTIND - 1`         # leave just filenames
      ^----------------^ SC2046: Quote this to prevent word splitting.
      ^----------------^ SC2006: Use $(...) notation instead of legacy backticked `...`.
       ^--^ SC2003: expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].

Did you mean: 
shift $(expr $OPTIND - 1)        # leave just filenames

In mkinstaller line 60:
    ${OS_SPECIFIC}
        ^------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
    "${OS_SPECIFIC}"
...
IanDarwin commented 3 years ago

Thanks very much for the feedback.

I've lived an breathed *Nix systems for decades and have seen a lot of things come and go. I've learned to respect automated testing tools but never substitute their judgement for my own. And please understand that I try hard to make my software portable (like my file(1) on Ubuntu), and that Linux is not the only game in town, nor bash the only shell. This script has to run on macOS, which is based on BSD Unix, and in my daily work I run on OpenBSD. On some systems, you might be surprised to note that bash is not even installed by default.

If you look around, there are systems on which /bin/sh is a very old Bourne shell, some on which it's linked to bash, some on which it's linked to a Korn shell, and so on. The intent has always been to provide a usable but minimal shell under the name /bin/sh, but there is no way of knowing in advance how minimal or not-minimal you are going to find on a given system.

I have updated the command-sub to use $() syntax to suppress that warning.

There are now 500 or so active Linux distros. I suspect they don't all implement the exact same set of shell implementations, but don't have time to test them all :-). Obviously this needs to work on Ubuntu. So, I have committed a version of mkinstaller which uses the older shell syntax for functions, and would be grateful if you could please test it on your system's /bin/sh and let me know how that works out before I roll it as a new version. Thanks!!

OTOH, 'shellcheck' is wrong about line 60: A) We don't care if it is split on spaces - it is command args. B) Worse: Putting the variable in quotes, on the default shell on some systems that don't have any OS specifics, results in a failure to run the script (not just a warning), because quoting the empty string gives a value of '', rather than letting it disappear. So I left that as is. Always the risk of automated tools: they rarely have the full context.

Again, thanks for helping make this more widely portable.

IanDarwin commented 3 years ago

With my latest commit, I tested on Rocky (CentOS replacement) and the old-style function def'n seems to work. I'll roll this into the next release. Thanks!

j75 commented 3 years ago

Yes, now it works on my Linux system. And I agree with you that sh is definitely the most portable shell-script to use (I think it is part of POSIX), so your choice is valid (I didn't actually try to convince you of the contrary, the script simply didn't work on my system).

OTOH if you permit, I would just simply suggest to check the existence of the jpackage binary at the beginning of the script (or before launching mvn).