Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
On Wednesday, 6 June 2018 15:18:43 UTC-4, Jan-Ivar Bruaroey wrote: > 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. 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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 :. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 :. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 On Wed, 6 Jun 2018 at 11:01, Mark Côté wrote: > > 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. > > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 :. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Launch of Phabricator and Lando for mozilla-central
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 ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Launch of Phabricator and Lando for mozilla-central
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. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Coding style: brace initialization syntax
On Wednesday, June 6, 2018 at 5:21:05 AM UTC+3, gsqu...@mozilla.com wrote:
> On Wednesday, June 6, 2018 at 5:35:59 AM UTC+10, Boris Zbarsky wrote:
> > On 6/5/18 3:10 PM, Emilio Cobos Álvarez wrote:
> > > I personally would prefer one space at each side when using braces:
> > >
> > > , mFoo { 0 }
> >
> > I think the reason people tend to think of this as not wanting spaces is
> > that they are thinking of it as a constructor call. The parentheses
> > syntax for initializer lists helps think of things that way, of course.
> > [...]
> > -Boris
>
> I also prefer `m{ 0 }`.
>
> `m(0)` (direct initialization) and `m{ 0 }` (list initialization) are really
> different operations, and may in fact call different constructors -- E.g.,
> the latter will prefer constructors that take an std::initializer_list.
> So I think it is important to emphasize the difference, especially for
> reviewers to be able to pick on that difference.
>
> g.
I completely agree with your point regarding the {} list initialize, and also
the rest of opinions posted here.
If what Eric wrote is the common ground here, I will be more than happy to
update the the coding style doc.
I think we’ve started to use “{ }” list initialiser on a mass scale since it
was easier to implement the auto-fix for bug 525063.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Coding style: brace initialization syntax
On Fri, 13 Apr 2018 10:22:06 -0400, Boris Zbarsky wrote:
> On 4/13/18 9:37 AM, Emilio Cobos Álvarez wrote:
>> Would people agree to use:
>>
>> , mIsRootDefined { false }
>>
>> Instead of:
>>
>> , mIsRootDefined{ false }
>
> So my take is that we should not use braced initializer syntax in
> constructor initializer lists. The reason for that is that it
> makes it much harder to scan for where the constructor body
> starts. Doubly so when ifdefs in the initializer list are
> involved. Triply so when someone writes it as:
>
> explicit TTextAttr()
> , mIsRootDefined
> {
> false
> }
> #ifdef SOMETHING
> #endif
> {
> }
>
> which is what clang-format did in some of the cases in the patch
> for bug 525063.
>
> In particular, I think this should have just used:
>
> , mIsRootDefined(false)
One advantage of list-initialization over direct initialization is
that narrowing conversions are prohibited in list-initialization.
This is particularly useful in member initializer lists, where the
types are not readily visible (which is another good reason for
default member initialization).
I guess that doesn't matter for bool initializers, but consistency
...
Below, D will compile, but L will not.
struct D
{
int i;
D() : i(3.14) {}
};
struct L
{
int i;
L() : i{ 3.14 } {}
};
One advantage of lack of space between identifier and
brace-init-list is that it is visibly different from the space
before the constructor body.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
