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

Make install script POSIX compliant #53

Merged
merged 3 commits into from Apr 16, 2021
Merged

Conversation

vladimyr
Copy link
Contributor

@vladimyr vladimyr commented Apr 5, 2021

Apart from being POSIX compliant this supports configuring install location through standard PREFIX environment variable, for example:

PREFIX="$HOME/.local" sh -c "$(curl -fsSL https://github.com/soywod/himalaya/raw/master/install.sh)"

@vladimyr vladimyr force-pushed the posix-install branch 2 times, most recently from 93a5525 to e203b16 Compare April 5, 2021 03:33
Copy link
Owner

@soywod soywod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the improvement!! I just have some questions (I put them in a review).

README.md Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@soywod soywod added the question Further information is requested label Apr 6, 2021
@chambln
Copy link
Contributor

chambln commented Apr 6, 2021

Sorry for the suggestions spam. A lot of this is redundant assignments passing values around. The gist of the program is as follows:

  1. Detect OS by matching uname -s | tr [:upper:] [:lower:] against known values and exit nonzero if unrecognised
  2. Download the corresponding release archive using curl(1)
  3. Extract the archive using tar(1)
  4. Copy himalaya.exe to /usr/local/bin/himalaya and make it executable
  5. Clean up any temporary files upon exit using trap(1)

There may be more robust ways of doing OS detection in a POSIX friendly way, but uname(1) might be enough (what's the uname output on Windows systems?). Could steal some some ideas from https://github.com/dylanaraps/pfetch.

Step 4 should probably be done as root. Can we call it a POSIX shell script if it makes calls to sudo? It may be better to suggest the user pipes the whole script into sudo sh than have sudo commands in the script. Then the user could substitute sudo(1) for doas(1) or su(1) without having to edit the script.

README.md Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@soywod soywod force-pushed the posix-install branch 2 times, most recently from 491b3dd to 03cb191 Compare April 15, 2021 13:26
@soywod
Copy link
Owner

soywod commented Apr 15, 2021

@chambln I did all the changes you proposed. Could you check one more time before I merge?

@soywod soywod added cli Related to the CLI optimization Optimization and removed question Further information is requested labels Apr 15, 2021
@vladimyr
Copy link
Contributor Author

Sorry for being unresponsive, I've had limited FOSS time for the last few days. Anyway if you are still interested I'll gladly go through your questions and explain the rationale behind my choices.

@soywod
Copy link
Owner

soywod commented Apr 16, 2021

@vladimyr I am sorry, I took the initiative to push commits on your branch to merge faster. Even if the discussion is not terminated with @chambln, the result is way better than what we got before 😃

Anyway if you are still interested I'll gladly go through your questions and explain the rationale behind my choices.

With great pleasure! Thanks for you contribution anyway.

@soywod soywod merged commit dd45b4b into soywod:master Apr 16, 2021
@soywod soywod self-assigned this Apr 17, 2021
@soywod soywod added this to the v1.0.0 milestone Apr 17, 2021
@chambln
Copy link
Contributor

chambln commented Apr 18, 2021

Looks much better already!

If POSIX compliance is the goal then sudo must not be used in the script. I think it's wise to leave privilege elevation to the person running the script anyway. We can still suggest using sudo in README.md, i.e.

curl ... | sudo sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the CLI optimization Optimization
Projects
None yet
3 participants