[libc++] Fix CODEOWNERS file for libc++ #65344
Conversation
ldionne
added
the
libc++
github-actions
bot
removed
the
libc++
cor3ntin
added
the
libc++
|
Anyone who wants to subscribe to a pull request using our current approach is going to have to be in the CODEOWNERS file, there's not really a way around that. In order to get to the state you want, we are probably going to need 2 teams, one for reviewers, and one for subscribers. This probably won't scale well if we need to do this throughout the whole project, but I don't think many other sub-projects have the same kind of structured code review process that would require this, so it might be OK. The only other solution, I can think of would be to try to experiment with the code review setings for the libcxx team. You could enable "Auto Assignment", which will assign individual reviewers from the team rather than the whole team itself. That would solve the problem of the confusting "On behalf of..." messages, I think. |
|
I think I don't understand what the purpose of the I think we have two "things" we're trying to achieve:
We need the ability to have two different sets of people for these two things. What do we use for (1)? Do we use automatic tagging of issues/PRs along with individual people setting a custom notification filter on the repository? Or do we use membership in a team like It seems to me like the CODEOWNERS file is meant to encode (2), not (1). If we use tags to achieve notifications, then I believe the only problem we have right now is that |
|
CODEOWNERS is encoding 1) and not 2) right now, it's not clear to me that label can be added on creation of the PR. The flow we have on GitHub issues adds a comment afterward to @teamname which doesn't yield an email with all the context we have on PR creation right now. |
I don't think that is correct, or at least it might be the intent but not really what's actually happening. For example, see #65279: 
It looks like the @llvm/pr-subscribers-libcxx group was added automatically as a reviewer of that PR. |
|
You're absolutely right. I just figured this out on my PR as well: it's not clear to me that there is a way to control this to match exactly our intent here. |
|
The reason we are using the CODEOWNERS file to achieve 1), is because it sends a notification with a full summary of the PR when it is first created. Any label based notification scheme (like we have for issues) is going to subscribe people to the issue after it is created, so they won't get the PR creation notification, but they will get a notification for all other updates to the PR. I wonder in general how many people want to subscribe to PRs, but don't want to be added as a reviewer? If this is a small group, we could do a hybrid approach where we use the CODEOWNERS file as it is intended to add reviewers and then do a label based notification system for people who only want to subscribe. Either way though, I think we are going to end up needing two groups, one for reviewers and one for subscribers. |
It's more about making sure that not just anyone can approve a patch on behalf of the library maintainers. It's useful to have a group representing "review authority" since it allows contributors to just submit patches and to know when they can be committed without having to know which individual reviews are required. Basically, whenever the code owner group is marked as "approved", the patch is good to go. Conflating that with the group used for notifications means that we can't attach any meaning to "being approved by the codeowner group" anymore, and that's what we want to avoid (at least for the runtimes, I don't want to speak for other projects). In that case, I think we want to create new teams: And then I will change the code-owner file for these three projects to point to these groups. At that point, however, I don't know what IMO it is more important to get this right than to be notified when the PR is created -- I might be wrong but I would think that being notified a few seconds after when the appropriate tag is added to the PR via Github Actions is enough? |
The motivation was to have a "good" notification, that is an email which is directly actionable and not "just a link", which led us to the CODE_OWNERS file. I'd note that LLVM as a project does not have concept of code owners required approval, commit access is technically enough to review and push code, and folks are supposed to apply best judgement with respect to their level of familiarity with parts of the codebase! I'm not sure there is another doc for the project than https://llvm.org/docs/CodeReview.html ? |
libc++/libc++abi/libunwind have had blocking reviewer group on Phabricator for a long time, since March 2020: |
|
The principal motivation seems to be "requires new committers to know what individuals to ping to get a review", and "make it easier for new committers to get involved (they don't need to fill the Reviewers: field anymore)", which is fairly common across the project: not clear to me that "blocking reviewer" is necessary over simply setting up "reviewer" though (Herald allows to setup "reviewers" without blocking anything, which is how I setup myself for most of the areas of MLIR where I am "code owner" (and I intend to continue to do so with CODE_OWNERS on GitHub to address exactly this problem of people who need to figure out who to ask a review from!) |
@llvm/pr-subscribers-{libcxx,libcxxabi,libunwind} should not be listed
as the code owner teams for these projects. These teams exist for
notification purposes, not to encode review authority.
ldionne
force-pushed
the
review/fix-code-owners-libcxx
branch
from
1d82d00
to
13342a8
Compare
|
@tstellar Could you create |
|
Hmm, I just saw your post here: https://discourse.llvm.org/t/changes-to-pull-request-subscription-system/73296/15. I am fine with abandoning this PR in favour of fixing the CODEOWNERS file as a whole, but I'd like to get this fixed sooner rather than later so we can be back in a state of parity with Phabricator for the runtimes. |
I think it's OK for the libcxx reviewers to use the file now, since it provide similar functionality to what you were using in Phabricator. I just don't want ad hoc updates to the file without a plan. |
|
Reopening. |
@llvm/pr-subscribers-libcxx should not be listed as the code owner team for libcxx, since that team is only meant to control PR notifications. In the current state of things, GitHub says that I approve PRs "on behalf of pr-subscribers-libcxx", which clearly makes no sense. For example #65265:
Note that we don't have similar reviewers teams for libcxxabi and libunwind, but we should make the same change for these projects once those GH teams have been created.
CC @MaskRay