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

use default shasum for os x if present #452

Merged
merged 1 commit into from
Jul 2, 2024
Merged

use default shasum for os x if present #452

merged 1 commit into from
Jul 2, 2024

Conversation

ppannuto
Copy link
Member

Fixes #450.

@ppannuto ppannuto requested a review from bradjc June 28, 2024 23:12
Copy link
Member

@alevy alevy 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 this is hacks on hacks that is going to be impossible to reliably test and maintain.

let FOUND=0

# Try from each mirror until we successfully download a .zip file.
for MIRROR in ${MIRRORS[@]}; do
URL=$MIRROR/$ZIP_FILE
echo "Fetching libc++ from ${MIRROR}..."
echo " Fetching ${URL}..."
wget -O $ZIP_FILE "$URL" && (echo "$GCC_SHA $ZIP_FILE" | sha256sum -c)
# Note: There must be two space characters for `shasum` (sha256sum doesn't care)
Copy link
Member

Choose a reason for hiding this comment

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

Two space characters where? what is this referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

Between the correct sha and the file name:

❯ echo "1e6f33cd5f44acefebc1ef677db4be33829a5228  README.md" | shasum -a 256 -c
README.md: OK
❯ echo "1e6f33cd5f44acefebc1ef677db4be33829a5228 README.md" | shasum -a 256 -c
shasum: standard input: no properly formatted SHA checksum lines found

@@ -19,14 +19,21 @@ MIRRORS=(\
"https://alpha.mirror.svc.schuermann.io/files/tock"\
)

if test -x /usr/bin/shasum; then
Copy link
Member

Choose a reason for hiding this comment

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

This at least needs a comment. Why this particular path? Do we always prefer shasum if it is present? Or only if sha256sum isn't present?

Is this the shasum that comes with Perl? If so, Perl is a pre-requisite for most Linux distributions in practice, does it make sense to just use shasum?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what happened in the kernel: tock/tock#1669. Should we have done something differently? I think we just want this to work on as many systems as possible.

@@ -17,14 +17,21 @@ MIRRORS=(\
"https://alpha.mirror.svc.schuermann.io/files/tock"\
)

if test -x /usr/bin/shasum; then
Copy link
Member

Choose a reason for hiding this comment

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

That this is duplicated is a bit of a red flag.

Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

Solution looks good to me, and this does solve a real problem.

It's disappointing that this is still an issue, and (apart from all the other reasons why this is bad) we probably don't want to require a host C/Rust toolchain to build our own sha256sum tool in this repo.

@ppannuto
Copy link
Member Author

ppannuto commented Jul 2, 2024

I think this is hacks on hacks that is going to be impossible to reliably test and maintain.

For better or worse, it's just propagating over the existing approach (hack? :) ) from the kernel repo, which has been unchanged for years now. Think this hits least-bad pragmatic approach for the current ecosystem of developer machines writ large.

@alevy alevy added this pull request to the merge queue Jul 2, 2024
Merged via the queue into master with commit 30f4ee9 Jul 2, 2024
4 checks passed
@alevy alevy deleted the shasum branch July 2, 2024 19:06
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.

lib: newlib and libc++ scripts assume sha256sum
4 participants