[llvm-dev] [RFC] Deprecate email code reviews in favor of Phabricator
Krzysztof Parzyszek via llvm-dev
Statement:
Our current code review policy states[1]:
“Code reviews are conducted, in order of preference, on our web-based code-review tool (see Code Reviews with Phabricator), by email on the relevant project’s commit mailing list, on the project’s development list, or on the bug tracker.”
This proposal is to limit code reviews only to Phabricator. This would apply to all projects in the LLVM monorepo. With the change in effect, the amended policy would read:
“Code reviews are conducted on our web-based code-review tool (see Code Reviews with Phabricator).”
Current situation:
- In a recent llvm-dev thread[2], Christian Kühnel pointed out that pre-commit code reviews rarely originate via an email (most are started on Phabricator), although, as others pointed out, email responses to an ongoing review are not uncommon. (That thread also contains examples of mishaps related to the email-Phabricator interactions, or email handling itself.)
- I don’t have specific information about post-commit reviews. It seems like the most common form is an email reply to the auto-generated commit message, although (in my personal experience), “raising a concern” in the commit on Phabricator or commenting in the pre-commit review is usually sufficient to get author’s attention.
- We have Phabricator patches that automatically apply email comments to the Phabricator reviews, although reportedly this functionality is not fully reliable[3,4]. This can cause review comments to be lost in the email traffic.
Benefits:
- Single way of doing code reviews: code reviews are a key part of the development process, and having one way of performing them would make the process clearer and unambiguous.
- Review authors and reviewers would only need to monitor one source of comments without the fear that a review comment may end up overlooked.
- Local Phabricator extensions would no longer be needed.
Concerns:
- For post-commit reviews, the commenter would need to find either the original review, or the Phabricator commit (e.g. https://reviews.llvm.org/rG06234f758e19). Those are communicated (perhaps ironically) via email, which implies that those automatic emails should remain in place.
- The current policy has been in place for a long time and it’s expected that some people will continue using email for reviews out of habit or due to lack of awareness of the policy change.
- Because of the larger variety, email clients may offer better accessibility options than web browsers.
Potential future direction:
This section presents a potential future evolution of the review process. Christian has conducted experiments suggesting that we can replace the XXX-commits mailing lists with notifications directly from Phabricator:
- For each of the mailing lists, we create a "project" with the same name in Phabricator, e.g. [5]. Every Phabricator user can join/leave these projects on their own.
- Everyone on these projects will receive the same email notifications from Phabricator as we have on the mailing lists. This is configured via "Herald" rules in Phabricator, as today, e.g. [7].
- Users can reply to these email notifications and Phabricator will incorporate these responses with their email client, see [6] for some example emails. Quoting and markup is supported as well.
- We do NOT migrate the membership lists. Users need to sign up to the projects manually. We will send an email with instructions to the mailing lists once everything is set up.
- The current XXX-commits mailing lists will be shut down
- The timeline for the migration is to be defined.
For experimenting, feel free to sign up to the prototype project at [5] . This project receives all commit and code review notifications.
[1] https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review
[2] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html
[3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html
[4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html
[5] https://reviews.llvm.org/project/view/104/
[6] https://reviews.llvm.org/D101432
[7] https://reviews.llvm.org/H769
--
Krzysztof Parzyszek kpar...@quicinc.com AI tools development
Martin Storsjö via llvm-dev
> Potential future direction:
>
> This section presents a potential future evolution of the review process.
> Christian has conducted experiments suggesting that we can replace the
> XXX-commits mailing lists with notifications directly from Phabricator:
>
> projects on their own.
> "Herald" rules in Phabricator, as today, e.g. [7].
> example emails. Quoting and markup is supported as well.
> mailing lists once everything is set up.
I don't mind formalizing that reviews are done on phabricator only.
However following projects that way would, most probably, have one
quite notable drawback compared with the current mailing list based
approach:
Right now, it's easy to distinguish between mails requiring different
levels of attention; ones with me in the To or CC fields are more visible
and I try to read all of them. I have personal Herald rules that CC me on
topics that I track. But I also browse the rest of the mails (quickly
glancing usually only) for other topics I might be interested in.
My suspicion is that if the mail delivery is in the form of a personal
subscription directly from Phabricator, it becomes much harder to
distinguish mails that stem from just following a project as a whole, vs
ones where I'm specifically CCd.
On the other hand I guess there can be other ways of filtering the mails
do distinguish between those cases, so maybe it would be manageable?
// Martin
Viorel Preoteasa via llvm-dev
I have started reading the tutorial about writing a LLVM backend, but I
got stuck very quickly following it.
The tutorial says that it focuses on existing examples from
llvm/lib/Target, in the LLVM release. As far as
I can see, the current release versions of LLVM do not contain these
examples.
I did find the examples in the source distribution of LLVM, and
currently I am building the source, however
my question is the following:
Is it possible to develop a LLVM backend using for example the LLVM
distribution available by default
on Ubuntu (20.04), or possibly the binary distribution for Windows?
Best regards,
Viorel Preoteasa
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Roman Lebedev via llvm-dev
or affecting their current behaviour in any way.
The only thing i think you want to change is to codify that all new patches
should be submitted to review via phabricator.
Roman
> _______________________________________________
> cfe-dev mailing list
> cfe...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Kevin P. Neal via llvm-dev
> This section presents a potential future evolution of the review
> process. Christian has conducted experiments suggesting that we can
> replace the XXX-commits mailing lists with notifications directly from
> Phabricator:
Wouldn't this make it more difficult for sites that archive the lists?
Right now it all works. If the lists were eliminated then it would be
harder to archive. Not impossible, but it would be more work.
Plus, how long would it take for archive sites to switch over? How much
history would only exist in Phab's database?
Couldn't the commit lists be made read-only except from Phab? That would
force reviews to happen on Phab but otherwise keep all existing email
setups working.
--
"A method for inducing cats to exercise consists of directing a beam of
invisible light produced by a hand-held laser apparatus onto the floor ...
in the vicinity of the cat, then moving the laser ... in an irregular way
fascinating to cats,..." -- US patent 5443036, "Method of exercising a cat"
Krzysztof Parzyszek via llvm-dev
-----Original Message-----
From: lldb-dev <lldb-dev...@lists.llvm.org> On Behalf Of Krzysztof Parzyszek via lldb-dev
Sent: Monday, May 3, 2021 2:03 PM
To: Kevin P. Neal <k...@neutralgood.org>; Christian Kühnel <kuh...@google.com>
Cc: clang...@lists.llvm.org; openm...@lists.llvm.org; lldb...@lists.llvm.org; libcx...@lists.llvm.org; flan...@lists.llvm.org; parallel...@lists.llvm.org
Subject: [EXT] Re: [lldb-dev] [cfe-dev] [llvm-dev] [RFC] Deprecate email code reviews in favor of Phabricator
I'll defer to Christian the discussion about this section.
+Christian
cfe...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
lldb...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
Anton Korobeynikov via llvm-dev
Development of new backend assumes working with LLVM source code
--
With best regards, Anton Korobeynikov
Department of Statistical Modelling, Saint Petersburg State University
Michael Kruse via llvm-dev
<cfe...@lists.llvm.org>:
> Couldn't the commit lists be made read-only except from Phab? That would
> force reviews to happen on Phab but otherwise keep all existing email
> setups working.
I occasionally respond the email list to report a broken commit,
especially if the commit was not based on a Phabriacator review.
However, Phabricator also create a discussion page for each commit, I
could just add a comment there.
Is there still a discussion to eventually move to reviews on GitHub?
That might have an influence on what we are going to do with *-commits
mailing lists?
Michael
Philip Reames via llvm-dev
In my view, this email is really too different topics. Given that, my response is split into two parts.
First, should we make phabricator our default for code review? I
am not opposed to this. I don't particular support it either, but
I would not spend time arguing against it. I would suggest that
we re-frame the proposal to distinguish precommit and post commit
review - with only the former moving to phabricator. I have not
seen post-commit done successfully on phabricator to date in any
wide spread manner.
Second, should we consider retiring llvm-commits and the other
mailing lists? My gut response is a flat out NO!!!! What we have
works. I am highly reluctant to run the risk of breaking our
existing processes - which for all their problems mostly work -
for the, to me, seemingly very minimal value obtained by moving
away from email discussion. Post commit review in email works. I
strongly suspect that if you try to change that, you will either
simply drive out post commit review discussion (bad idea!) or
discussions will move to private email exchanges (bad idea!). I'm
open to being convinced here, but the burden of proof is high.
The risk we'd be talking about with such a transition is immense.
Philip
Mehdi AMINI via llvm-dev
In my view, this email is really too different topics. Given that, my response is split into two parts.
First, should we make phabricator our default for code review? I am not opposed to this. I don't particular support it either, but I would not spend time arguing against it. I would suggest that we re-frame the proposal to distinguish precommit and post commit review - with only the former moving to phabricator. I have not seen post-commit done successfully on phabricator to date in any wide spread manner.
Christian Kühnel via llvm-dev
I don't mind formalizing that reviews are done on phabricator only.
However following projects that way would, most probably, have one
quite notable drawback compared with the current mailing list based
approach:
Right now, it's easy to distinguish between mails requiring different
levels of attention; ones with me in the To or CC fields are more visible
and I try to read all of them. I have personal Herald rules that CC me on
topics that I track. But I also browse the rest of the mails (quickly
glancing usually only) for other topics I might be interested in.
X-Phabricator-Cc: <PHID-PROJ-6nrw7h47scgenrj2njpx>
X-Herald-Rules: <74>, <368>, <665>, <667>, <671>, 700>, <576>, <615>, <770>
X-Phabricator-Stamps: actor(@bruno) application(Differential) author(@bruno) herald(H74) herald(H368) herald(H576) herald(H615) herald(H665) herald(H667) herald(H671) herald(H700) herald(H770) monogram(D99434) object-type(DREV) phid(PHID-DREV-6ivftbt7xso57bvmy2br) reviewer(@aralisza) reviewer(@delcypher) reviewer(@dvyukov) reviewer(@kubamracek) reviewer(@vitalybuka) reviewer(@yln) revision-status(needs-review) subscriber(@hoy) subscriber(@jfb) subscriber(@kubamracek) subscriber(@llvm-commits) subscriber(@lxfind) subscriber(@modimo) subscriber(@rjmccall) subscriber(@t.p.northover) subscriber(@wenlei) tag(#llvm) via(web)
Christian Kühnel via llvm-dev
Strong, effectively blocking, -1 to shutting down -commits lists,
or affecting their current behaviour in any way.
The only thing i think you want to change is to codify that all new patches
should be submitted to review via phabricator.
Christian Kühnel via llvm-dev
Is there still a discussion to eventually move to reviews on GitHub?
That might have an influence on what we are going to do with *-commits
mailing lists?
Martin Storsjö via llvm-dev
On Tue, 4 May 2021, Christian Kühnel wrote:
> Having your own, custom Herald rules is always superior to general rules for
> a project. They are naturally targeted towards your use cases. However I
> wanted to offer a proper email integration for all users without having to
> write their own rules. So the idea was to offer a "similar enough"
> alternative for the XXX-commits mailing lists.
>
> I just checked your rules [1] and you add yourself to the list of
> subscribers for certain revisions. For these notifications you should be on
> the "TO" section of the email, right?
revisions where I'm reviewer or author, I'm in the TO field.
> The emails going through the project [1] are sent as CC to me. There is a
> ton of header attributes that could be used for filtering:
>
> X-Phabricator-Cc: <PHID-PROJ-6nrw7h47scgenrj2njpx>
>
> X-Herald-Rules: <74>, <368>, <665>, <667>, <671>, 700>, <576>,
> <615>, <770>
>
> X-Phabricator-Stamps: actor(@bruno) application(Differential)
> author(@bruno) herald(H74) herald(H368) herald(H576)
> herald(H615) herald(H665) herald(H667) herald(H671) herald(H700)
> herald(H770) monogram(D99434) object-type(DREV)
> phid(PHID-DREV-6ivftbt7xso57bvmy2br) reviewer(@aralisza)
> reviewer(@delcypher) reviewer(@dvyukov) reviewer(@kubamracek)
> reviewer(@vitalybuka) reviewer(@yln)
> revision-status(needs-review) subscriber(@hoy) subscriber(@jfb)
> subscriber(@kubamracek) subscriber(@llvm-commits)
> subscriber(@lxfind) subscriber(@modimo) subscriber(@rjmccall)
> subscriber(@t.p.northover) subscriber(@wenlei) tag(#llvm)
> via(web)
>
>
> Do you think this is good enough for filtering?
explicit subscriber but also subscribe to the "project", those mails would
still have the same tags.
So is there a way to distinguish mails where I'm an explicit tagged
subscriber (and thus in CC, also e.g. for reviews where I've taken part in
discussion) and I'm also getting them via the project subscription (so the
mail does have all the tags for subscription delivery)? Or would I be
getting two mails for that situation, once for the project subscription
and once for personal subscription to the individual review?
// Martin
Christian Kühnel via llvm-dev
In my view, this email is really too different topics. Given that, my response is split into two parts.
First, should we make phabricator our default for code review? I am not opposed to this. I don't particular support it either, but I would not spend time arguing against it. I would suggest that we re-frame the proposal to distinguish precommit and post commit review - with only the former moving to phabricator. I have not seen post-commit done successfully on phabricator to date in any wide spread manner.
Second, should we consider retiring llvm-commits and the other mailing lists? My gut response is a flat out NO!!!! What we have works.
I am highly reluctant to run the risk of breaking our existing processes
I strongly suspect that if you try to change that, you will either simply drive out post commit review discussion (bad idea!) or discussions will move to private email exchanges (bad idea!).
Christian Kühnel via llvm-dev
Hmm, maybe. The issue is pretty much the reverse - if I'm listed as
explicit subscriber but also subscribe to the "project", those mails would
still have the same tags.
So is there a way to distinguish mails where I'm an explicit tagged
subscriber (and thus in CC, also e.g. for reviews where I've taken part in
discussion) and I'm also getting them via the project subscription (so the
mail does have all the tags for subscription delivery)? Or would I be
getting two mails for that situation, once for the project subscription
and once for personal subscription to the individual review?
Aaron Ballman via llvm-dev
<cfe...@lists.llvm.org> wrote:
>
> Statement:
>
> Our current code review policy states[1]:
>
> “Code reviews are conducted, in order of preference, on our web-based code-review tool (see Code Reviews with Phabricator), by email on the relevant project’s commit mailing list, on the project’s development list, or on the bug tracker.”
>
> This proposal is to limit code reviews only to Phabricator. This would apply to all projects in the LLVM monorepo. With the change in effect, the amended policy would read:
>
> “Code reviews are conducted on our web-based code-review tool (see Code Reviews with Phabricator).”
Personally, I am in favor of this policy for initiating code reviews,
but am opposed to it for post-commit feedback. The problem, as I see
it, is that not every change *gets* code review via Phab and the email
lists are the only place on which to provide that post-commit
feedback. This largely comes up in two ways: NFC changes and changes
made by code owners in the area of the compiler which they own. We
still need *some* mechanism by which to provide them post-commit
feedback. Currently, the way we provide that is frequently via an
email reply to the commit message on the *-commits list so that the
issue can be seen by both the patch author and the community at large.
> Current situation:
>
> In a recent llvm-dev thread[2], Christian Kühnel pointed out that pre-commit code reviews rarely originate via an email (most are started on Phabricator), although, as others pointed out, email responses to an ongoing review are not uncommon. (That thread also contains examples of mishaps related to the email-Phabricator interactions, or email handling itself.)
> I don’t have specific information about post-commit reviews. It seems like the most common form is an email reply to the auto-generated commit message, although (in my personal experience), “raising a concern” in the commit on Phabricator or commenting in the pre-commit review is usually sufficient to get author’s attention.
> We have Phabricator patches that automatically apply email comments to the Phabricator reviews, although reportedly this functionality is not fully reliable[3,4]. This can cause review comments to be lost in the email traffic.
>
>
>
> Benefits:
>
> Single way of doing code reviews: code reviews are a key part of the development process, and having one way of performing them would make the process clearer and unambiguous.
> Review authors and reviewers would only need to monitor one source of comments without the fear that a review comment may end up overlooked.
> Local Phabricator extensions would no longer be needed.
>
>
>
> Concerns:
>
> For post-commit reviews, the commenter would need to find either the original review, or the Phabricator commit (e.g. https://reviews.llvm.org/rG06234f758e19). Those are communicated (perhaps ironically) via email, which implies that those automatic emails should remain in place.
The Phab commit message does not have any subscribers though, and so
my understanding is that comments on that Phab interface are not
communicated out to the community as a whole, which means the
community may miss out on important post-commit-review information
like general awareness of the problem, workarounds people can use
until the author corrects something, alternative ideas on how to fix
the issue, etc. Or do I misunderstand the way Phab works in this
workflow?
Also, "the commenter would need to find the Phabricator commit"
concerns me -- adding extra burden on the people providing feedback
means that *some* amount of those people will struggle enough to
simply not provide that feedback. Responding to an email is about as
low as I think we can set that bar, so the current approach has better
ergonomics for giving feedback. When I look at an existing NFC commit
email (https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20210503/368413.html),
I don't see any direct link back to the Phabricator commit, so I have
to know to get the hash and try using that with an
https://reviews.llvm.org/rG in front of it. None of the existing links
in the email get me to where I'd need to go to add my review feedback,
but hitting the Reply button in my mail client will work. Adding a
third link and telling people "click on the link in the email" means
they've got a 50/50 shot of getting the right link because one of the
links goes to GitHub where you can also add comments.
> The current policy has been in place for a long time and it’s expected that some people will continue using email for reviews out of habit or due to lack of awareness of the policy change.
> Because of the larger variety, email clients may offer better accessibility options than web browsers.
>
>
>
> Potential future direction:
>
> This section presents a potential future evolution of the review process. Christian has conducted experiments suggesting that we can replace the XXX-commits mailing lists with notifications directly from Phabricator:
>
> For each of the mailing lists, we create a "project" with the same name in Phabricator, e.g. [5]. Every Phabricator user can join/leave these projects on their own.
> Everyone on these projects will receive the same email notifications from Phabricator as we have on the mailing lists. This is configured via "Herald" rules in Phabricator, as today, e.g. [7].
Tangential complaint -- our use of Herald causes some pain for me as a
list moderator because we've reached the point where Herald
automatically adds so many subscribers to a review that it gets marked
as spam for every email that is generated for the review. It's to the
point on cfe-dev where over half of the moderated emails are
consistently not spam some weeks. This delays the community receiving
the information while the patch reviewers/subscribers continue to get
it. In turn, this causes a problem where sometimes the people
subscribed to the patch say something is OK and the patch lands before
the community ever sees the review and has a chance to comment on it.
I'm wary of suggestions that involve heavier use of Herald until we
get that mailing list issue resolved.
> Users can reply to these email notifications and Phabricator will incorporate these responses with their email client, see [6] for some example emails. Quoting and markup is supported as well.
> We do NOT migrate the membership lists. Users need to sign up to the projects manually. We will send an email with instructions to the mailing lists once everything is set up.
> The current XXX-commits mailing lists will be shut down
> The timeline for the migration is to be defined.
Given how often Phabricator goes down (which is not super often, but
often enough that people know it happens), I am deeply uncomfortable
with the idea of shutting down the current *-commits mailing lists
because of the chance for data loss. Personally, I think the *-commits
lists are working well and I would prefer they be left alone.
~Aaron
>
> For experimenting, feel free to sign up to the prototype project at [5] . This project receives all commit and code review notifications.
>
>
>
>
>
> [1] https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review
>
> [2] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html
>
> [3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html
>
> [4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html
>
> [5] https://reviews.llvm.org/project/view/104/
>
> [6] https://reviews.llvm.org/D101432
>
> [7] https://reviews.llvm.org/H769
>
>
>
>
>
> --
>
> Krzysztof Parzyszek kpar...@quicinc.com AI tools development
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Christian Kühnel via llvm-dev
> “Code reviews are conducted on our web-based code-review tool (see Code Reviews with Phabricator).”
Personally, I am in favor of this policy for initiating code reviews,
but am opposed to it for post-commit feedback. The problem, as I see
it, is that not every change *gets* code review via Phab and the email
lists are the only place on which to provide that post-commit
feedback.
The Phab commit message does not have any subscribers though, and so
my understanding is that comments on that Phab interface are not
communicated out to the community as a whole, which means the
community may miss out on important post-commit-review information
like general awareness of the problem, workarounds people can use
until the author corrects something, alternative ideas on how to fix
the issue, etc. Or do I misunderstand the way Phab works in this
workflow?
Also, "the commenter would need to find the Phabricator commit"
concerns me -- adding extra burden on the people providing feedback
means that *some* amount of those people will struggle enough to
simply not provide that feedback. Responding to an email is about as
low as I think we can set that bar, so the current approach has better
ergonomics for giving feedback.
Tangential complaint -- our use of Herald causes some pain for me as a
list moderator because we've reached the point where Herald
automatically adds so many subscribers to a review that it gets marked
as spam for every email that is generated for the review.
Given how often Phabricator goes down (which is not super often, but
often enough that people know it happens), I am deeply uncomfortable
with the idea of shutting down the current *-commits mailing lists
because of the chance for data loss. Personally, I think the *-commits
lists are working well and I would prefer they be left alone.
Krzysztof Parzyszek via llvm-dev
I’m opposed to separating the pre- and post-commit reviews. One of the goals of this proposal is to have the entire review history in one place, and using a combination of email and Phabricator would prevent that. If I want to find out why a commit has been reverted, I have to search the post-commit emails to see what happened. I guess one could argue that pre- and post-commit reviews could happen on different pages (Dxxx vs rGxxx), but, in my view, that is still better than emails. The two concerns I have about post-commit reviews on Phabricator are that
- People will keep doing it via email, because there is no mechanism (short of making the -commits lists read-only) that would actively inform them about the change in policy.
- It takes a bit of manual work to get to the rG page for the commit, and if the commit message doesn’t include the link to the pre-commit review, the corresponding D page may be hard to locate.
Regarding point (2), I’m hoping that something can be done in Phabricator to make the post-commit reviews easier: it may be as simple as sending the commit message to a list of subscribers.
From: Philip Reames <list...@philipreames.com>
Sent: Monday, May 3, 2021 5:08 PM
To: Krzysztof Parzyszek <kpar...@quicinc.com>; llvm-dev <llvm...@lists.llvm.org>
Cc: clang...@lists.llvm.org; openm...@lists.llvm.org; lldb...@lists.llvm.org; cfe...@lists.llvm.org; libcx...@lists.llvm.org; flan...@lists.llvm.org; parallel...@lists.llvm.org
via llvm-dev
I’m not hearing any particular objection to moving to Phabricator-only for reviews. The trend has been in that direction for years anyway. The barriers for one-time contributors are (a) registration, and (b) unfamiliarity with a user-hostile web UI. Regarding registration, it’s either subscribing to llvm-commits or to Phab; and IIUC, Phab understands github IDs, which makes that barrier pretty low. Regarding the web UI, we have (or did have, admittedly I haven’t looked lately) step-by-step instructions in the docs for how to navigate Phab successfully, which mitigates (b) as much as we can.
I am in the camp of, why mess with llvm-commits? The commit emails that Phab sends out already have the commit URL in them right at the top, which looks like it goes to a page where comments can be added (I haven’t actually tried adding a comment). If those posted comments go to llvm-commits and the author, then I think the only necessary step to turn off email post-commit review is to make llvm-commits read-only except for Phabricator itself.
We don’t need the ability to have email replies on llvm-commits be recorded in Phab, if we can almost as easily make comments on Phab that go to llvm-commits. Possibly I am overlooking some problem that abandoning llvm-commits entirely is going to solve?
--paulr
Krzysztof Parzyszek via llvm-dev
I'm not sure if there is a general fix for the spam issue. I have had this problem as well with the existing lists, so it's not limited to Phabricator. In our spam filter I have whitelisted enough emails to avoid the unnecessary filtering almost completely, but it is an issue nevertheless.
From: Aaron Ballman <aa...@aaronballman.com>
Sent: Tuesday, May 4, 2021 6:24 AM
To: Krzysztof Parzyszek <kpar...@quicinc.com>
Cc: llvm-dev <llvm...@lists.llvm.org>; clang...@lists.llvm.org; openm...@lists.llvm.org; lldb...@lists.llvm.org; cfe...@lists.llvm.org; libcx...@lists.llvm.org; flan...@lists.llvm.org; parallel...@lists.llvm.org
via llvm-dev
> seamless---the rG link is not included anywhere. Hopefully that could be
> fixed with some Phabricator configuration tweaks, like sending the commit
> email to the -commits list.
Dnnnnn review):
URL: https://github.com/llvm/llvm-project/commit/b04148f77713c92ee57b33b7b858ad18ce8d78e3
Does that take you to a different place than the rG link would?
Seems like they ought to go to the same place.
--paulr
Aaron Ballman via llvm-dev
>
> > You're right that doing post-commit reviews on Phabricator is not
> > seamless---the rG link is not included anywhere. Hopefully that could be
> > fixed with some Phabricator configuration tweaks, like sending the commit
> > email to the -commits list.
>
> The commit email has a URL: link, e.g. this recent one (which has no
> Dnnnnn review):
>
> URL: https://github.com/llvm/llvm-project/commit/b04148f77713c92ee57b33b7b858ad18ce8d78e3
>
> Does that take you to a different place than the rG link would?
> Seems like they ought to go to the same place.
That sends you to the GitHub commit for the change. The corresponding
rG link would be:
https://reviews.llvm.org/rGb04148f77713c92ee57b33b7b858ad18ce8d78e3
~Aaron
James Henderson via llvm-dev
> You're right that doing post-commit reviews on Phabricator is not
> seamless---the rG link is not included anywhere. Hopefully that could be
> fixed with some Phabricator configuration tweaks, like sending the commit
> email to the -commits list.
The commit email has a URL: link, e.g. this recent one (which has no
Dnnnnn review):
URL: https://github.com/llvm/llvm-project/commit/b04148f77713c92ee57b33b7b858ad18ce8d78e3
Does that take you to a different place than the rG link would?
Seems like they ought to go to the same place.
--paulr
_______________________________________________
via llvm-dev
> > Dnnnnn review):
> >
> > URL: https://github.com/llvm/llvm-project/commit/b04148f77713c92ee57b33b7b858ad18ce8d78e3
> >
> > Does that take you to a different place than the rG link would?
> > Seems like they ought to go to the same place.
>
> That sends you to the GitHub commit for the change
github doesn't know the Phab URL. We'd have to have Phab send a
commit email instead, and insert the rG URL. That's a bit more
complicated, yep.
--paulr
Christian Kühnel via llvm-dev
Christian Kühnel via llvm-dev
Mehdi AMINI via llvm-dev
None of the existing links
in the email get me to where I'd need to go to add my review feedback,
but hitting the Reply button in my mail client will work. Adding a
third link and telling people "click on the link in the email" means
they've got a 50/50 shot of getting the right link because one of the
links goes to GitHub where you can also add comments.
> The current policy has been in place for a long time and it’s expected that some people will continue using email for reviews out of habit or due to lack of awareness of the policy change.
> Because of the larger variety, email clients may offer better accessibility options than web browsers.
>
>
>
> Potential future direction:
>
> This section presents a potential future evolution of the review process. Christian has conducted experiments suggesting that we can replace the XXX-commits mailing lists with notifications directly from Phabricator:
>
> For each of the mailing lists, we create a "project" with the same name in Phabricator, e.g. [5]. Every Phabricator user can join/leave these projects on their own.
> Everyone on these projects will receive the same email notifications from Phabricator as we have on the mailing lists. This is configured via "Herald" rules in Phabricator, as today, e.g. [7].
Tangential complaint -- our use of Herald causes some pain for me as a
list moderator because we've reached the point where Herald
automatically adds so many subscribers to a review that it gets marked
as spam for every email that is generated for the review. It's to the
point on cfe-dev where over half of the moderated emails are
consistently not spam some weeks.
This delays the community receiving
the information while the patch reviewers/subscribers continue to get
it. In turn, this causes a problem where sometimes the people
subscribed to the patch say something is OK and the patch lands before
the community ever sees the review and has a chance to comment on it.
I'm wary of suggestions that involve heavier use of Herald until we
get that mailing list issue resolved.
> Users can reply to these email notifications and Phabricator will incorporate these responses with their email client, see [6] for some example emails. Quoting and markup is supported as well.
> We do NOT migrate the membership lists. Users need to sign up to the projects manually. We will send an email with instructions to the mailing lists once everything is set up.
> The current XXX-commits mailing lists will be shut down
> The timeline for the migration is to be defined.
Given how often Phabricator goes down (which is not super often, but
often enough that people know it happens),
Aaron Ballman via llvm-dev
Sorry, think-o on my part. I meant to say cfe-commits, not cfe-dev.
> But also, I didn't know that the mailman instance had a spam filter: I thought this was based purely on whether the sender is subscribed (or whitelisted)?
It also filters based on number of recipients on the message and the
size of the message body (which occasionally gets tripped by
legitimate reviews that have very large diffs).
>> This delays the community receiving
>> the information while the patch reviewers/subscribers continue to get
>> it. In turn, this causes a problem where sometimes the people
>> subscribed to the patch say something is OK and the patch lands before
>> the community ever sees the review and has a chance to comment on it.
>> I'm wary of suggestions that involve heavier use of Herald until we
>> get that mailing list issue resolved.
>>
>> > Users can reply to these email notifications and Phabricator will incorporate these responses with their email client, see [6] for some example emails. Quoting and markup is supported as well.
>> > We do NOT migrate the membership lists. Users need to sign up to the projects manually. We will send an email with instructions to the mailing lists once everything is set up.
>> > The current XXX-commits mailing lists will be shut down
>> > The timeline for the migration is to be defined.
>>
>> Given how often Phabricator goes down (which is not super often, but
>> often enough that people know it happens),
>
> Data-driven: I am aware of two outages for the last entire year, and each lasted for < 1/2 day.
According to my totally unscientific method of searching for
"phabricator down" in my archived email, I see about 600 messages on
the topic. This also includes scheduled downtime announcements,
resumed service announcements, DNS mishaps local to people, and
replies on threads so it's not meant to suggest Phab has gone down
anywhere near 600 times. However, the kind of issues I am worried
about are ones where Phabricator would give exceptions (as in
https://lists.llvm.org/pipermail/llvm-dev/2020-June/142836.html).
~Aaron
>
>
>
>>
>> I am deeply uncomfortable
>> with the idea of shutting down the current *-commits mailing lists
>> because of the chance for data loss. Personally, I think the *-commits
>> lists are working well and I would prefer they be left alone.
>>
>> ~Aaron
>>
>> >
>> > For experimenting, feel free to sign up to the prototype project at [5] . This project receives all commit and code review notifications.
>> >
>> >
>> >
>> >
>> >
>> > [1] https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review
>> >
>> > [2] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html
>> >
>> > [3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html
>> >
>> > [4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html
>> >
>> > [5] https://reviews.llvm.org/project/view/104/
>> >
>> > [6] https://reviews.llvm.org/D101432
>> >
>> > [7] https://reviews.llvm.org/H769
>> >
>> >
>> >
>> >
>> >
>> > --
>> >
>> > Krzysztof Parzyszek kpar...@quicinc.com AI tools development
>> >
>> >
>> >
>> > _______________________________________________
>> > cfe-dev mailing list
>> > cfe...@lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> _______________________________________________
>> cfe-dev mailing list
>> cfe...@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Mehdi AMINI via llvm-dev
Krzysztof Parzyszek via llvm-dev
The other side to consider is email reliability: how often emails get stuck somewhere along the way (i.e. delayed), how often they end up in the spam filter, etc. Also, in the thread where the idea for this RFC first came up, I mentioned some of the issues I have with my email client (Outlook). Outlook is pretty bad when it comes to email threads: some responses are shown together, others show up as separate emails. With the volume of traffic (especially on the -commits lists) it’s quite easy to overlook some email, which is in practice equivalent to never receiving the email in the first place.
From: Mehdi AMINI <joke...@gmail.com>
Sent: Wednesday, May 5, 2021 3:00 PM
To: Aaron Ballman <aa...@aaronballman.com>
Cc: Krzysztof Parzyszek <kpar...@quicinc.com>; llvm-dev <llvm...@lists.llvm.org>; clang...@lists.llvm.org; openm...@lists.llvm.org; lldb...@lists.llvm.org; cfe...@lists.llvm.org; libcx...@lists.llvm.org; flan...@lists.llvm.org; parallel...@lists.llvm.org
Subject: [EXT] Re: [cfe-dev] [RFC] Deprecate email code reviews in favor of Phabricator
