Skip to content
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

Separate weekly cargo update PRs and add boostrap #130937

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Sep 27, 2024

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
Copy link
Collaborator

rustbot commented Sep 27, 2024

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2024
@tgross35
Copy link
Contributor Author

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

@rustbot rustbot assigned Kobzol and unassigned Mark-Simulacrum Sep 27, 2024
@tgross35 tgross35 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 27, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Sep 27, 2024

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
Copy link
Contributor Author

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

@onur-ozkan
Copy link
Member

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
Copy link
Contributor Author

tgross35 commented Sep 27, 2024

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
Copy link
Member

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!

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 28, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Sep 28, 2024

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)

Previously there was only one main workspace; within the past few
months, it was split into three (root, library, and rustbook). These
workspaces have somewhat different requirements for what can be updated
and what may be problematic, and making one large PR for everything
doesn't suit this well.

Change the jobs to create separate pull requests for each relevant
update. This will also make it easier to create a distinct job for
bootstrap.
@tgross35 tgross35 changed the title Update src/bootstrap/Cargo.lock in the weekly job Separate weekly cargo update PRs and add boostrap Sep 29, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Sep 30, 2024

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.

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -0,0 +1,51 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a reasonable heuristic indeed.

@bors
Copy link
Contributor

bors commented Sep 30, 2024

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

@tgross35
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants