-
-
Notifications
You must be signed in to change notification settings - Fork 186
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(actions): swap sha256sum for shasum -a 256 #1390
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1390 +/- ##
==========================================
+ Coverage 97.24% 97.30% +0.06%
==========================================
Files 48 48
Lines 4423 4423
==========================================
+ Hits 4301 4304 +3
+ Misses 122 119 -3
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for discovering this error and taking the initiative to fix it, @kenibrewer! 🙏
Your current fix works for macOS and Ubuntu, but it doesn't work for Windows:
D:\a\_temp\29fa6760-37f4-4d7b-9324-e184f44d2852.sh: line 1: shasum: command not found
I see three options:
- Use Python's
hashlib
module (see my inline suggestions). - Use the cache key computation from Poetry's own CI config.
- Make this step conditioned on the OS and add another one with
sha256sum
for Windows.
I'm leaning towards solution 2, but solution 1 is less intrusive. Solution 3 adds too much duplicate code for my taste, but I wanted to mention it nevertheless.
I like solution 2 best as well. It's a lot more readable while solution 1 is pretty hard to parse. |
1e671a0
to
1fe34b7
Compare
1fe34b7
to
5d0485a
Compare
@sisp Can I get another workflow run? |
Looks like that didn't work for the windows install either.
I set up a test PR into my own fork so I can run the workflows myself. I'll probably test option 1 next and see if I can make it clearer and readable. |
a00e055
to
1d2b31d
Compare
Previous approach was broken on macOS due to missing sha256sum command. This adopts the same cache-key approach as is used in Poetry pipelines. Closes copier-org#1389
1d2b31d
to
6141276
Compare
@sisp This is now passing all tests and generates cache keys on all platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, @kenibrewer, thank you! 🙏
This addresses an issue in macOS builds may be impacting caching. The command
sha256sum
which doesn't exist inmacOS-latest
is replaced withshasum -a 256
which is present in bothubuntu-latest
andmacOS-latest
Closes #1389