@PiotrZSL There is a difference between being a reviewer and being a subscriber. If a team is removed from the reviewer list, it’s possible all the team members are still subscribed to the PR and will receive updates. That’s what I’m trying to find out.
I’ll look at that auto-subscribe feature in any case to see if that will help.
The thing is that from a pull request view you do not see subscribers.
Maybe we need biger granulation on teams, like team: pr-reviewers-clang-tidy,
and then subscribers team would be just silently subscribed, and reviewers team would be assing to review, maybe even pr-owners-clang-tidy, in such case this load balancing mechanizm could be implemented in better way…
I would like to start pushing back on this already. There is basically no chance this deadline will work in practice for Clang. For example, I have several long-running patches under review in Phabricator, such as:
⚙ D140828 [C++] Implement "Deducing this" (P0847R7) (started Jan 1, 2023)
https://reviews.llvm.org/D133289 (started Sept 5, 2022)
(and others). These patches are still being actively worked on and reviewed, so it’s not stale work. I’m working as quickly as I can to drain my phabricator review queue, but between the ongoing 17.x release and upcoming conference talks/standards meetings, there’s no way Oct 1 is something I can meet. Further, it’s unreasonable to expect anyone to try to split these kinds of complex reviews across both Phab and GitHub, so I’m left wondering what can be done.
One possible solution could be to put Phab into a mode where new reviews cannot be initiated in Phab, but comments and uploading new revisions of existing reviews are still allowed, then leave phab up and hobbling until we’re ready to shut the service down in Oct 2024. I have no idea how feasible that is, though.
Tangentially, there’s a problem that’s become more obvious to me in the past week: resurrecting old patches is a thing people still regularly do:
https://reviews.llvm.org/D154716#4636803
https://reviews.llvm.org/D141192#4638188
https://reviews.llvm.org/D92733#4638319
https://reviews.llvm.org/D92790#4638635
(and that’s just the ones I noticed this week!) Putting Phab into read only mode makes that quite a bit harder – currently, our best hope of reaching authors is on the patch itself. Obviously, we cannot leave Phab up and running forever – so this is more a request for us to think about and document some best practices for how to revive old reviews. So far, the only thing I can come up with is to try to find them on Discourse and failing that, try to scrape their email address out of the review email that goes to the commits list to email them privately. That said, IMO, there’s no reason to presume anyone is reachable on Discourse or that contributors are going to be comfortable privately emailing strangers instead of asking on a public thread where others can chime in to help, so hopefully someone has some better ideas that are more new-contributor-friendly. Leaving Phab in a “comment only” mode could help in the short-term though.
I Agree with @AaronBallman. I think we should actively discourage/prevent if we can people from pushing new PRs on Phab, but there is no reason to move existing, active PRs to Github as we would lose all the history/comments so we’d basically have to start reviews from scratch which is not something we can afford, and doing so would provide little benefit.
There is a nice red banner now on Phab! Seems like we’re good in terms of folks who just don’t know.
We could also on just disable creating new revision entirely right now? And we wouldn’t be in a hurry for the read-only switch.
@MaskRay : what about a 403 on the URL to create a new revision? (the one used by arc as well, assuming it used a different entry point for creation and update!)
I agree and I made a similar suggestion, though I am not a decision maker.
There is substantial work to migrate existing patches to GitHub pull requests. Allowing comments is important for a smoother transition, otherwise people cannot even say: “hey, this is now available at https://github.com/llvm/llvm-project/pull/xxx, please move the discussion there” “a similar patch patch has landed and this can be closed (but we cannot because we are in a read-only mode)”. (And
sometimes it’s difficult to connect Phabricator handles with GitHub handles)
@mehdi_amini: We could also on just disable creating new revision entirely right now? And we wouldn’t be in a hurry for the read-only switch.
@MaskRay : what about a 403 on the URL to create a new revision? (the one used by arc as well, assuming it used a different entry point for creation and update!)
I am fine with disabling new revisions right now.
Some components like comments may be usable for a bit longer, but there is no service-level agreement.
I added a banner discouraging new differentials as that seemed straightforward. Disabling new differentials may need a bit more exploratory work (GitHub - phacility/phabricator: Effective June 1, 2021: Phabricator is no longer actively maintained.)…
diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php
index ecb12bb81d..4e397fee58 100644
--- a/src/view/page/PhabricatorStandardPageView.php
+++ b/src/view/page/PhabricatorStandardPageView.php
@@ -473,4 +473,14 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView
$developer_warning,
$header_chrome,
+
+ phutil_tag_div(
+ 'aphront-developer-error-callout',
+ array(
+ pht('Please use '),
+ phutil_tag('a', array('style' => 'color:#ccc', 'href' => 'https://github.com/llvm/llvm-project/pulls'), pht('GitHub pull requests')),
+ pht(' for new patches. '),
+ phutil_tag('a', array('style' => 'color:#ccc', 'href' => 'https://discourse.llvm.org/t/update-on-github-pull-requests/71540'), pht('Phabricator shutdown timeline')),
+ )),
+
phutil_tag(
'div',
BTW, with existing information I have, the read-only mode is changing this variable:
phab@llvm-reviews:/srv/http/phabricator$ grep read-only conf/local/local.json
"cluster.read-only": false,
⚓ T4571 Allow Phabricator to run in Read-Only Mode We will need to figure out how read-only it is:) If someone can take a look at the code and make a confirmation, that would be really useful.
I was thinking about handling it through URL rewrite in the Apache2 config actually, not touching Phab at all.
That’s why I mentioned
the one used by arc as well, assuming it used a different entry point for creation and update!
because we’d want people to be able to update revisions and comment, just not create new ones.
Just checked, for the web based UI, we could have Apache2 redirect Login
But arc is using the same API for creating and editing a revision, so this would require some changes to Phab code itself: ⇵ differential.revision.edit
Apparently flag is just implemented and used in a couple of places (18 usages). Very likely this feature is far from being complete.
first patch:
Most stuff is 80-90% broken right now. For example:
- Stuff like submitting comments doesn’t work, and gives you a confusing, unhelpful error.
- None of the UI really knows that it’s read-only. EditEngine stuff should all hide itself and say “you can’t add new comments while an install is in read-only mode”, for example, but currently does not.
This flag is used in few files responsible for cache:
If we’re leaving Phan alive enough to comment on old reviews, an open question is whether new community members are going to be able to do that. Because that would require leaving account registration open one way or another.
(Unfamiliar with conduit methods)
Edit src/applications/differential/conduit/DifferentialConduitAPIMethod.php ? Is anyone interested able to write a patch? 
edit: created Reviews.llvm.org read-only mode for the discussion.
Thank you for investigation! I created a new post Reviews.llvm.org read-only mode for the discussion.
I don’t think it’s necessarily a good idea to disable new patches on Phabricator just yet, even though most of them should be on GitHub now. The reason is that there are occasionally patch series where new patches are added/spun off etc into new patches, which are based on other patches that are still under review in Phabricator. As these patches won’t have equivalent git branches in somebody’s fork, a new PR cannot be based on top of an existing Phab review, so either they have to push their still in-review-on-Phab patches to a GitHub repo and go from there or they have to create a PR that also includes these commits too. Neither of these seem ideal.
I’ve seen at least one case where somebody hit this, and chose to create a new Phabricator patch, precisely to avoid these issues.
I think we can just do a soft reviewer-led rejection of new patches on Phabricator for now at least, i.e. if somebody asks for review on a new Phabricator patch, unless there is a good reason (such as the above) to review it there, the reviewer should ask the patch creator to create it in GitHub instead. I suspect there won’t be many people in this situation anyway, what with the bright red banner at the top of the screen.
I tried to subscribe to one of these new ‘pr-subscribers-xx’ teams, but the request is pending.
Aren’t we free to subscribe to whatever PR groups we are interested? If there’s an intention to use these teams as mandatory reviewers, then maybe we need a separated set of teams that can be freely subscribed by anyone.
Another question: what’s the process to decide on the maintainers of these teams? I didn’t see any discussion about this (but may have missed, ofc).
Thanks!
@nlopes Which teams have you tried to subscribe to? So far, the person who requested the team is being made the maintainer. I’m not sure there is a good way to make self-subscribe work for the teams, unfortunately.
For the debug-info team, Tom made me the maintainer because I was first to suggest it. I added the code owners as maintainers, which I think is the right thing to do for most teams. Code ownership might not be obvious in some other cases though.
For a pr-subscribers-xx maintainer (approver for the purpose of patch subscription, not related to authority), it seems that GitHub only presents the “pending request” information on the https://github.com/orgs/llvm/teams/pr-subscribers-* page, not on any other page (confirmed by @boomanaiden154-1)
I don’t think we can expect a maintainer to visit https://github.com/orgs/llvm/teams/pr-subscribers-* pages they manage to check new pending join requests routinely. So if they miss the email notification would like to join "LLVM"'s "pr-subscribers-llvm-binary-utilities" team, the request may stay pending forever.
The current best way is perhaps to have more maintainers for the pr-subscribers-xxx teams. I guess every maintainer will receive a would like to join "LLVM"'s "pr-subscribers-llvm-binary-utilities" team email notification (with the high volume of llvm-project emails, I think the error rate is not very low), but it’s probably better than neglecting an email notification. Unfortunately it seems that a maintainer cannot give maintainer state to another member. Perhaps only administrators can do this.
OK, that’s interesting, because I get email notifications for the issue-subscribers-* teams when someone wants to join. I’m’ not sure why it doesn’t do the same for the pr-subscribers-* teams.
As I requested many teams when the system was set up, I have gotten a lot of email notifications. To prevent from my missing “would like to join XXX” email notifications and to decrease my workload, consider changing the maintainer for the following teams
https://github.com/orgs/llvm/teams/pr-subscribers-llvm-lto/ @teresajohnson
https://github.com/orgs/llvm/teams/pr-subscribers-lld-coff/ @mstorsjo
https://github.com/orgs/llvm/teams/pr-subscribers-lld-macho/ @int3
https://github.com/orgs/llvm/teams/pr-subscribers-lld-wasm/ @sbc100
https://github.com/orgs/llvm/teams/pr-subscribers-sanitizer/ @fmayer
I have also invited some members which I think active to these teams, in case they do not know this notification system. Sorry if I failed to invite some folks.
Note that @MaskRay said “if they miss the email notification” not that the notification doesn’t happen. It does.
