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

Making install script more robust #233

Merged
merged 2 commits into from
May 19, 2023
Merged

Conversation

saccarosium
Copy link
Contributor

Hi,
This PR tries to make the install script more robust, and more portable to other system other than mainstream Linux distros.

Changes:

  • The script is now POSIX compliant. This means that runs without problems on all those system that either don't have bash installed or has a very outdated version (e.g. MacOS, BSD, Alpine, etc).
  • Fix some shell scripting errors
  • Adds some shell scripting best practices
  • Better logging by showing error code in che echoed message

Copy link
Owner

@michaelb michaelb left a comment

Choose a reason for hiding this comment

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

Overall a good idea, but there are too many things I don't like for me to accept this PR.

Even so, thanks for contributing.

README.md Outdated Show resolved Hide resolved
install.sh Outdated
force_build=$1
local_version="v$(grep "^version" Cargo.toml | cut -d '"' -f 2)"

if [ "$1" = "-f" ] || [ "$1" = "--force" ]; then
Copy link
Owner

Choose a reason for hiding this comment

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

while conceptually better than just checking if there is an argument to the script, this would break many configs, and this is not even proper parsing (it would have to be modified to accomodate other arguments).

I don't think this is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this is not even proper parsing (it would have to be modified to accomodate other arguments)

I know but does the script more elaborate parsing? You were checking the first argument of the script, that could be anything. This is very unpredictable and it is a bad design choice handling this type of input.

I've chosen -f because it is a common pattern in cli application, but we could make it so if the user input anything other than one the script will exit and print an usage string.

# if $1 is not empty and not 1 exit
if [ -n $1 ] && [ $1 != 1 ]; then
  echo "Usage: install.sh <1>"
  echo "1: force install"
  exit 1
fi

@saccarosium
Copy link
Contributor Author

If you want we can maintain the script without structural change and only convert it to a POSIX shell for better compatibility

@michaelb
Copy link
Owner

That would make the PR go from 'meh' to 'great' :-)

I think the CI will pick up automatically the new interpreter

However, another user had proposed a change on install.sh since you opened this PR (that allows seeing in a plugin's manager window the output of cargo build while installing), so you may want to rebase your changes, or maybe create a new PR altogether

@saccarosium
Copy link
Contributor Author

It is the same script but now can run on every POSIX complaint OS. Made che changes and rebase on new master

@michaelb michaelb merged commit c73adb9 into michaelb:master May 19, 2023
5 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants