Launch of Phabricator and Lando for mozilla-central

272 views
Skip to first unread message

Mark Côté

unread,
Jun 6, 2018, 10:57:18 AM6/6/18
to
The Engineering Workflow team is happy to announce the release of Phabricator and Lando for general use. Going forward, Phabricator will be the primary code-review tool for modifications to the mozilla-central repository, replacing both MozReview and Splinter. Lando is an all-new automatic-landing system that works with Phabricator. This represents about a year of work integrating Phabricator with our systems and building out Lando. Phabricator has been in use by a few teams since last year, and Lando has been used by the Engineering Workflow team for several weeks and lately has successfully landed a few changesets to mozilla-central.

Phabricator is a suite of applications, but we are primarily using the code-review tool, called Differential, which will be taking the place of MozReview and Splinter. Bug tracking will continue to be done with Bugzilla, which is integrated with Phabricator. You will log into Phabricator via Bugzilla. We will soon begin sunsetting MozReview, and Splinter will be made read-only (or replaced with another patch viewer). An upcoming post will outline the plans for the deprecation, archival, and decommission of MozReview, with Splinter to follow.

I also want to thank Phacility, the company behind Phabricator, who provided both excellent support and work on Phabricator itself to meet our requirements in an exceptionally helpful and responsive way.

User documentation on Phabricator catered specifically to Mozillians can be found at https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html. It is also linked from within Phabricator, in the left-hand menu on the home page.

User documentation on Lando can be found at https://moz-conduit.readthedocs.io/en/latest/lando-user.html.

MDN documentation is currently being updated.

At the moment, Phabricator can support confidential revisions when they are associated with a confidential bug, that is, a bug with one or more security groups applied. Lando, however, cannot currently land these revisions. This is a limitation we plan to fix in Q3. You can follow https://bugzilla.mozilla.org/show_bug.cgi?id=1443704 for developments. See http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#landing-patches for our recommendations on landing patches in Phabricator without Lando.

Similarly, there are two other features which are not part of initial launch but will follow in subsequent releases:
* Stacked revisions. If you have a stack of revisions, that is, two or more revisions with parent-child relationships, Lando cannot land them all at once. You will need to individually land them. This is filed as https://bugzilla.mozilla.org/show_bug.cgi?id=1457525.
* Try support. Users will have to push to the Try server manually until this is implemented. See https://bugzilla.mozilla.org/show_bug.cgi?id=1466275.

Finally, we realize there are a few oddities with the UI that we will also be fixing in parallel with the new features. See https://bugzilla.mozilla.org/show_bug.cgi?id=1466120.

The documentation lists several ways of getting in touch with the Engineering Workflow team, but #phabricator and #lando on IRC are good starting points.

Boris Zbarsky

unread,
Jun 6, 2018, 11:18:43 AM6/6/18
to
On 6/6/18 10:57 AM, Mark Côté wrote:
> An upcoming post will outline the plans for the deprecation, archival, and decommission of MozReview, with Splinter to follow.

Just a quick question: will Bugzilla's "edit attachment as comment"
functionality remain?

> * Stacked revisions. If you have a stack of revisions, that is, two or more revisions with parent-child relationships, Lando cannot land them all at once.

Does Differential support this case now? I recall there were problems
around it at one point, but maybe they were fixed?

Which hg tree does Lando land on? Does it also use the autoland tree?

-Boris

Mark Côté

unread,
Jun 6, 2018, 11:48:45 AM6/6/18
to
On Wednesday, 6 June 2018 11:18:43 UTC-4, Boris Zbarsky wrote:
> On 6/6/18 10:57 AM, Mark Côté wrote:
> > An upcoming post will outline the plans for the deprecation, archival, and decommission of MozReview, with Splinter to follow.
>
> Just a quick question: will Bugzilla's "edit attachment as comment"
> functionality remain?

Good question. Probably, as it has different uses, but it shouldn't be used to work around Phabricator. :)

> > * Stacked revisions. If you have a stack of revisions, that is, two or more revisions with parent-child relationships, Lando cannot land them all at once.
>
> Does Differential support this case now? I recall there were problems
> around it at one point, but maybe they were fixed?

Differential has always supported this, just not quite as smoothly as MozReview. See http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#series-of-commits. We are still evaluating ways to make this smoother. Our ideal situation is to get this into upstream Phabricator; we're currently discussing this with Phacility. Failing that, we have a few options for tools we can build ourselves. More to come onthis.

> Which hg tree does Lando land on? Does it also use the autoland tree?

For mozilla-central, yes. Lando also supports other hg repos, where it will land directly.

Mark

Boris Zbarsky

unread,
Jun 6, 2018, 12:08:49 PM6/6/18
to
On 6/6/18 11:48 AM, Mark Côté wrote:
> Good question. Probably, as it has different uses, but it shouldn't be used to work around Phabricator. :)

A related question: How is Phabricator's offline support? This was a
continuous pain point for me with mozreview: it was basically impossible
to make any progress on a review while offline.

> Differential has always supported this, just not quite as smoothly as MozReview. See http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#series-of-commits.

Great!

-Boris

Mark Côté

unread,
Jun 6, 2018, 12:20:19 PM6/6/18
to
On Wednesday, 6 June 2018 12:08:49 UTC-4, Boris Zbarsky wrote:
> On 6/6/18 11:48 AM, Mark Côté wrote:
> > Good question. Probably, as it has different uses, but it shouldn't be used to work around Phabricator. :)
>
> A related question: How is Phabricator's offline support? This was a
> continuous pain point for me with mozreview: it was basically impossible
> to make any progress on a review while offline.

Another good question. :) Given that it is also a rich web app, I'm tempted to say "not awesome", but I'll have to try it out. If it is indeed not awesome, we can try to work with upstream to improve the situation.

Mark

Boris Zbarsky

unread,
Jun 6, 2018, 1:41:27 PM6/6/18
to
On 6/6/18 12:20 PM, Mark Côté wrote:
> Another good question. :) Given that it is also a rich web app, I'm tempted to say "not awesome", but I'll have to try it out. If it is indeed not awesome, we can try to work with upstream to improve the situation.

OK. To be clear, the offline support of "Edit attachment as comment" is
pretty good as these things go. As long as I have the diff open in a
tab before I go offline, I can review in a text editor (or directly in
the textarea if I loaded that page before going offline), paste it into
the textbox when I go back online, and no one can tell that there was
anything offline involved...

-Boris

Jan-Ivar Bruaroey

unread,
Jun 6, 2018, 1:41:37 PM6/6/18
to
On 6/6/18 11:48 AM, Mark Côté wrote:
> On Wednesday, 6 June 2018 11:18:43 UTC-4, Boris Zbarsky wrote:
>>> * Stacked revisions. If you have a stack of revisions, that is, two or more revisions with parent-child relationships, Lando cannot land them all at once.
>>
>> Does Differential support this case now? I recall there were problems
>> around it at one point, but maybe they were fixed?
>
> Differential has always supported this, just not quite as smoothly as MozReview. See http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#series-of-commits. We are still evaluating ways to make this smoother. Our ideal situation is to get this into upstream Phabricator; we're currently discussing this with Phacility. Failing that, we have a few options for tools we can build ourselves. More to come onthis.

That sounds like a no. I suspect a disconnect here:

I think bz is asking about mozReview's ability to handle multiple
commits in a single review (and handle updates in both dimensions). This
fits the hg evolve model well, and was AFAIK a unique workflow of mozReview.

That seems different from the ability to review a set of commits not
based straight off of central (e.g. on top of a local patch set under
different review). Even github allows that, but hardly scales past 2.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1457525#c3 it sounds
like all commits are squashed when pushed for review, and that
Differential just can't handle this complexity.

I for one will dearly miss mozReview's "evolve" design around reviewing
a whole set of broken-down commits, having done mozreviews with ~3-106
(!) commits in the past.

.: Jan-Ivar :.

Boris Zbarsky

unread,
Jun 6, 2018, 1:49:55 PM6/6/18
to
On 6/6/18 1:41 PM, Jan-Ivar Bruaroey wrote:
> I think bz is asking about mozReview's ability to handle multiple
> commits in a single review (and handle updates in both dimensions). This
> fits the hg evolve model well, and was AFAIK a unique workflow of
> mozReview.

This isn't actually what I was asking. I was asking whether there is
decent support for reviewing a stack of dependent patches, with the
ability to do per-patch interdiffs, have different reviewers for
different patches, etc. I wasn't really asking about the thing
mozreview does, because I don't use it myself, fwiw. But I sure do
upload commit series to Bugzilla.

Basically, I want some equivalent to "hg bzexport" or something better
but for the new setup. Based on
http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#series-of-commits
it sure sounds like that doesn't exist yet but is planned...

-Boris

Boris Zbarsky

unread,
Jun 6, 2018, 1:52:10 PM6/6/18
to
On 6/6/18 1:49 PM, Boris Zbarsky wrote:
> Basically, I want some equivalent to "hg bzexport" or something better

One example of "better" would be being able to see "the diff with the
first N patches applied" without having to do painstaking work. This is
something the "attach patches to Bugzilla" approach lacks. Mozreview
kinda sorta has it in that you can pull the whole thing into a local
tree and diff locally, but it's not as nice as it could be.

-Boris

Chris AtLee

unread,
Jun 6, 2018, 2:43:05 PM6/6/18
to Côté, Mark, dev-platform
This is really great news, I'm really excited to start using it!

Automated landings from code review is such a game changer for
productivity and security.

Congrats to everyone involved.

Cheers,
Chris
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

Jan-Ivar Bruaroey

unread,
Jun 6, 2018, 2:52:44 PM6/6/18
to
On 6/6/18 1:49 PM, Boris Zbarsky wrote:
> On 6/6/18 1:41 PM, Jan-Ivar Bruaroey wrote:
>> I think bz is asking about mozReview's ability to handle multiple
>> commits in a single review (and handle updates in both dimensions).
>> This fits the hg evolve model well, and was AFAIK a unique workflow of
>> mozReview.
>
> This isn't actually what I was asking.  I was asking whether there is
> decent support for reviewing a stack of dependent patches, with the
> ability to do per-patch interdiffs, have different reviewers for
> different patches, etc.  I wasn't really asking about the thing
> mozreview does, because I don't use it myself, fwiw.

Sorry, I shouldn't have assumed. FWIW mozReview handles all this
exceedingly well today IMHO, using the "Diff Revision" slider, even for
the whole patch set.

> One example of "better" would be being able to see "the diff with the first N patches applied"

Sounds exactly like what mozReview's "Diff Revision" slider does today.

(as long as you never rebase your patches during review. Its INTER-diff
fails to weed out any new unrelated changes then, a longstanding bug
that sadly never got fixed).

> Basically, I want some equivalent to "hg bzexport" or something better
> but for the new setup.  Based on
> http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#series-of-commits
> it sure sounds like that doesn't exist yet but is planned...

I guess I fail to see how that can be retrofitted... but what do I know.

.: Jan-Ivar :.

Boris Zbarsky

unread,
Jun 6, 2018, 3:03:53 PM6/6/18
to
On 6/6/18 2:52 PM, Jan-Ivar Bruaroey wrote:
> Sorry, I shouldn't have assumed. FWIW mozReview handles all this
> exceedingly well today IMHO, using the "Diff Revision" slider, even for
> the whole patch set.

Sort of. As long as people don't mess up their mozreview ids....

>> One example of "better" would be being able to see "the diff with the
>> first N patches applied"
>
> Sounds exactly like what mozReview's "Diff Revision" slider does today.

No, not at all.

Say we have a tree at revision B. Someone writes three changesets P1,
P2, P3 on top of it and asks me for review.

Mozreview will show me the equivalent of "diff -r B -r P1", "diff -r P1
-r P2" and "diff -r P2 -r P3". If there are then edits to P1 to produce
Q1, the diff revision slider will let me choose between "diff -r B -r
Q1" and "diff -r P1 -r Q1".

But what I'm talking about is viewing "diff -r B -r P2" or "diff -r P1
-r P3". I don't believe mozreview provides this right now via its UI.

> (as long as you never rebase your patches during review. Its INTER-diff
> fails to weed out any new unrelated changes then, a longstanding bug
> that sadly never got fixed).

It used to try to weed them out, but that produced interdiffs that
weeded out actual changes. This is a hard problem....

>> Basically, I want some equivalent to "hg bzexport" or something better
>> but for the new setup.  Based on
>> http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#series-of-commits
>> it sure sounds like that doesn't exist yet but is planned...
>
> I guess I fail to see how that can be retrofitted... but what do I know.

Well... it sounds like right now you can do something in Differential
with dependent patches but you have to manually indicate they are
dependent. This is the part that could be automated/retrofitted.

Whether the dependent patches thing does the other stuff one would want
is not not clear to me.

-Boris

Jan-Ivar Bruaroey

unread,
Jun 6, 2018, 3:18:43 PM6/6/18
to
On 6/6/18 3:03 PM, Boris Zbarsky wrote:
> On 6/6/18 2:52 PM, Jan-Ivar Bruaroey wrote:
> Mozreview will show me the equivalent of "diff -r B -r P1", "diff -r P1
> -r P2" and "diff -r P2 -r P3".  If there are then edits to P1 to produce
> Q1, the diff revision slider will let me choose between "diff -r B -r
> Q1" and "diff -r P1 -r Q1".
>
> But what I'm talking about is viewing "diff -r B -r P2" or "diff -r P1
> -r P3".  I don't believe mozreview provides this right now via its UI.

Ah, you want a vertical slider as well, so to speak, orthogonal to Diff
Revision.

Thanks for explaining!

.: Jan-Ivar :.

Mark Côté

unread,
Jun 6, 2018, 4:07:22 PM6/6/18
to
No, at the moment you would have to pull them down with "arc patch" to see part or all of a stack of revisions applied together. I'll ask upstream if they have any plans to implement this in the UI.

Mark

Xidorn Quan

unread,
Jun 6, 2018, 7:58:43 PM6/6/18
to dev-pl...@lists.mozilla.org
On Thu, Jun 7, 2018, at 12:57 AM, Mark Côté wrote:
> Phabricator is a suite of applications, but we are primarily using the
> code-review tool, called Differential, which will be taking the place of
> MozReview and Splinter. Bug tracking will continue to be done with
> Bugzilla, which is integrated with Phabricator. You will log into
> Phabricator via Bugzilla. We will soon begin sunsetting MozReview, and
> Splinter will be made read-only (or replaced with another patch viewer).
> An upcoming post will outline the plans for the deprecation, archival,
> and decommission of MozReview, with Splinter to follow.

Does using Phabricator still require using the PHP client? I recall gps mentioned before that there was a plan to integrate it into hg or mach instead of requiring developers to install another runtime and client. How is that going on?

If the plan is that everyone should just install the PHP runtime and use that client, maybe MozillaBuild should include PHP (installing it on Linux and macOS should be included in mach bootstrap I guess).

- Xidorn

James Graham

unread,
Jun 7, 2018, 8:32:51 AM6/7/18
to dev-pl...@lists.mozilla.org
On 06/06/2018 15:57, Mark Côté wrote:

> Similarly, there are two other features which are not part of initial launch but will follow in subsequent releases:
> * Stacked revisions. If you have a stack of revisions, that is, two or more revisions with parent-child relationships, Lando cannot land them all at once. You will need to individually land them. This is filed as https://bugzilla.mozilla.org/show_bug.cgi?id=1457525.

Have we considered the impact this will have on our CI load? If we
currently have (say — I didn't bother to compute the actual number) an
average of 2 commits per push, it seems like this change could increase
the load on inbound by a corresponding factor of 2 (or perhaps less if
the multiple-final-commit workflow is so bad that people start pushing
fewer, larger, changes).

Mark Côté

unread,
Jun 7, 2018, 9:13:32 AM6/7/18
to
Yes, it still requires PHP, and yes, I'd like to bundle it. This depends, though, on how we end up improving commit-series support.

Mark
Reply all
Reply to author
Forward
0 new messages