I have read this thread a few times and from what I see cfe-commits won’t get notifications tomorrow but may in the future?
I would really prefer to have this on the mailing list as well. I really don’t want to subscribe to all PRs that feels like a downgrade.
Apologies, I was away for most of August and I am still catching up. I guess I expected details to be a lot more elaborate and clear by this point and a lot of things don’t feel clear to me at all.
It will certainly continue to get commit messages, as those are sent by a different mechanism.
@tstellar I know this has come up before, the sticking point was that PR subscribers need to be github users, so we’d need bot/zombie/artificial/something users to be associated with cfe-commits, llvm-commits, whatever-commits is that correct? What happened with that?
Hope this helps. I will try to get the documentation ready for review as soon as possible.
I’m happy to help review!
We’re going to set up a webhook that sends emails to the commit lists.
Thank you – the commits lists are critical information for some folks, so keeping continuity there is greatly appreciated.
I have created a Pull Request for the documentation changes here: [Docs] Update documentation for the new GitHub workflow by tru · Pull Request #65162 · llvm/llvm-project · GitHub
Please feel free to comment on it, but also I think we should try to get something into the site ASAP, and we can discuss minor changes later. I just want to make sure we don’t have stale info on the docs site.
I see one very problematic issue.
Example: [clang-tidy] Adding an initial version of the "Initialized Class Members" checker. by adriannistor · Pull Request #65189 · llvm/llvm-project · GitHub
Group pr-subscribers-clang-tidy were removed from a review, once any member of that group commented. This will prevent other members of group to get notifications about that review.
Can this “replacement” be somehow disabled ?
Were the group members unsubscribed too or were they just removed as reviewers?
When one of group member review pull request, then “group team” is removed from a pull request.
I found some settings under team group, “Enable auto assignment” this may help but it’s still may be problematic if someone outside selected reviewers from a group will review, then it may remove entire group.
Found some discussion here: Ability to keep remaining team members as reviewers after one team member starts a review · community · Discussion #5289 · GitHub
@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.
