Backpropagate Cargo.lock updates to all lock files

[ryoqun]

Problem

Non-default Cargo lock files tend to be forgot to update. this causes many problems:

• CI build isn't deterministic because lock files are changed on the fly while ci is running; So, git diff on CI shows some difference.
• auditing cumulative Cargo lock update is very tedious or hard: 959c1ea#diff-7d7ecaa361c7585f6ee9b01378cd1393R2872
• back-porting sometimes incurs noise of unrelated previous cargo lock updates, which is again tedious to audit: d210727#diff-d2ede298d9a75c849170d6ef285eda1eR6578
• dependabot sometimes fails because it can't update non-default Cargo lock files

This is a problem which I noticed at #8882. Since then, I've been always bothered with our trust-chain in the build/CI system a bit... Let's fortify it step by step before we get pray of third-party crate publisher's credential hack :)

Summary of Changes

• never accept stale lock files
• for that, give special handling to dependabot

live example: https://github.com/solana-labs/solana/pull/9159/commits

partly related to this: #8587 (comment)

upstream (dependabot)'s related issue: https://github.com/dependabot/feedback/issues/5

[mvines]

This seems really complex :-/

Maybe unchecking this box will help?

Also isn't there a new Cargo.lock format available now that we could enable that'll reduce the backport conflicts?

[ryoqun]

This seems really complex :-/

Yeah, but I researched this a bit and no better solution came up for weeks while pondering when waiting builds...

Maybe unchecking this box will help?

I think the top-level here means the crates specified directly by Cargo.toml. So, non-top-level packages is the so-called transitively-dependant packages. So, this isn't what we want. Unchecking that would produce tons of tons dependabot PRs because plain cargo update produces so large diff locally already. Also, that would increase frequency of PRs even after we got over the initial surge.

For the dependabot fiddling path, I could register each Cargo.{toml,lock}s to dependabot but it'd cause separate PRs just for the same crate update until they support grouping of update.

Also isn't there a new Cargo.lock format available now that we could enable that'll reduce the backport conflicts?

Yeah, that's true. But that doesn't solve all problems I'd like to fix as described above. Can do as a separate PR for the format change.

[mvines]

Ok so I think my main concern about this PR is that how it adds a ton of stuff into ci/docker-run.sh and ci/test-checks.sh, where instead I think ideally this is all very isolated in it's own ci/dependabot-pr.sh-like file.

I think the ideal end state is a new build step in https://github.com/solana-labs/solana/blob/master/ci/buildkite.yml, that:
a. runs ci/dependabot-pr.sh before checks, with a "wait" directive,
b. but only runs for dependabot PRs

To get there will be a multi-step process:

1. We aren't surfacing all the right build environment variables to buildkite from ci-gate in order to implement 1. https://github.com/mvines/ci-gate/blob/51d99887ea7cf3dd1d030a55cc6980f1fc8d09c7/index.js#L121-L127 is missing the "author" information (cc: https://buildkite.com/docs/apis/rest-api/builds#create-a-build) in particular, so all builds appear to be coming from me 🙃.
2. Buildkite has support for build step conditionals, https://buildkite.com/docs/pipelines/conditionals, that will allow us to filter out this special build step from non-dependabot PRs (build.env() in particular)

I can look at adding (1) if this approach sounds good to you.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale pull request has been automatically closed. Thank you for your contributions.

[ryoqun]

Ok so I think my main concern about this PR is that how it adds a ton of stuff into ci/docker-run.sh and ci/test-checks.sh, where instead I think ideally this is all very isolated in it's own ci/dependabot-pr.sh-like file.

I think the ideal end state is a new build step in https://github.com/solana-labs/solana/blob/master/ci/buildkite.yml, that:
a. runs ci/dependabot-pr.sh before checks, with a "wait" directive,
b. but only runs for dependabot PRs

To get there will be a multi-step process:

1. We aren't surfacing all the right build environment variables to buildkite from ci-gate in order to implement 1.    https://github.com/mvines/ci-gate/blob/51d99887ea7cf3dd1d030a55cc6980f1fc8d09c7/index.js#L121-L127 is missing the "author" information (cc: https://buildkite.com/docs/apis/rest-api/builds#create-a-build) in particular, so all builds appear to be coming from me upside_down_face.

2. Buildkite has support for build step conditionals, https://buildkite.com/docs/pipelines/conditionals, that will allow us to filter out this special build step from non-dependabot PRs (`build.env()` in particular)

I can look at adding (1) if this approach sounds good to you.

Yeah, that looks great way to accomplish what I want to achieve. :)

[codecov]

Codecov Report

Merging #9180 into master will decrease coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #9180     +/-   ##
========================================
- Coverage    80.6%   80.6%   -0.1%     
========================================
  Files         279     279             
  Lines       63375   63375             
========================================
- Hits        51110   51105      -5     
- Misses      12265   12270      +5     

[ryoqun]

@mvines I think this pr is ready for review again!

I'm playing with this dependabot's PR this time: https://github.com/solana-labs/solana/pull/9508/commits

test builds:

this pr (for normal users): https://buildkite.com/solana-labs/solana/builds/22845
test subject pr build take 1 (it back-propagates updates to all locks): https://buildkite.com/solana-labs/solana/builds/22842
test subject pr build take 2 (--locked build and usual test runs with success): https://buildkite.com/solana-labs/solana/builds/22843

[ryoqun]

This is what I really want to do before any our depending crates are compromised. :)

[mvines]

Using _TARGET_LOCK_FILES to pass a list into scripts/cargo-for-all-lock-files.sh feels a little too sneaky to me. It would be nice to fit this on the scripts/cargo-for-all-lock-files.sh command line.

$ scripts/cargo-for-all-lock-files.sh $(_TARGET_LOCK_FILES) -- update $parsed_update_args perhaps? And if there's no -- argument then we use $(git ls-files :**/Cargo.lock)

[ryoqun]

Yeah, I've been concerned a bit too... I followed an easier path considering these script's intended usage frequency. Thanks for accurately pin-pointing this while reviewing! I've changed my mind and give this some love. :)

[ryoqun]

@mvines I think I've fixed all your suggestions! Also, I've updated my experiment pr: #9508 Off course, both builds are passing. :)

[ryoqun]

I've rebased all outstading dependabot's PRs on this after the merge.Now, all those prs seem to be working: #9446 #9607.

[ryoqun]

I'll wait a week or so before I back-port this to all version branches (It should be almost no effort).

Also, I'll do quick-post to the upstream's dependabot/feedback#5 to share this for the rust+dependabot community.