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

Install script fixes for better Mac support #1194

Merged

Conversation

richwrightnyc
Copy link
Contributor

@richwrightnyc richwrightnyc commented Jan 17, 2023

Trying to install on Mac and encountered a bug. At line 18, sha256sum was throwing an error because that command has a different syntax on Macs.

Verifiying checksum...
Error: ./support/scripts/install: line 18: sha256sum: command not found

Fixed this by adding additional logic for checksum validation if uname -s returns "Darwin"

Created an array for supported system types, for better readability and future extensibility.

@richwrightnyc richwrightnyc marked this pull request as ready for review January 17, 2023 05:59
@sundowndev sundowndev linked an issue Jan 17, 2023 that may be closed by this pull request
Copy link
Owner

@sundowndev sundowndev left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @HudsonOnHere 👍🏻

However I don't think that's a great way of handling this. Verifying checksum is optional and sha256sum could be missing on other platforms too (assuming this script can evolve at any time to support more platforms). Instead, I would suggest we simply skip checksum verification and display a warning message such as "WARNING: checksum verification was skipped because sha256sum is not installed."

EDIT: another solution could be to check whever sha256sum or shasum is installed and use one of them, without checking the platform. If none is installed, exit with error.

WDYT?

@sundowndev
Copy link
Owner

Eventually, this PR could fix #1202 as well. I think we can drop usage of wget and use curl for all requests.

@richwrightnyc
Copy link
Contributor Author

richwrightnyc commented Jan 29, 2023

Thanks for your PR @HudsonOnHere 👍🏻

However I don't think that's a great way of handling this. Verifying checksum is optional and sha256sum could be missing on other platforms too (assuming this script can evolve at any time to support more platforms). Instead, I would suggest we simply skip checksum verification and display a warning message such as "WARNING: checksum verification was skipped because sha256sum is not installed."

EDIT: another solution could be to check whever sha256sum or shasum is installed and use one of them, without checking the platform. If none is installed, exit with error.

WDYT?

@sundowndev Thanks for your feedback -- I like your idea about checking for which shasum is installed, I did not realize that the intention was for this validation to be optional, so I will update the code so that it does not fail if the validation does not succeed.

Eventually, this PR could fix #1202 as well. I think we can drop usage of wget and use curl for all requests.

I can also update the code to use curl here instead, that should be a simple fix.

EDIT: I just spent some time on the two items above and I think I have a good idea of how to accomplish both of these. I'll have an updated PR soon.

@sundowndev
Copy link
Owner

sundowndev commented Jan 30, 2023

@HudsonOnHere Thanks for following up!

On a second thought, let's perform the check on which program is available and then still exit with error when the verification fails. However, we can display a warning message if neither sha256sum or shasum is installed.

@richwrightnyc
Copy link
Contributor Author

Sounds good to me -- I should have some updates in the next day or two.

…ptional arguments, a path to manual download if the automatic checks fail, and fallbacks to curl if wget is not installed.
@richwrightnyc
Copy link
Contributor Author

@sundowndev I think this update should address all of our concerns. Sorry for the amount of changes introduced, but I thought it would be best to refactor the code so that we could handle everything better and make it more robust.

Here's what changed:

  • Made checksum validation mandatory
  • added an optional flag to skip checksum in case it's needed
  • added a manual download mode as an option, or as a fallback in case the automatic check with uname fails for some reason.
  • added a detailed help message with usage examples
  • added checks for wget before downloading, if not installed use curl
  • added checks for sha256sum before checksum validation, if not installedl use shasum

support/scripts/install Outdated Show resolved Hide resolved
support/scripts/install Outdated Show resolved Hide resolved
support/scripts/install Show resolved Hide resolved
@sundowndev sundowndev mentioned this pull request Feb 16, 2023
Copy link
Owner

@sundowndev sundowndev left a comment

Choose a reason for hiding this comment

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

I think we're almost good.

Remaining changes:

  • Fix the user input not working on manual mode
  • Fix issues highlighted by shellcheck

…f =~ to match as a regex rather than literally.
1. SC2155 (warning): Declare and assign separately to avoid masking return values.
2. SC2061 (warning): Quote the parameter to -name so the shell won't interpret it.
@richwrightnyc
Copy link
Contributor Author

richwrightnyc commented Mar 1, 2023

Remaining changes:

  • Fix the user input not working on manual mode

@sundowndev Please see my comment on our thread above, the solution is to fetch the script using a different command

bash <( curl -sSL https://raw.githubusercontent.com/sundowndev/phoneinfoga/78c4478f7b6988531e041e9196b0ca768cef975e/support/scripts/install ) -m
  • Fix issues highlighted by shellcheck (see below)

These issues have been corrected

Merge branch 'master' into feat/install-script-mac-support
sundowndev

This comment was marked as resolved.

@richwrightnyc
Copy link
Contributor Author

Left a comment about the list of supported OS

Sorry I must be missing it, I don't see anything

@sundowndev
Copy link
Owner

@HudsonOnHere Sorry it was a mistake on my end.

@sundowndev sundowndev merged commit 7d15bd6 into sundowndev:master Mar 3, 2023
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.

Bug - Install script fails on Mac
2 participants