Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Changes to shasum package #50

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Conversation

madsnedergaard
Copy link
Member

Changes from sha1sum which is not installed on MacOS to shasum which should be available on all unix-based systems.

shasum by default uses the sha1 algorithm, so it should have no effect on anything AFAIK.

Fixes #49

@skovhus
Copy link
Contributor

skovhus commented Sep 3, 2020

Thanks for fixing this! I usually bootstrap my OS X installation with relevant gnu tools and replaces the bsd tools, so I didn’t catch this one :/

What is the performance of shasum (perl) compared to sha1sum (c)? If sha1sum is much faster, we could update the README with the brew install step.

To test the performance you could run the command on the entire src folder in tmrow. :)

@madsnedergaard
Copy link
Member Author

Thanks for fixing this! I usually bootstrap my OS X installation with relevant gnu tools and replaces the bsd tools, so I didn’t catch this one :/

What is the performance of shasum (perl) compared to sha1sum (c)? If sha1sum is much faster, we could update the README with the brew install step.

To test the performance you could run the command on the entire src folder in tmrow. :)

I tried removing everything except the hashing to actually test in on Drone, and there is a significant difference - now sure if it's enough to ask all brick users to also run a brew install md5sha1sum first though :)

Screenshot 2020-09-03 at 12 06 41Screenshot 2020-09-03 at 12 07 46

@skovhus
Copy link
Contributor

skovhus commented Sep 3, 2020

Thanks for testing the performance so carefully! I thought you would just test locally.

Not sure how much it affects the overall performance? It is just a few seconds, then let us not bother our dear users with a brew install step.

If it is more substantial then I think an additional install step is acceptable for an increase in build time.

@madsnedergaard
Copy link
Member Author

Thanks for testing the performance so carefully! I thought you would just test locally.

I also tested locally, but I've come to the conclusion that everything takes way longer when running in CI - so I don't really trust local comparisons for this 😄

Not sure how much it affects the overall performance? It is just a few seconds, then let us not bother our dear users with a brew install step.

If it is more substantial then I think an additional install step is acceptable for an increase in build time.

The screenshots shows running only the get_hash function for all packages, so it's 1.8s for sha1sum and 5.53s for shasum. So I think we should stick with shasum for now as it's a fairly small costs for users unless they have listed a ton of inputs (which they probably shouldn't anyway).

Copy link
Contributor

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

🚢

@madsnedergaard madsnedergaard merged commit 52029c7 into master Sep 3, 2020
@madsnedergaard madsnedergaard deleted the mn/switch-shasum-package branch September 3, 2020 10:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing dependency sha1sum
3 participants