FYI: LLVM Phabricactor notifications.

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

Gavin Cui via cfe-dev

The Revision won't have 'rL' in it, until after it has been committed, and the SVN revision number is assigned. That's why the particular herald rule doesn't fire until the revision is closed.

There is negative value in having changes to Clang (and LLDB, and maybe others) cc'd to llvm-commits. Those projects have their own mailing lists. Spamming llvm-commits helps nobody.

 

Maybe the problem is that Clang and LLDB commits are somehow associated with 'rL' instead of some tag of their own.  I don't know anything about how that was all set up.  All I know is that it's annoying and clutters up the mailing list.

--paulr

 

From: MyDeveloper Day [mailto:[hidden email]]
Sent: Monday, June 03, 2019 1:08 PM
To: Robinson, Paul
Cc: David Jones; llvm-dev; clang developer list; Aaron Ballman
Subject: Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

 

Sorry maybe I didn't explain what I saw..(again sorry if this is known already and I'm stating the obvious to people who know better than me.) 

 

The Revision contains rL in the repository field, as such the herald rule will add the LLVM project.and the llvm-commit subscriber whenever the revision is updated.

 

When the commit AutoCloses, it updates the Revision, as such if the revision still has the rL repository field then I believe the rule will fire again and add them. (post commit)

 

This revision was automatically updated to reflect the committed changes.  

 

Conditions

Passed

Repository is any of rCRT, rL, rLLD, rPLO, rT

Passed

Revision title does not contain [private]

Passed

Rule passed.

Action: Add projects

Added Projects

Added a project: LLVM.

 

Action: Add subscribers

Added Subscribers

Added a subscriber: llvm-commits.

 

Its true I'm  a little surprised that the Herald rule hadn't fired earlier and added the LLVM project and llvm-commit subscriber already, unless somehow it had, but they were removed manually, but I see no trace of that in the history.

 

Potentially you could extend the rule to say "Revision status is not any of  Accepted", but I guess the whole point of that rule is to tell people watching rL that a commit for a revision marked as being part of rL has been changed

 

Unfortunately there isn't any connection between the Revision repository and the actual repo its committed to.

 

I do notice LLVM doesn't use the "Owners" application in Phabricator, this is an great way of ensuring code is automatically channeled to code owners/reviews for a particular area via the "Affected packages" in a hearld rule, and can be used to automate the adding of reviewers, blocking reviews, adding projects (but alas it doesn't allow the setting of the repository from what I can tell) 

 

MyDeveloperDay (Paul)

 

 

 

On Mon, Jun 3, 2019 at 5:21 PM <[hidden email]> wrote:

As there is no mention of the repository being change the revisions feed (https://reviews.llvm.org/D62616) I suspect it was created that way, and its only as the commit fires that it gets added.  (it might be clearer if a herald rule so these are added at review creation, although anyone then removing them will get them readded at commit if they still have the incorrect repository.)

 

Note the highlighted part of this quote from the revision-closed email:

 

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362363: [CodeComplete] Add a bit more whitespace to completed patterns (authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits
.

 

The LLVM project was added at the time Phabricator saw the closing commit, not when the revision was created.

If it had been added at the time the revision was created, all review emails would have gone to both lists. They did not.

--paulr

 

From: MyDeveloper Day [mailto:[hidden email]]
Sent: Monday, June 03, 2019 11:53 AM
To: Robinson, Paul
Cc: [hidden email]; llvm-dev; clang developer list; Aaron Ballman
Subject: Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

 

PaulR

 

(sorry again if this is known knowledge)

 

There's no reason for Herald to be adding project LLVM/subscriber llvm-commits at the last second here.

 

Its possible the rL (LLVM) had be added as the repository in the review on creation rather than rCFE, if thats the case then the herald rule "H270" is going to fire because it see the repository in the review, so add LLVM project and llvm-commits as a subscriber automatically. It won't care that this has gone into rCFE and not rL  (I mean it does go into rL via the cfe/trunk but I'm not sure if you want to notify for that)

 

As there is no mention of the repository being change the revisions feed (https://reviews.llvm.org/D62616) I suspect it was created that way, and its only as the commit fires that it gets added.  (it might be clearer if a herald rule so these are added at review creation, although anyone then removing them will get them readded at commit if they still have the incorrect repository.)

 

MyDeveloperDay (Paul)


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

Gavin Cui via cfe-dev
In reply to this post by Gavin Cui via cfe-dev

IIRC Ben Hamilton was the one who set these Herald rules up. I don’t have his email, but someone who does could CC him.

 

From: cfe-dev <[hidden email]> on behalf of cfe-dev <[hidden email]>
Reply-To: "[hidden email]" <[hidden email]>
Date: Monday, June 3, 2019 at 11:21 AM
To: "[hidden email]" <[hidden email]>
Cc: "[hidden email]" <[hidden email]>, cfe-dev <[hidden email]>, "[hidden email]" <[hidden email]>
Subject: Re: [cfe-dev] [llvm-dev] FYI: LLVM Phabricactor notifications.

 

The Revision won't have 'rL' in it, until after it has been committed, and the SVN revision number is assigned. That's why the particular herald rule doesn't fire until the revision is closed.

There is negative value in having changes to Clang (and LLDB, and maybe others) cc'd to llvm-commits. Those projects have their own mailing lists. Spamming llvm-commits helps nobody.

 

Maybe the problem is that Clang and LLDB commits are somehow associated with 'rL' instead of some tag of their own.  I don't know anything about how that was all set up.  All I know is that it's annoying and clutters up the mailing list.

--paulr

 

From: MyDeveloper Day [mailto:[hidden email]]
Sent: Monday, June 03, 2019 1:08 PM
To: Robinson, Paul
Cc: David Jones; llvm-dev; clang developer list; Aaron Ballman
Subject: Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

 

Sorry maybe I didn't explain what I saw..(again sorry if this is known already and I'm stating the obvious to people who know better than me.) 

 

The Revision contains rL in the repository field, as such the herald rule will add the LLVM project.and the llvm-commit subscriber whenever the revision is updated.

 

When the commit AutoCloses, it updates the Revision, as such if the revision still has the rL repository field then I believe the rule will fire again and add them. (post commit)

 

This revision was automatically updated to reflect the committed changes.  

 

Conditions

Passed

Repository is any of rCRT, rL, rLLD, rPLO, rT

Passed

Revision title does not contain [private]

Passed

Rule passed.

Action: Add projects

Added Projects

Added a project: LLVM.

 

Action: Add subscribers

Added Subscribers

Added a subscriber: llvm-commits.

 

Its true I'm  a little surprised that the Herald rule hadn't fired earlier and added the LLVM project and llvm-commit subscriber already, unless somehow it had, but they were removed manually, but I see no trace of that in the history.

 

Potentially you could extend the rule to say "Revision status is not any of  Accepted", but I guess the whole point of that rule is to tell people watching rL that a commit for a revision marked as being part of rL has been changed

 

Unfortunately there isn't any connection between the Revision repository and the actual repo its committed to.

 

I do notice LLVM doesn't use the "Owners" application in Phabricator, this is an great way of ensuring code is automatically channeled to code owners/reviews for a particular area via the "Affected packages" in a hearld rule, and can be used to automate the adding of reviewers, blocking reviews, adding projects (but alas it doesn't allow the setting of the repository from what I can tell) 

 

MyDeveloperDay (Paul)

 

 

 

On Mon, Jun 3, 2019 at 5:21 PM <[hidden email]> wrote:

As there is no mention of the repository being change the revisions feed (https://reviews.llvm.org/D62616) I suspect it was created that way, and its only as the commit fires that it gets added.  (it might be clearer if a herald rule so these are added at review creation, although anyone then removing them will get them readded at commit if they still have the incorrect repository.)

 

Note the highlighted part of this quote from the revision-closed email:

 

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362363: [CodeComplete] Add a bit more whitespace to completed patterns (authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits
.

 

The LLVM project was added at the time Phabricator saw the closing commit, not when the revision was created.

If it had been added at the time the revision was created, all review emails would have gone to both lists. They did not.

--paulr

 

From: MyDeveloper Day [mailto:[hidden email]]
Sent: Monday, June 03, 2019 11:53 AM
To: Robinson, Paul
Cc: [hidden email]; llvm-dev; clang developer list; Aaron Ballman
Subject: Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

 

PaulR

 

(sorry again if this is known knowledge)

 

There's no reason for Herald to be adding project LLVM/subscriber llvm-commits at the last second here.

 

Its possible the rL (LLVM) had be added as the repository in the review on creation rather than rCFE, if thats the case then the herald rule "H270" is going to fire because it see the repository in the review, so add LLVM project and llvm-commits as a subscriber automatically. It won't care that this has gone into rCFE and not rL  (I mean it does go into rL via the cfe/trunk but I'm not sure if you want to notify for that)

 

As there is no mention of the repository being change the revisions feed (https://reviews.llvm.org/D62616) I suspect it was created that way, and its only as the commit fires that it gets added.  (it might be clearer if a herald rule so these are added at review creation, although anyone then removing them will get them readded at commit if they still have the incorrect repository.)

 

MyDeveloperDay (Paul)


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] FYI: LLVM Phabricactor notifications.

Gavin Cui via cfe-dev
In reply to this post by Gavin Cui via cfe-dev
On 05/31/2019 12:54 AM, Manuel Klimek wrote:
> We should be able to control the noise of these, but I also wonder whether we can switch to github reviews as part of the git move :)
> (we just lost our long time phab maintainer on the team)
>

Hi,

Were we ever able to resolve this issue?

I'm trying to test a GitHub app that auto-closes pull requests, and
I'm also seeing excessive notifications from Phabricator.  Every time
someone commits a new patch, the pull requests seems to get rebased
behind the scenes and I get a new commit notification.

Thanks,
Tom

> On Fri, May 31, 2019 at 12:44 AM Shoaib Meenai via llvm-dev <[hidden email] <mailto:[hidden email]>> wrote:
>
>     I believe (and I believe it was James who pointed this out on IRC) that Phabricator pulls in all refs, and GitHub stores PR commits under refs/pull.
>
>     On 5/30/19, 3:37 PM, "llvm-dev on behalf of Tom Stellard via llvm-dev" <[hidden email] <mailto:[hidden email]> on behalf of [hidden email] <mailto:[hidden email]>> wrote:
>
>         On 05/30/2019 10:04 AM, Sachkov, Alexey via llvm-dev wrote:
>         > +llvm-dev
>         >
>         >
>         >
>         > *From:* cfe-dev [mailto:[hidden email] <mailto:[hidden email]>] *On Behalf Of *Bader, Alexey via cfe-dev
>         > *Sent:* Thursday, May 30, 2019 7:31 PM
>         > *To:* clang-dev developer list <[hidden email] <mailto:[hidden email]>>
>         > *Subject:* [cfe-dev] FYI: LLVM Phabricactor notifications.
>         > *Importance:* Low
>         >
>         >
>         >
>         > Hi,
>         >
>         >
>         >
>         > I think some of contributors to the Clang received a notifications about some commits done in the past.
>         >
>         > I wanted to share my thoughts on why it might has happened.
>         >
>         >
>         >
>         > I think the commits from this PR https://github.com/llvm/llvm-project/pull/13were pulled by Phabricator (probably with aim to review GitHub pull requests in https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=AD-hZ9rLcmVgzhl1TFTAvKbX-nrgHD75y9sqN5zTEsg&s=KaCscc77Z-fBuYo2dCuykCeHg35yKQXRwp6uVuIsPos&e= environment).
>         >
>         >
>         >
>         > The PR has 2000+ commit, most of which are some old commits from the master branch, and when Phabricator pulled the commits from PR, it sent a notification to the commit author (or committer).
>         >
>         >
>         >
>         > Probably we can do something to avoid this situation in the future.
>         >
>         >
>
>         + Manuel, Chandler
>
>         Any idea what happened here? Is phabricator setup to automatically import
>         pull requests?
>
>         -Tom
>
>         >
>         > Sorry for the inconvenience.
>         >
>         >
>         >
>         > Alexey
>         >
>         >
>         >
>         >
>         > --------------------------------------------------------------------
>         > Joint Stock Company Intel A/O
>         > Registered legal address: Krylatsky Hills Business Park,
>         > 17 Krylatskaya Str., Bldg 4, Moscow 121614,
>         > Russian Federation
>         >
>         > This e-mail and any attachments may contain confidential material for
>         > the sole use of the intended recipient(s). Any review or distribution
>         > by others is strictly prohibited. If you are not the intended
>         > recipient, please contact the sender and delete all copies.
>         >
>         >
>         >
>         > _______________________________________________
>         > LLVM Developers mailing list
>         > [hidden email] <mailto:[hidden email]>
>         > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=AD-hZ9rLcmVgzhl1TFTAvKbX-nrgHD75y9sqN5zTEsg&s=42V9lvDIvDXSSvzhNIJLw7ddZMt54c2yNRxFpSMppEg&e=
>         >
>
>         _______________________________________________
>         LLVM Developers mailing list
>         [hidden email] <mailto:[hidden email]>
>         https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=AD-hZ9rLcmVgzhl1TFTAvKbX-nrgHD75y9sqN5zTEsg&s=42V9lvDIvDXSSvzhNIJLw7ddZMt54c2yNRxFpSMppEg&e=
>
>
>     _______________________________________________
>     LLVM Developers mailing list
>     [hidden email] <mailto:[hidden email]>
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12