-
Notifications
You must be signed in to change notification settings - Fork 103
Fix #178: Make MNT{4,6}-753, cp6_782 tests run conditionally #179
Conversation
This is a good start! Nice find on the action. However as things stand this is a bit too aggressive, because it doesn't handle when the dependencies change. I think the way to handle that would be to check the current |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Okay sweet I think this does what we want LMK |
Looks great! Maybe the only diff would be to update the other |
.github/workflows/ci.yml
Outdated
~/.cargo/registry | ||
~/.cargo/git | ||
target | ||
key: mnt4_753-deps-updated-${{ runner.os }}-${{ hashFiles('**/Cargo.lock') }} |
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.
I think we can avoid the mnt4_753-deps-updated-
prefix, as the Cargo.lock
file is shared across the entire workspace. So we'd be unnecessarily creating an additional cache entry and possibly causing false cache misses if we use this key. Ditto for the other curves.
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 last change didn't result in "cache hits" like I expected...
Does cargo generate-lockfile
need to be added before the cache invocation at line 53?
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.
Second run seems to work as expected. I'll wait for you to let me know when to merge.
Mostly looks good to me! A minor optimization that we could do would be to simply check to see if the cache exists, instead of downloading it also. See https://github.com/actions/cache#inputs for how to do this. Thanks for the great work! |
Sweet! Thanks for the help! I made a little note: arkworks-rs/algebra#720 |
Make MNT{4,6}-753, cp6_782 tests run conditionally
Fixes #178
Fixes #180
What does this PR do?
updates ci.yml so that tests for MNT{4,6}-753 and cp6_782 are run only when either:
I sanity checked the following conditions:
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
n/aFiles changed
in the Github PR explorer ?