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

Remove custom installer, create setup.py #1140

Closed
ghost opened this issue Mar 30, 2020 · 4 comments
Closed

Remove custom installer, create setup.py #1140

ghost opened this issue Mar 30, 2020 · 4 comments

Comments

@ghost
Copy link

ghost commented Mar 30, 2020

Historically, GAM has shipped with a custom installer script. With all of this great work to split up the project into smaller modules, it behooves the project to also include a proper setup.py.

@jay0lee
Copy link
Member

jay0lee commented Mar 30, 2020

Most GAM admins utilize the custom installer today. It ensures they are getting the latest version of Python, OpenSSL and Python libraries without the admin needing to install and maintain any of the above. In short, the custom installer is not going anywhere.

I'm not opposed to a setup.py / GAM being pip-installable but it has not been high enough priority for me to carry out. PRs always welcomed.

@ghost
Copy link
Author

ghost commented Mar 30, 2020

The admin should have to install their own Python, OpenSSL, and Python libraries. It's not okay for one userspace application to dictate OS versions and just start mucking around with internals like that. Instructions in a readme file of what prerequisites are needed offer a clear advantage by removing this project's responsibilities of the admin's machines and removing custom code. As it stands, there's a very difficult-to-read installation script clocking in at 375 lines - 202 of which are raising bugs in shellcheck.

@ghost
Copy link
Author

ghost commented Mar 30, 2020

Case in point, install-gam.sh does not set set -e or set -u, so errors/unset variables will be ignored. Line 220 has temp_archive_dir=$(mktemp -d 2>/dev/null || mktemp -d -t 'mytmpdir'). If at any point that command outputs nothing or an error (e.g. a typo in a commit that updates the line), then the program will happily move along. The end of the file has trap "rm -rf $temp_archive_dir" EXIT, which will rm -rf a potentially unset variable. GAM is now liable for the possibility of wiping out a user's home directory in entirety.

It's an unlikely scenario, but has happened before. This could be easier to maintain and safer if it used standard Python packaging rather than going its own road for no gain.

@jay0lee
Copy link
Member

jay0lee commented Mar 30, 2020

This is actually a dupe of #720. Again, PRs are always welcome.

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

No branches or pull requests

1 participant