Highfive rotation

1

Niko is the only person on the highfive review list for a large percentage of the rust-lang/rust repository. I wanted to open up a conversation about possibly finding more people to cover the repository. I don't think there is a major problem right now, but I think it might make things run more smoothly if we were clearer about who is responsible for different parts of the repo.

AFAIK, currently Niko is the only (automatic) reviewer for:

  • Everything in the root directory (Cargo.lock, Cargo.toml, etc.)
  • src/bootstrap
  • src/build_helper
  • src/etc
  • src/libpanic_abort
  • src/libpanic_unwind
  • src/libproc_macro
  • src/libprofiler_builtins
  • src/libunwind
  • src/llvm-project
  • src/rtstartup
  • src/rustc
  • src/rustllvm
  • src/stage0.txt
  • src/stdarch
  • src/tools/ — a bunch of things here, including:
  • src/tools/cargo
  • src/tools/clippy
  • src/tools/compiletest
  • src/tools/miri
  • src/tools/rls
  • src/tools/rust-installer
  • src/tools/rustbook
  • src/tools/rustfmt
  • src/tools/tidy

I personally would like to know who should review things for bootstrap, tidy, lockfile updates, and license changes since I need to modify them occasionally. I sorta know who, but I don't want to bug the wrong people, or have people approve things that they probably shouldn't.

A problem I'd like to discuss is the feeling of being saddled with responsibility if one is added to the highfive list. I am already over-subscribed, so I'm reluctant to add myself to the rotation because I'm already ignoring too many things now.

One rough idea I had was to make an option in highfive to have a time-limited signup. Like I could sign up for a directory for anywhere from 1 month to a year, and then it could automatically remove me. That might help with the feeling of getting "stuck" with something. But I'm not sure if that would actually be beneficial.

Maybe others have ideas on how to make this process better? Are there people or groups that can be added to the directories listed above?

8 Likes
2

I can take this.

Many entries in the list are low-volume, if they don't have an owner it may make sense to assign them to people from triage team who will then ping people who they think are appropriate for reviewing and reassign to them.
That should provide better reaction times, ~day instead of ~week.

1 Like
3

Can I take this?

Fairly low volume and fits with me

4

I would be happy to review changes to root-directory files and license files. And since I'm on the cargo team, I'd also be happy to be on highfive rotation for src/tools/cargo.

5

I can help with src/llvm-project and src/rustllvm, at least.

6

I can take:

  • Everything in the root directory (Cargo.lock, Cargo.toml, etc.)
  • src/bootstrap
  • src/build_helper
  • src/etc
  • src/stage0.txt
  • src/tools/compiletest
  • src/tools/rust-installer (though I think it's a submodule?)
  • src/tools/tidy

These should be libs under current rules I believe:

  • src/libpanic_abort
  • src/libpanic_unwind
  • src/stdarch (note, I believe this is a submodule)
7

As a result submodule update PRs are assigned to nikomatsakis, see e.g. submodules: update clippy from b91ae16e to 2855b214 by matthiaskrgr · Pull Request #69278 · rust-lang/rust · GitHub :slight_smile:

8

Yes, my point is that submodule updates usually need less scrutiny (presuming they're not changing public details of our implementation in a way that needs e.g. lang signoff).

9

Perhaps something that could relieve the stress would be assigning more than one person to a PR, and they can agree between themselves who has the most bandwidth to review the PR... i.e. for each new PR, reviewers A and B would be assigned by highfive-bot and only one of the needs to review it.

A modest extension of this would be to assign the PR to a team with the expectation that somebody on the team reviews it.

Of course, I'm not on the highfive rotation at all, so maybe an actual reviewer might feel differently...

10

My worry with this is that then both of them would assume the other will review the PR.

5 Likes
11

(and lang, in particular for the last one)

I've done / reviewed some things here, so I could be added to this.

12

highfive's assignment of one person to a PR is excellent for avoiding this.

That said, it'd be nice if there were a way to re-invoke highfive, to pull in an additional reviewer if something feels complex.

13

Yeah, I thought that might be the case. An alternate might be to have two reviewers and make them both mandatory so that they "egg each other on" in some sense... but I'm not sure that would help with the original problem of stress...

14

There's an issue:

15

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.