Separate weekly cargo update PRs and add boostrap

[tgross35]

Previously we only had one workspace that covered library, rustbook, and compiler. These have since been split off to their own workspaces. It seems to make sense to also give them separate update pull requests since they have different update requirements.

Create a bootstrap update pull request now that it is easy to add new ones.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tgross35]

I just noticed that bootstrap is one of the lockfiles that doesn't get any automatic updates. Not sure if this is desired or not, but the change is easy enough that I just put a PR up.

r? @Kobzol

[Kobzol]

I also noticed that, but I kind of assumed that it is on purpose 🤔 I recently tried to update the lockfile and sadly it increased the number of bootstrap dependencies slightly I think. And for bootstrap we want to have the dep. count be as small as possible.

CC @onur-ozkan What do you think, should we update it automatically?

[tgross35]

Crosslinking, I put up a cargo update PR to see the current state and the diff wasn't too big #130951

[onur-ozkan]

There are two concerns: breaking the build system and keeping the dependency stack as small as possible. We pinned the versions of the risky dependencies that could break the build system, so automatic updates shouldn’t be an issue there. But when it comes to keeping the dependency stack small, automatic updates don’t play well on that.

[tgross35]

We could also have it do a separate PR so it's at least available but not tied to the others. That actually might be nicer for the library lockfile too.

[onur-ozkan]

We could also have it do a separate PR so it's at least available but not tied to the others. That actually might be nicer for the library lockfile too.

Sounds very good!

[tgross35]

Alright, I think I have something that will do a separate PR for each lockfile, and then added bootstrap to that. The script works well, but is there any good way to validate the GHA portion? (Specifically, if: github.repository_owner == 'rust-lang' && !contains(fromJSON(needs.check_bors_status.outputs.skip_keys), matrix.name) seems like just the sort of thing that sounds like it should work based on the docs but could fail in real life)

[Kobzol]

No many other ways to test than just run it, I suppose. There's https://github.com/nektos/act, but this seems to complex for it to work.

[Kobzol]

I tried to run the script locally and it gave me this error:

+ jq -n --arg path . --arg lockfile_path ./Cargo.lock --rawfile lockfile ./Cargo.lock --rawfile log cargo_update.log '{ $path, $lockfile_path, $lockfile, $log }'
+ jq -n --arg name root --slurpfile output update_output.json --slurpfile value single_update_output.json '$output[0] + { $name: $value[0] }'
jq: error: syntax error, unexpected ':', expecting '}' (Unix shell quoting issues?) at <top-level>, line 1:
$output[0] + { $name: $value[0] }                    
jq: error: May need parentheses around object key expression at <top-level>, line 1:
$output[0] + { $name: $value[0] }               
jq: 2 compile errors

I ran it from the root as ./src/ci/scripts/update-all-lockfiles.sh. Does that work for you?

[Kobzol]

It would be great to write all new script logic in the repo either in Rust (once we get Cargo scripts) or at least Python, the bash magic is very opaque IMO. On the other hand, it's difficult to beat jq for these simple JSON transformations. Could you please at least add some comments on top of the jq invocations to explain what are they doing? Thanks!

If it was in Python, we could do some nicer stuff though, e.g. check how many dependencies will be compiled after the update, which would be useful especially for bootstrap. Otherwise with every update, we will probably manually go and check if the dep count wasn't regressed.

[tgross35]

Oh I had no clue python was fine for this sort of thing! That makes it easier, I struggled through jq. The first command just builds a dictionary for that single loop iteration, then the second command appends it to create this structure:

{
  "root": {
    "path": ".",
    "lockfile_path": "./Cargo.lock",
    "lockfile":  "entire lockfile ...",
    "log": "output capture"
  },
  "library": {
    "path": "library",
    "lockfile_path": "library/Cargo.lock",
    "lockfile":  "entire lockfile ...",
    "log": "output capture"
  },
  "rustbook": {
    "path": "src/tools/rustbook",
    "lockfile_path": "src/tools/rustbook/Cargo.lock"
    "lockfile":  "entire lockfile ...",
    "log": "output capture"
  },
  "bootstrap": {
    "path": "src/bootstrap",
    "lockfile_path": "src/bootstrap/Cargo.lock",
    "lockfile":  "entire lockfile ...",
    "log": "output capture"
  }
}

But I'll rewrite it in python since this is apparently problematic anyway.

[tgross35]

If it was in Python, we could do some nicer stuff though, e.g. check how many dependencies will be compiled after the update, which would be useful especially for bootstrap. Otherwise with every update, we will probably manually go and check if the dep count wasn't regressed.

For this, do you just mean adding a note to the PR with the total dependencies in the lockfile before and after the update?

[Kobzol]

Yeah, it would be nice to see that before/after count in the update log (that is then posted to the PR). However, I'm not sure how easy it is to find out (e.g. through cargo metadata?). If we had to literally start compilation and examine the stderr, then it might be a bit too annoying.

[tgross35]

Wouldn't it just be enough to check the number of [[package]] entries in the lockfile? If we just want a rough idea of whether there are added or removed dependencies (python 3.11+ has tomllib so even better yay)

[Kobzol]

That sounds like a reasonable heuristic indeed.

[bors]

☔ The latest upstream changes (presumably #131009) made this pull request unmergeable. Please resolve the merge conflicts.

[tgross35]

I ran it from the root as ./src/ci/scripts/update-all-lockfiles.sh. Does that work for you?

Huh, I did test it a bunch of times and it does work for me. I wonder if it is a version thing, my jq is 1.7.1 and bash is 3.2.57 (MacOS ancient version...). Anyway I'll just move this to python.

[tgross35]

Finally got back to this, I did the rewrite and moved a few more tasks to the script to keep some more complexity out of the yaml file. Sample commit message:

library: run `cargo update`

Dependency count before update: 52
Dependency count after update: 52

Log:

```text
library dependencies:
     Locking 5 packages to latest compatible versions
    Updating cc v1.1.22 -> v1.1.31
    Updating compiler_builtins v0.1.134 -> v0.1.135
    Updating dlmalloc v0.2.6 -> v0.2.7
    Updating object v0.36.4 -> v0.36.5
    Updating windows-sys v0.52.0 -> v0.59.0
note: pass `--verbose` to see 5 unchanged dependencies behind latest
```

And the PR body:

Automation to keep dependencies in `Cargo.lock` current.

Dependency count before update: 52
Dependency count after update: 52

The following is the output from `cargo update` in `library`:

```text
library dependencies:
     Locking 5 packages to latest compatible versions
    Updating cc v1.1.22 -> v1.1.31
    Updating compiler_builtins v0.1.134 -> v0.1.135
    Updating dlmalloc v0.2.6 -> v0.2.7
    Updating object v0.36.4 -> v0.36.5
    Updating windows-sys v0.52.0 -> v0.59.0
note: pass `--verbose` to see 5 unchanged dependencies behind latest
```

I think this is ready for another look when you get the chance.

[Kobzol]

Nit: I would remove the leading dashes, because by CLI convention, that usually means that it is optional and it can be combined with other flags. But here we treat it as an exclusive subcommand, e.g. python3 update-all-lockfiles.py run-update. In other words, it is not a CLI option, but a CLI subcommand.

Also USAGE should be updated in that case.

[tgross35]

Good point, I removed the leading -- from all of the options.

[Kobzol]

I'm moderately sure that r-l/rust is configured in a way that only cargo_update can be pushed to, and creation of this branch might be restricted. We'd need to check what happens and update the branch protections accordingly.

[Kobzol]

@Mark-Simulacrum Could you please make the branch protection (i.e. branch creation) work for cargo-update-*?

[Kobzol]

tomllib is only available from Python 3.11, but the default version of Python used on our CI (in ubuntu-latest) is Python 3.10. So we'd need to update the Python version, install the library or do it in a different way.

[tgross35]

Since all we need is a total number of, counting [[package]] works here. Updated to drop the dependency.

[Kobzol]

It looks like it might take some time until Renovatebot is ready to work on the rust-lang/rust repository. Until then, making this action implemented in Python makes it easier to maintain, IMO, although it all gets a bit complicated. Left some comments, otherwise the logic looks fine to me.

Maybe it could be simplified a bit by using a CI matrix in the workflow file, where the YAML file would tell the Python script which lockfile should be updated, to avoid the update-all -> restore dance. But I wouldn't change the existing logic, since hopefully this will get replaced by Renovatebot one day..

[tgross35]

Maybe it could be simplified a bit by using a CI matrix in the workflow file, where the YAML file would tell the Python script which lockfile should be updated, to avoid the update-all -> restore dance. But I wouldn't change the existing logic, since hopefully this will get replaced by Renovatebot one day..

Actually, is it possible to exit a workflow job early? I could have the matrix job first check if a PR exists for that branch, then try the update, and just skip the rest of the job if the PR exists or if the update does nothing. I think this would make things much simpler, eliminating the JSON serialization that is shared between steps.

I guess this could probably be implemented by writing a SKIP environment variable and checking it in the if of each following workflow step?

[Kobzol]

It is not possible to exit early without literally cancelling the whole workflow. But I don't think that it's needed, as you said, you can figure out whether the PR should be created or not, and then simply "do nothing" in followup steps if not, using e.g. a bash condition or an if step property that depends on the previous step result.

Check the "step outputs" suggestion here.