rnpgp / gpg-build-scripts

Build scripts to automatically install GnuPG (GNU Privacy Guard) on Linux
MIT License
5 stars 4 forks source link

The purpose of prerequisites installer in "install all" #21

Open skalee opened 6 years ago

skalee commented 6 years ago

Currently, the gpg_install_all.sh script is capable of installing prerequisite packages:

https://github.com/riboseinc/gpg-build-scripts/blob/f0051bb9390f33a1ea96dec1546bd9171b6e63fc/install_gpg_all.sh#L40-L42

The problem is that following piece of code has been written specifically for CentOS. No other distributions are supported:

https://github.com/riboseinc/gpg-build-scripts/blob/f0051bb9390f33a1ea96dec1546bd9171b6e63fc/install_gpg_all.sh#L81-L86

Even worse, these four items aren't enough in case of clean CentOS installation (e.g. from Docker image)—PinEntry requires some additional package in order to show password dialog (ncurses-devel is a frequent choice).

Finally, the detect_platform() function is partly buggy—it requires the presence of /etc/os-release file, and prints error when it's missing (e.g. on OS X), but that does not stop the build:

https://github.com/riboseinc/gpg-build-scripts/blob/f0051bb9390f33a1ea96dec1546bd9171b6e63fc/install_gpg_all.sh#L44-L48

Of course these issues can be fixed, however IMHO the prerequisites installer is too tightly bound to target system, and should be removed from this script. I can think of two better solutions:

  1. Extract prerequisites installer to a separate script, e.g. install_gpg_prerequisites.sh.
  2. Do not provide any prerequisites installer, and cede this responsibility to user (required software should be listed in README then).

@dewyatt @ronaldtse WDYT? I am leaning towards option 2.

Also, if you want some easy install on CentOS Docker images, a Dockerfile can be added to this repository. Actually, I wrote it already: https://github.com/riboseinc/gpg-build-scripts/blob/bbbb4321526611987858a30ea04c30ebf1ca7730/Dockerfile. Though it requires some review, I'm not proficient with Docker.

dewyatt commented 6 years ago

I think @ronaldtse wasn't fond of this idea when I proposed it in https://github.com/riboseinc/gpg-build-scripts/pull/8. I believe he wants the "install all" script to be as user-friendly as possible, or that was the impression I got. (https://github.com/riboseinc/gpg-build-scripts/pull/8#issuecomment-432490325)

skalee commented 6 years ago

I am removing this feature in #17 then. IMHO it only confuses and makes things complicated. Dockerfile is a better idea.

dewyatt commented 6 years ago

@skalee I think you may have misinterpreted what I meant. I meant that I think @ronaldtse does want the pre-requisite installation in install_gpg_all.sh.

skalee commented 6 years ago

If the main intention was to easy install in Docker container, then proper Dockerfile seems user-friendlier. And I have a strong suspicion that indeed easy install in Docker container was the main concern, because docker.sh script boots Docker container from a CentOS image, and that current prerequisites installer works only in CentOS.

Nevertheless, if my suspicion is wrong, then I can keep that prerequisites installer.

My strongest argument against is that it may become a maintenance hell. It is a potentially very large number of control flow branches (one per *NIX distribution), which should be maintained and tested. Well, I know how to do it in Travis (thanks to Docker), but I'm not a big fan of test suites which take like an hour to run.

@ronaldtse WDYT?

skalee commented 6 years ago

FYI with #17 being merged, the prerequisites installer has been quietly removed. However, questions above remain open IMO.

skalee commented 6 years ago

@ronaldtse Does #28 solve the prerequisites requirement?