Add Cargo.lock
#14483
Add Cargo.lock
#14483
Conversation
github-actions
bot
added
documentation
There was a problem hiding this comment.
Thank you @mbrobbel -- I think this is quite nice and a significant improvement over the current status. It is also quite nice to be able to do cargo run -p datafusion-cli
I think we can avoid checking in dev/depcheck/Cargo.lock as depcheck is a test / CI tool (and we didn't check in Cargo.lock for proto/gen for exampele:
As this has potential to impact a bunch of workflows I think we should leave this open for a few days to let people have a chance to respond, but all in all I like it a lot ❤️
.github/workflows/extended.yml
Outdated
| @@ -87,7 +87,7 @@ jobs: | |||
| - name: Run tests | |||
| run: | | |||
| cd datafusion | |||
| cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests | |||
| cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked | |||
There was a problem hiding this comment.
cargo test shouldn't need the --locked flag (here & other places)
it would suffice to verify cargo lock updatodateness once in single job
There was a problem hiding this comment.
Yes, I added it everywhere to let all these jobs fail early when the lock file needs to be updated (to save CI resources), because in that case all those jobs are going to have to run again when the lock file is updated. But I'm also fine with only keeping it for one check.
There was a problem hiding this comment.
Fail-fast is not unreasonable, i didn't think about this.
otoh one could say same thing about formatting issues. there is definitely tradeoff between conserving CI resources and maxing our single PR feedback signal.
let's put cargo.lock consistency in the "formatting bucket", ie reuse previous design decision for now. i am open to revisiting it.
There was a problem hiding this comment.
Ok, I'll just keep it for
datafusion/.github/workflows/extended.yml
Line 37 in 684c7df
Edit: I meant to link:
datafusion/.github/workflows/rust.yml
Line 51 in 684c7df
There was a problem hiding this comment.
thanks!
can you please add a code comment there why --locked is useful?
|
@mbrobbel many changes in this PR are about adding If, however, it's necessary to have |
|
I will leave this open for another day or so for additional comments and then plan to merge |
| @@ -60,7 +60,10 @@ jobs: | |||
| with: | |||
| rust-version: stable | |||
There was a problem hiding this comment.
this is a nice idea to only use --locked in one test 👍
|
I merged up from main to resolve a conflict and applied the suggestion. I plan to merge this once we get a clean CI run |
|
🚀 let's give it a try |
Which issue does this PR close?
Rationale for this change
See linked issue.
What changes are included in this PR?
Cargo.lockfrom.gitignore--lockedto cargo in CIdatafusion-cliback to workspaceclilock file check in CICargo.lockfile fordev/depcheckcargo updatefrom release instructionscargo updatecliDockerfileAre these changes tested?
CI.
Are there any user-facing changes?
Added
Cargo.lockfile.