Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
skalee opened this issue Nov 8, 2018 · 6 comments
Open

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

skalee opened this issue Nov 8, 2018 · 6 comments

Comments

@skalee
Copy link
Contributor

skalee commented Nov 8, 2018

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
Copy link
Contributor

dewyatt commented Nov 13, 2018

I think @ronaldtse wasn't fond of this idea when I proposed it in #8. I believe he wants the "install all" script to be as user-friendly as possible, or that was the impression I got. (#8 (comment))

@skalee
Copy link
Contributor Author

skalee commented Nov 13, 2018

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

@dewyatt
Copy link
Contributor

dewyatt commented Nov 14, 2018

@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
Copy link
Contributor Author

skalee commented Nov 14, 2018

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
Copy link
Contributor Author

skalee commented Nov 16, 2018

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

@skalee
Copy link
Contributor Author

skalee commented Nov 20, 2018

@ronaldtse Does #28 solve the prerequisites requirement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants