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.
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).
@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.
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
I’m in favor of adjusting the plan from “make Phab readonly” to “disable the creation of new diffs”, I think that accomplishes the goal of directing folks to use the new system going forward. When the schedule was being discussed, it wasn’t clear if this was an option, and we planned assuming it wasn’t.
Would that plan work for you and Clang?
Thanks, that does seem like a real issue. To try to relate this to the bugzilla migration, in that case, we actually did a full migration of all the ongoing issues, and there was an implicit forward link from bugzilla to github via llvm.org/prNNN .
In the short term, if the plan to disable new diff uploads succeeds, comments will be open, and this won’t be an issue. We’ll need to set a new timeline for going readonly, and just to throw something out, I’d suggest February 2024 (4 months after October), but I haven’t gathered any consensus on that.
In the long term after comments are disabled, I do expect folks will want to do as you suggest: reopen long closed patches, and without comments, there won’t be a way to link forward from the Phab review to the PR, we can only link backwards from the PR to the Phab review. That’s not the best, but is that something you can live with? I think six months out, we’ll be living in the new world, and we’ll be out of the habit of commenting on Phabricator.
Let’s be sure to distinguish “diff uploads” (which I interpret as, uploading a new diff to an existing review) versus “creating new Differentials” i.e. creating new reviews in the Differential tool. We want to turn off the latter but not the former.
Yeah, if we’re able to find a way to allow uploading a new diff for an existing review, but not allow creating a new review, that would be fantastic.
If we can’t do that… I think we should plan to keep Phab up and running in its regular mode until we’ve drained enough of the existing queue, and hope the banner + peer pressure get folks to start using GitHub (though, to be honest, if we find a significant portion of the community stays on Phab at that point, I think it gives us some feedback we should take very seriously; hopefully we avoid that situation though).
Yup, and this is different in that we’re not migrating phab reviews (open or closed) to be GitHub PRs. This is one reason why the static instance of Phab is going to be crucial once we get to that point; losing old reviews would be extremely painful (we could still find most of the information in the emails generated from phab which are still archived, but things like suggested code changes don’t always make it into email form so we’d be losing data).
So long as we still allow uploading new diffs for existing reviews, that seems reasonable. I think Feb 2024 might be viable for Clang, but it’s hard to say (the automatic type inference patch has been ongoing for over a year already).
Perhaps that will be fine? It’s a bit hard to say, the reviews I see people revive are anywhere from this year to 5+ years ago; I’ve also seen people ask plenty of questions on old reviews, like “did you consider or is this a bug?” where it’s not trying to revive the patch but get more info from the people involved with it.
I think if someone wants to ask “did you consider” kind of questions, we can point folks to file an issue on GitHub and give it the question label, linking to the static review page.
Reviving an old review is a bit harder to solve that way; perhaps there’s something we could do on the static page though? e.g., maybe there’s a button that says “email everyone on this review?” and it does a mailto link to whatever email address is associated with the accounts that have subscribed to the thread? Something along those lines might help newer folks because it removes the “but HOW do I contact these people?” problem. Or it could be total overkill.
We used the same system with issues and it worked out OK. After the initial rush, there were only a few requests per month. However, the difference with issues was that we did not have team maintainers so the requests went to llvm-project admins instead. Maybe that would be better?