NFFT / nfft

The official NFFT library repository
https://tu-chemnitz.de/~potts/nfft/
GNU General Public License v2.0
174 stars 46 forks source link

[bootstrap.sh] Possible adjustments #122

Closed kevinmatthes closed 3 years ago

kevinmatthes commented 3 years ago

I took a look at bootstrap.sh and noticed two points which may be adjusted in order improve it.

These problems are in detail as follows:

  1. Missing tutorial: users might take a look at the implementation of bootstrap.sh and try to search the named tutorial. Since it does not exist, yet, this can lead to questions, how to build the library without the shell script. Furthermore, some users could be disappointed when they relied on the existence of the tutorial.
  2. Shebang: the shebang is used in UNIX systems to tell the executing shell where to look for the intended interpreter. Since there are different possibilities to state the desired interpreter, e. g. by naming the installation path or the path to the referencing variable, one should prefer the most general method to define it. This avoids problems on machines where the actual installation path differs from the stated one.

I would like to suggest the following options in order to fix these problems:

  1. Either create the tutorial or remove the reference to it.
  2. Change the shebang to #!/usr/bin/env bash. When using Python 3, for instance, developers should definitely name the intended interpreter since there are still some Linux distributions which still support Python 2 -- although this support is supposed to end within the next years. In order to avoid problems because of version specific features, the interpreter should be defined clearly. Since one cannot rely on the same installation path on other machines, it is quite common to define it as #!/usr/bin/env python3 since this variable, env, stores the information about which Python 3 interpreter is set as default one. This semantic works with Bash, as well, although most machines offer a Bash installation in /bin/bash. This is just an enhancement in order to avoid unnecessary errors on machines where the default installation path differs.

Furthermore, the latter point, (2), also concerns the configure script, as well, which is created during the execution of bootstrap.sh. Here, the shebang #! /bin/sh is even more problematic, as it contains an additional space character, for example. Hence, some IDEs consider it completely faulty (919 errors, 81 warnings, 130 suppressed further errors and warnings in VSCodium, for instance). Since the bootstrap.sh already requires Bash in order to create configure, the shebang should be changed to #!/usr/bin/env bash, as well. We should think about redesigning it in order to avoid this unintended behaviour of IDEs when viewing this temporary file, but this should be rather discussed in a separate issue, I suppose.

What do you think about these two points?

kevinmatthes commented 3 years ago

I just adjusted the shebang and tested it on Debian 10 (just the execution of bootstrap.sh). The script still works properly (the output is the same as before). If the change should cause problems on other OS, please leave an according comment here.

michaelquellmalz commented 3 years ago

I referenced to README instead of the tutorial, which is not anymore in the git repository.