Phabricator update

Greetings llvm’ers,

I’m working on an update to phabricator that brings our fork significantly closer to the upstream version, thus making maintenance much easier; this will allow us to upgrade more often.

Upstream has fixed many issues and introduced new ways to customize the email flow, so

a) while rolling out the new version I’ll be fiddling with config settings; during that time, you might see emails that are not yet what we’ll want the end-state to be; I’ll announce on this thread once this is done

b) the new version gives people more control over the emails; that means, if you’re a phab user, you might get some emails that the list doesn’t get (if you wish to); you can configure that from phab’s email settings

Once this is finished, please respond on this thread with your concerns / comments, or file bugs at https://llvm.org/bugs/enter_bug.cgi?product=Website.

Cheers,
/Manuel

Unfortunately threading will be broken for changes currently under review (new patches shouldn’t have the problem).
I’m very sorry for this inconvenience :frowning:

We can live with that. :slight_smile:

Phab is looking a lot better already, thanks for the hard work!

cheers,
--renato

This might be slightly off topic, but I’d really like a way to be able to run the buildbots off a Phabricator Diff before committing.

Even further off topic, in phab wishlist land: It'd be awesome if it were capable of inferring extended patch context by looking at the svn repo/git mirrors (rather than requiring the person submitting the patch to re-upload with -U999).

Jon

This might be slightly off topic, but I’d really like a way to be able to run the buildbots off a Phabricator Diff before committing.

Please file feature requests in the llvm bug tracker :slight_smile:

Even further off topic, in phab wishlist land: It’d be awesome if it
were capable of inferring extended patch context by looking at the svn
repo/git mirrors (rather than requiring the person submitting the patch
to re-upload with -U999).

Yea, this is hard, because detecting which path a patch goes against is hard (for example, if the patch path is a pure add for a new tools/sometool directory, is it in clang or llvm?) and requiring people to do global top-level llvm patches seems rather problematic.

The workaround is to use arc diff (the command line tool).

Ok, most of the things should be back to normal - I’m still working on weeding out unhelpful headers (the top-level things that say who did what), let me know if you see ones you find particularly bad from now on (I just switched a bunch off based on the past day of llvm patches).

And a tip: don’t set the Revision / Project field in phab - we don’t use that for anything, and currently it’ll mainly make the emails longer…

Cheers,
/Manuel

Even further off topic, in phab wishlist land: It’d be awesome if it
were capable of inferring extended patch context by looking at the svn
repo/git mirrors (rather than requiring the person submitting the patch
to re-upload with -U999).

Yea, this is hard, because detecting which path a patch goes against is hard (for example, if the patch path is a pure add for a new tools/sometool directory, is it in clang or llvm?) and requiring people to do global top-level llvm patches seems rather problematic.

The workaround is to use arc diff (the command line tool).

Speaking of which, in case anyone else has issues with arc today, this might save you a minute or two.

$ arc diff
Exception
ERR-CONDUIT-CALL: API Method “differential.query” does not define these parameters: ‘arcanistProjects’.
(Run with --trace for a full exception trace.)

Fix this by upgrading with “arc upgrade”.

Then, say you’re sending a patch for Clang, you’ll need to use “cfe-commits-list” in the subscriber list instead of “cfe-commits”. Otherwise you get:

“”"

Commit message has errors:

  • Error parsing field “Subscribers”: The objects you have listed
    include objects which do not exist (cfe-commits).
    “”"

(this might be temporary?)

Cheers,
Andrew

Even further off topic, in phab wishlist land: It’d be awesome if it
were capable of inferring extended patch context by looking at the svn
repo/git mirrors (rather than requiring the person submitting the patch
to re-upload with -U999).

Yea, this is hard, because detecting which path a patch goes against is hard (for example, if the patch path is a pure add for a new tools/sometool directory, is it in clang or llvm?) and requiring people to do global top-level llvm patches seems rather problematic.

The workaround is to use arc diff (the command line tool).

Speaking of which, in case anyone else has issues with arc today, this might save you a minute or two.

$ arc diff
Exception
ERR-CONDUIT-CALL: API Method “differential.query” does not define these parameters: ‘arcanistProjects’.
(Run with --trace for a full exception trace.)

Fix this by upgrading with “arc upgrade”.

Then, say you’re sending a patch for Clang, you’ll need to use “cfe-commits-list” in the subscriber list instead of “cfe-commits”. Otherwise you get:

“”"

Commit message has errors:

  • Error parsing field “Subscribers”: The objects you have listed
    include objects which do not exist (cfe-commits).
    “”"

(this might be temporary?)

Thanks for reporting - this happened as part of the upgrade automatically (mailing lists were migrated to mailing list users). Changed the names back, you should be able to type cfe-commits/llvm-commits/lldb-commits again.

Hi Manual,

Since the Phabricator update I noticed that the username of the reviewer isn’t displayed for some comment added to the review (e.g.: http://reviews.llvm.org/D11016). Can you check it out what is causing this issue?

Thanks,
Tamas

Those seem to be comments by the original author? Is this a problem?

Not always, see http://reviews.llvm.org/D10676 or any other.

I can only see the commenter name if they change some status, like
adding themselves to the list.

--renato

Ah, thanks, interesting - investigating

Ok, the problems is that in an attempt to make the emails more concise (so we have less clutter at the top where it says " added a comment") I changed the translation of " added a comment" to the empty string. The problem with that is that the same translation is apparently also used in the UI. I’ll undo that configuration. Let me know if the " did " comments get too annoying and I’ll figure something out.

Great, thanks!

Hi Manuel,

Thanks for the update it has some nice new features. There is one issue I am seeing since the update though. I am the reviewer on http://reviews.llvm.org/D10386 but I didn’t get any email for the updates since my entry on July 1. Do you know what’s going on? We had some issues recently with Phab emails being delayed on Apple mail servers but these emails seems to have completely disappeared.

Thanks,
Adam

You were in sendgrid’s bounce list (there must have been at least one bounce from your email to the apple servers). Please let me know if other people had similar problems.

Ok, found more bounces - removed them all, things should work again…

You were in sendgrid’s bounce list (there must have been at least one bounce from your email to the apple servers). Please let me know if other people had similar problems.

Ah, lovely :(. Thanks for the quick fix.