-
Notifications
You must be signed in to change notification settings - Fork 62
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
Spack cache change, bump versions #1926
Conversation
brenthuisman
commented
Jul 21, 2022
•
edited
Loading
edited
- It's actually a Github cache trick: Feature request: option to update cache actions/cache#342 (comment)
- First run: 1h 7m 38s
- Second run: same duration. Hmm....
- Third run: ~10 minutes!
- 4th: ~7.5 min.
- Bump in configs: macos 10.15 will disappear in a month: The macOS 10.15 Actions runner image will begin deprecation on 5/31/22 and will be fully unsupported by 12/1/22 for GitHub and ADO actions/runner-images#5583
- Bump clang-max to 14, gcc-max to 11
- macos-min to 11, macos-max to 12.
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.
A few questions and a more general comment: this is actually mainly bumping versions
and thus mixes two concerns.
.github/workflows/lint.yml
Outdated
@@ -12,7 +12,7 @@ jobs: | |||
fail-fast: false | |||
steps: | |||
- name: Set up Python | |||
uses: actions/setup-python@v2 | |||
uses: actions/setup-python@v4 | |||
with: | |||
python-version: 3.6 |
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.
Shouldn't this be 3.8 by now?
Suggestion beyond this PR: Figure out how to setup global defines in these YAML files. Maybe there's #include
and we can set some values (like py-version) once.
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.
Python min-ver is 3.7, so yes, that can be updated.
Macos wheels are 3.8 and up, because that's the first version that support 'fat wheels'. People who use 3.7 should still be able to build themselves.
.github/workflows/spack.yml
Outdated
@@ -23,8 +23,8 @@ jobs: | |||
uses: actions/cache@v2 | |||
with: | |||
path: ~/.spack-cache | |||
key: cache-${{ github.sha }} | |||
restore-keys: cache- | |||
key: ccache-${{ github.run_id }} |
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.
Why the double c
? There's a tool ccache
, but this is not used here.
It might be the origin of the naming in the linked GH Actions issue, though.
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.
no reason.
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.
Interestingly it does matter. The commit where I removed it failed, adding back it passed. I can think of no other reason that that the action in fact uses ccache
, because what else would you use :)
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.
That's very obtuse. Please make a big, red flashing sign for that.
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.
Dug a little, and could not find any reason to believe it actually matters. Probably the cache was just cold after deleting a c
. I changed it to sth that assumes no magic, and trust that it'll work. It'll take a run or two extra if you want to be absolutely sure.
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.
There's no need for the double "c" :)
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.
👍