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

fix(install): remove wget and sha256sum dependencies #1013

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

curzolapierre
Copy link
Member

@curzolapierre curzolapierre commented Sep 26, 2023

Fixes #1012

Tested on Benjamin's Mac

@curzolapierre curzolapierre self-assigned this Sep 26, 2023
Comment on lines +3 to +6
function sha256sum() {
shasum -a 256 "$@"
}

Copy link
Member Author

@curzolapierre curzolapierre Sep 26, 2023

Choose a reason for hiding this comment

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

This is the actual fix for sha256sum

Copy link
Member

Choose a reason for hiding this comment

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

question: from what I understood of the actual problem, the call to sha256sum works as this script used to work on Benjamin's Mac. It's the introduction of the function check_command_exists that made the install fail. Isn't it?

Copy link
Member Author

@curzolapierre curzolapierre Sep 27, 2023

Choose a reason for hiding this comment

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

Not really, it never worked on Benjamin's Mac since the addition of the call to sha256sum.
This binary seems not installed by default on MacOS.

The introduction of check_command_exists stopped the script before the error occurred.

checksum_computed=$(sha256sum ${tmpdir}/${archive_name} | cut -d" " -f1)
checksum_expected=$(wget --quiet --output-document - $checksums_url | grep $archive_name | cut -d" " -f1)
checksum_computed=$(sha256sum "${tmpdir}/${archive_name}" | cut -d" " -f1)
checksum_expected=$(curl --silent --location "$checksums_url" | grep "$archive_name" | cut -d" " -f1)
Copy link
Member Author

@curzolapierre curzolapierre Sep 26, 2023

Choose a reason for hiding this comment

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

This is the actual fix for wget

Copy link
Member

Choose a reason for hiding this comment

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

question: I have the same question as the one raised above. From what I understand of the issue, the problem is not the usage of wget that is here for years according to the Git history. The problem is the call to check_command_exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes check_command_exists blocked user that didn't have wget installed.
But in the same way as this command failed due to missing binary.

I found it more consistent to use curl as we are already asking to use for the download https://doc.scalingo.com/platform/cli/start

@curzolapierre curzolapierre marked this pull request as ready for review September 26, 2023 14:26
Comment on lines +3 to +6
function sha256sum() {
shasum -a 256 "$@"
}

Copy link
Member

Choose a reason for hiding this comment

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

question: from what I understood of the actual problem, the call to sha256sum works as this script used to work on Benjamin's Mac. It's the introduction of the function check_command_exists that made the install fail. Isn't it?

@@ -58,14 +54,14 @@ main() {
echo
}

if [ "x$DEBUG" = "xtrue" ] ; then
if [ -n "$DEBUG" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

it's not equivalent. Why don't we keep the equivalent?

Suggested change
if [ -n "$DEBUG" ] ; then
if [ "$DEBUG" = "true" ] ; then

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not equivalent, I change it to be more consistent with the usage of the DEBUG of the CLI itself (which can be used with DEBUG=1).

checksum_computed=$(sha256sum ${tmpdir}/${archive_name} | cut -d" " -f1)
checksum_expected=$(wget --quiet --output-document - $checksums_url | grep $archive_name | cut -d" " -f1)
checksum_computed=$(sha256sum "${tmpdir}/${archive_name}" | cut -d" " -f1)
checksum_expected=$(curl --silent --location "$checksums_url" | grep "$archive_name" | cut -d" " -f1)
Copy link
Member

Choose a reason for hiding this comment

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

question: I have the same question as the one raised above. From what I understand of the issue, the problem is not the usage of wget that is here for years according to the Git history. The problem is the call to check_command_exists.

Copy link
Member

@EtienneM EtienneM 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 answers :)

@curzolapierre curzolapierre merged commit ed93375 into master Sep 27, 2023
5 checks passed
@curzolapierre curzolapierre deleted the fix/1012/fix_install_script_on_mac_os branch September 27, 2023 12:08
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.

[MacOS] Install script failed due to usage of sha256sum
2 participants