-
Notifications
You must be signed in to change notification settings - Fork 127
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
Ignore Cargo.lock
files
#1044
Ignore Cargo.lock
files
#1044
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 6 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
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.
Strong ACK. This is much needed.
Ack. |
c38e28d
to
6c83ca3
Compare
rebasing to get this merged |
I don't think this is a good idea. Lockfiles exist for a reason, and if you read the link that @marathon-gary sent, you will see why.
|
Btw, we already ignore a few lockfiles which is not ideal. Regarding inter-workspace dependency management, I think we can stop using the version+path definition on |
I am aware of the link you shared but dont agree with you. I think its enough to have msrv check on each pr that makes sure the pr is always building on at least 1.75. Regrding your points: Anyway, my feeling is you just pouring here what you read in the link but in practice things are quite different. It would be good to try and show how this is failing rather than "it might fail".. |
Cargo.toml does not limit crates versions, it does specify the range that we want. Without a lock, it will perform dependency selection every time a fetch is run, leading to flaky CI. Also, crates can be yanked from registry. And, in practice, if you checkout
On 2) I agree with you. Lockfiles are not magical we still have to inspect dependencies before adding them to the project but to ensure that we always get the version that was inspected we need a lockfile. Don't get me wrong, I'm not saying we can't merge this. I'm just saying this is counterproductive. |
6c83ca3
to
f7a5b5a
Compare
I don't have strong opinion about this PR and I would prefer it being closed rather than trying to get it merged. It could be a good convince for devs and also for PR review process, but nothing crucial. |
Re-opening this as I am not able to rebase regularly on my local machine. |
I'm doing some research on this topic. Here's some findings:
|
This looks good to me. Basically library crates should NOT have a Thinking longer term, this now gets us back to the need to separate the library from binaries in the repo itself...the For now, @plebhash's suggestions make sense to me. |
add
Cargo.lock
files to.gitignore