The necessity for reviews to wait until approval (and not assume silence is consent) has been the norm in the LLVM project historically, and is documented in LLVM Code-Review Policy and Practices — LLVM 19.0.0git documentation "Specifically, once a patch is sent out for review, it needs an explicit approval before it is committed. "
Even in the days of Phabricator there was some confusion around this, especially with the libcxx folks having more robust precommit checking, developers might create a phab review only for the precommit checking - not expecting to wait for approval. But this could set the wrong expectations for other developers about the norms of the project or why some reviews seemed to be committed without approval, and others not. In phab, the “draft” mode could be used to create a personal phab review that wasn’t published to the community - allowing access to the precommit checks without creating/increasing this confusion.
I think it’s perfectly fine to open a PR for the sole purpose of making use of CI checks (or other automation). We allow people to directly commit to the repo without pre-commit review using their own judgement – it seems nonsensical to forbid them from doing the same using a PR.
In such cases, it is usually a good idea to explicitly mention that no review is expected in the PR description, so reviewers don’t waste time on it. However, for short-term revert PRs, it is generally understood that this is the case even without explicit notice.
I don’t think that anything specific needs to be done about this – apart from you refraining from posting these unnecessary comments in the future.
Hmm, that’s helpful - do you know if that means they don’t send email notifications? That’d help at least me - seeing emails about PRs that get created and committed without approval tends to lead me to go and investigate why (& also just having more email, especially emali that’s not meant to elicit actions by other people seems bad/adds to the email noise on the project).
& can they be committed from draft mode? At least for the phab drafts the UI awkwardness was localized to the person using it (yeah, I know I’m committing something that says “draft” but no one else can see that/be confused by it) but with that not being the case in github, it might not be such a good tool here.
I’m reasonably certain that the pr-subscriber job doesn’t run on draft PRs. So you won’t get email when someone creates a draft PR. But you can’t directly commit a draft PR, so when you go to commit it, you have to make it public, which ends up running the PR subscribers job, which pings the relevant teams based on the labels.
(At least that’s how I’m reasonably certain it would go, haven’t done any explit testing of this).
We allow people to directly commit to the repo without pre-commit review using their own judgement – it seems nonsensical to forbid them from doing the same using a PR.
If there’s a clear distinction between a PR created for review and one for CI only, I’m OK with it - though the extra email/notifications seem unfortunate. Maybe there’s a way to use a tag on the PR that’d ensure it doesn’t trigger any of the tag-based email notifications?
& if we had such a solution, I think it’d be appropriate to apply it to revert PRs too, for consistency (& if possible, the notification reductions). @akorobeynikov did you setup/know who setup the email notifications for the pull request tags? Is there a way we could have a tag that says “don’t send any notifications”?
I’m not 100% confident, but if you click the ‘draft’ button when creating a PR, it won’t add reviewers and send notifications. But I think the bot that tags subscribers might trigger that?
I concur that we should not have policy which permits unreviewed direct commits, but prohibits PR-based unreviewed commits. (IMO, it would probably be good to eventually move towards encouraging every commit to go through a PR, even ones that don’t require pre-commit review.)
Given that our current policy text does seem to prohibit this from a plain reading, we need to modify that policy.
I’m not sure it’s a downside that people are notified for such PRs? If someone does happen to have review comments before it’s merged, that’s useful to know, even if the author doesn’t need to wait for such a review to occur.
@jyknight are the motivations of the current policy (to avoid silence being consent when a change goes unreviewed and is then committed) of interest to you? Do you want to preserve that distinction through such a policy change?
I’d like it to be clear which things were sent with an expectation of review, and which weren’t - today, but certainly moreso in a future where everything is sent as a PR.
And in that future I think it’d be even more important to figure out the email notifications, so folks can prioritize responding in a timely manner to folks waiting for reviews before they move forward with a change.
Yes, we should preserve that. A person should only commit without pre review when it’s appropriate. Sadly, how to determine appropriateness is not very clear, but “I originally thought this change should be reviewed, but when nobody approved it, I now think it shouldn’t be reviewed after all” is definitely a bad rationale – but QUITE tempting. We should continue to ban that
Also, IMO we ought to – with appropriate community discussion and consensus – move towards considering fewer changes as appropriate for unreviewed commit. I’d love to get to a world where really only completely trivial changes and reverts of recent commits are permitted under that policy.
But for now, what I’m asking for is just a minor tweak: to acknowledge that creating a PR is not necessarily an intent to request pre-commit review. And thus it’s not a-priori inappropriate to create a PR and then immediately merge it without review.
I don’t quite get the confusion here, we’re allowed to push without pre-merge review, isn’t this the same theoretical confusion as well?
Also that mean somehow some new developer would see my PR that we up for a couple of hours for the sake of benefiting from CI checks? Like they would actively monitor PRs in the project and somehow make inference about what they can do as well? By this account they will see my commits pushed without PRs and do the same as well…
We should also dissect the “Specifically, once a patch is sent out for review, it needs an explicit approval before it is committed”: it was introduced in 2013 so that folks sending a patch to the mailing list wouldn’t just get into despair when they don’t get a review in multiple weeks and land it.
I will argue that when I open a PR for the purpose of running the CI, or for reverting a change, I am not sending a patch for review: I am using the infrastructure to merge a change. I claim that the developer policy does not intend to prevent this.
(edit: I typed this earlier today, and since then @jyknight argued basically in the same direction)
Also the best part from migrating to pull-request to me (as a bot maintainer) so far: I can revert a PR in 3 clicks on GitHub without dropping to the console (click revert on the original PR, which opens a new one, and then click merge on the new PR). This also nicely link the revert in the original PR without other action, and GitHub also propose to add the original author as reviewer which notifies them of the revert.
That implies a PR that isn’t reviewed before merge.
I had several similar situations before. I sent PR and no comment came in. And I am the maintainer of the area and I think it is necessary to land that in a specific period. Then I’ll comment as “I’ll land it in 1/2/4 weeks in no comments come in” then land it after that. I don’t do this in area I am not maintaining. I feel it makes sense to me and it runs well. How do you think about it?
The part that is the most unexpected to me is where you “think it is necessary to land that in a specific period”, where is this necessity coming from? Do you have example of such a PR? I would like to understand better when is the LLVM project in “need of landing something within some period”? I could see this happening when there are items intended to be done before a release (like if we know we have regressions from the previous release for example), but such cases are solved (I believe) through raising attention to the release managers, flagging release blockers, and escalating on Discourse as necessary.
This series patch is necessary for clang to support P1689 format raised by CMake guys. CMake needs support from the clang side to support C++20 modules for clang. It is pretty important for C++20 Modules. There are people requiring it (Land C++ modules changes required for CMake before LLVM 16 branch?). And if we look back today, it is a correct decision. It helped clang to support C++20 modules experimentally in 3.26 and claims the experiment is over in 3.28. And it got a lot appreciations from the users. I think this is a pretty good example about necessity.
I’m not convinced: did you post on Discourse (or cfe-dev@ if it was still active at that time) about the plan for this feature? Call for reviewers or collaboration? What did you do to raise awareness on the problem of getting timely reviews there?
Reviewers time is and has always been a problem, it’s not clear to me that the obvious conclusion is that the project is better off just landing code like that because of potential impact.
If this is critical for downstream users in cmake or other cares about this, maybe they could get involved in the community and help find reviewer time?
I have a hard time believing that impactful and important features can’t find a reviewer when we raise awareness on this.
What did you do to raise awareness on the problem of getting timely reviews there?
I pinned several times in the review series. I also called for review in the private meeting between modules developers.
it’s not clear to me that the obvious conclusion is that the project is better off just landing code like that because of potential impact.
I think the project becomes better due to such changes. I’ve seen multiple users saying the support for modules since clang15 is becoming better and better. I wouldn’t argue if this doesn’t mean the project getting better.
If this is critical for downstream users in cmake or other cares about this, maybe they could get involved in the community and help find reviewer time?
PR that reverts another PR: Totally fine, and this is the easiest way to do it (using the GitHub web UI). I don’t mind getting notified a revert PR was created, and I think newer contributors would reasonably understand that reverting a commit that broke buildbots is a special case with respect to our usual reviews.
PR for a commit that would normally be committed directly, but the author wants to take advantage of pre-commit CI: Not a case I’d thought about before and I’d expect the PR summary makes it clear that’s the purpose. I don’t see a problem.
PR is opened but later committed as reviewers couldn’t be found: In general I think this shouldn’t happen. Seems to be what this thread is moving towards focusing on, but I don’t think that was really the case the thread was created to discuss?
This only works if there’s at least 2-3 experienced contributors that regularly review for any given area of LLVM. Yes, it shouldn’t happen that no reviewers can be found, just as there should be more regular contributors to C++ modules.
But reality can be far away from “should”, and before @ChuanqiXu took over, there was very little progress in this area for a very long time. As an outside observer, I really want to praise their work, working through some very involved topics across the ecosystem to make things work (e.g. interacting with SG15, itanium-cxx-abi, libcxx, etc.), writing RFCs, fixing issues, always involving potential stakeholders and patiently asking for feedback, and quickly reacting to it, if and when it ever arrives.
The question is though: How many weeks or months is the “correct” time to wait for review if you know the codebase and are confident that your patch is correct? In the case of C++ modules at least, “progressing under (predominantly) one person” has yielded results that are clearly to be preferred over “stagnating while waiting for a second reviewer[1]”.
which also has the added risk of eventually losing the primary contributor due to frustration. ↩︎
I wonder if one way of helping with the review situation is to have a discussion about what the expected standards for code review are.
For myself as a reviewer, I basically have two modes of review when it comes to complex changes:
A thorough review, digging in to truly understand the change to the point where I feel like I know everything it’s doing and why it’s the right thing to do.
A more superficial review that may skip on trying to understand e.g. some algorithmic subtleties, in favor of focusing on whether the change integrates with its surroundings in a reasonable way.
Obviously, the second mode is far less time consuming, but perhaps this level of review would be sufficient in cases like the C++ modules work that is being discussed here, and making that clear could help encourage reviewers.
As a related data point, the Linux kernel and related communities have established a protocol for making a vaguely similar distinction: there are Reviewed-by tags and Acked-by tags.
