-
Notifications
You must be signed in to change notification settings - Fork 68
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
ci: use maintained action to setup rust toolchain #585
Conversation
b779455
to
f334d14
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
=======================================
Coverage 84.38% 84.38%
=======================================
Files 77 77
Lines 19224 19226 +2
Branches 19224 19226 +2
=======================================
+ Hits 16222 16224 +2
Misses 2200 2200
Partials 802 802 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
nice thanks! changes to existing CI LGTM, just a question on adding dependabot and also overall question: it looks like the two most popular rust toolchains in CI are dtolnay/rust-toolchain and actions-rust-lang/setup-rust-toolchain with the latter saying that it was heavily inspired by dtolnay's? I guess this has caching built-in as the main benefit?
.github/dependabot.yaml
Outdated
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.
oh nice! are we planning on enabling dependabot for the repo?
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.
well - i guess thats up for debate. but since this was literally just copy and paste from somewhere else, it was low cost to spark the discussion by just providing the solution 😆.
From my end, I think the utility may unfortunately be limited as auto-updates for the major dependencies (e.g. arrow) almost never work out of the box - at least in delta-rs. That said, I still landed on "let's add it" to keep the actions fresh and have the occasional rust version bump that works.
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.
yea I think it is helpful insofar as it just automatically opens small PRs to bump versions and we may (probably) have to manually help upgrade but the 'reminder' to upgrade seems useful? curious to hear what others think
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.
the dependabot-quickstart-guide doesn't recommend setting up manually like this, but rather suggests going into settings and letting github create this file. Wondering if we should just do it that way?
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'm okay trying this out, but also a bit skeptical of the utility here. As far as I can tell it only runs cargo update
. Generally with how we've set up our Cargo.toml
this will only do minor updates (i.e. it'll never update arrow 53->54). Maybe that's good, but we need a process to also run cargo outdated
and try and stay on top of more major updates as well, so I'm not sure how much time this saves us.
just want to make sure: is the caching working? I see the following errors in test (ubuntu-latest) in the post-run step:
|
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.
looks good for the runner changes. not as sure about dependabot :)
.github/workflows/build.yml
Outdated
@@ -109,7 +92,7 @@ jobs: | |||
- name: Setup cmake | |||
uses: jwlawson/actions-setup-cmake@v2 | |||
with: | |||
cmake-version: '3.30.x' | |||
cmake-version: "3.30.x" |
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.
In yaml double quotes allow escapes and other things, so I tend to prefer single quotes unless I need that, as, fewer surprises. Any reason you changed this?
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.
auto-formatting ... maybe i need to make prettier or whoever it was less agressive :).
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.
Okay sg. Maybe change back before merge, but not a big deal.
.github/dependabot.yaml
Outdated
@@ -0,0 +1,14 @@ | |||
# Please see the documentation for all configuration options: | |||
# https://help.github.com/github/administering-a-repository/configuration-options-for-dependency-updates |
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.
This link doesn't seem to work. it just goes to a generic support page
.github/dependabot.yaml
Outdated
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.
the dependabot-quickstart-guide doesn't recommend setting up manually like this, but rather suggests going into settings and letting github create this file. Wondering if we should just do it that way?
.github/dependabot.yaml
Outdated
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'm okay trying this out, but also a bit skeptical of the utility here. As far as I can tell it only runs cargo update
. Generally with how we've set up our Cargo.toml
this will only do minor updates (i.e. it'll never update arrow 53->54). Maybe that's good, but we need a process to also run cargo outdated
and try and stay on top of more major updates as well, so I'm not sure how much time this saves us.
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.
LGTM long as all the caching still works :)
Co-authored-by: Zach Schuermann <[email protected]>
actions now show ... Restoring cache ...
Cache hit for: v0-rust-test-Linux-x64-f47faad6-dc67ccb0
Received 209715200 of 500222278 (41.9%), 199.4 MBs/sec
Received 496027974 of 500222278 (99.2%), 235.8 MBs/sec
Received 500222278 of 500222278 (100.0%), 229.7 MBs/sec
Cache Size: ~477 MB (500222278 B) so i am reasonably confident the caching still works. |
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!
.github/workflows/build.yml
Outdated
@@ -109,7 +92,7 @@ jobs: | |||
- name: Setup cmake | |||
uses: jwlawson/actions-setup-cmake@v2 | |||
with: | |||
cmake-version: '3.30.x' | |||
cmake-version: "3.30.x" |
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.
Okay sg. Maybe change back before merge, but not a big deal.
What changes are proposed in this pull request?
The
actions-rs/toolchain
action is deprecated in favor ofactions-rust-lang/setup-rust-toolchain
. This PR updates the usages of the respective actions in the github workflows. THe new action already includes an integration with the rust-cache action, so no need to set that up separately anymore.This also sets up a dependabot configuration for
cargo
andgithub-actions
which we may or may not choose to keep.How was this change tested?
no code changes.