[GHC DevOps Group] The future of Phabricator

[Ben Gamari]

Herbert Valerio Riedel <hvriedel at gmail.com> writes:

> On 2018-10-30 at 00:54:42 -0400, Ben Gamari wrote:
>> TL;DR. For several reasons I think we should consider alternatives to
>>        Phabricator. My view is that GitLab seems like the best option.
>>
>>
>> Hello everyone,
>>
>> Over the past year I have been growing increasingly weary of our
>> continued dependence on Phabricator. Without a doubt, its code review
>> interface is the best I have used. However, for a myriad of reasons I
>> am recently questioning whether it is still the best tool for GHC's
>> needs.
>
> TL;DR. IMO, Phabricator is still the best tool we know about for *GHC's needs* ;-)
>
> [...]
>
>> For one, at this point we have no options for support in the event that
>> something goes wrong as the company responsible for Phabricator,
>> has closed their support channels to non-paying customers.
>
> While it's certainly a good idea to have an emergency plan ready, I
> don't think we need to act prematurely before something has actually
> happened. Phabricator is open-source and therefore there's little that
> can go so catastrophically wrong that we wouldn't give us more than enough
> time to act upon.
>
My point is that we have already had more than one issue that has cost
significant time to resolve. Most recent is the Harbormaster issue,
which lead to me having to drop everything to fix Harbormaster, which
broke as a result of merging Hadrian [1]. I spent an hour trying to
identify the issue and another hour trying to find a workaround before
deciding to give up and just try deploying the CircleCI/Phabricator
bridge that we had waiting to be deployed. Unfortunately, I then
encountered another Harbormaster bug [2], which I still haven't worked
out. After a couple of hours of scratching my head at this one I took to
writing this email.


[1] Harbormaster attempts to reuse working directories but does an
    inadequate job of cleaning stale submodules. This results in
    the any post-merge builds failing at `git checkout`.

[2] Harbormaster "Send HTTP Request" build step nondeterministically
    resets connections after receiving valid responses, failing with the
    less-than-descriptive error message of "HTTP 28". I still have no
    idea why.


> (Also, there's still the option of becoming part of that
> paying-customers group and thus influence their focus -- after all, we'd
> be contributing to a improving an OSS codebase; and not a proprietary
> closed product such as GitHub)
>
At this point I'm not convinced that the benefits that Phabricator
brings us are worth the significant expense that doing this would incur,
especially.

>> Furthermore, in the past year or two Phacility has been placing their
>> development resources in the parts their customers pay them for, which
>> appear to be much different that the parts that we actively use. For
>> this reason, some parts that we rely on seem oddly half-finished.
>>
>> This concern was recently underscored by some rather unfortunate
>> misfeatures in Harbormaster which resulted in broken CI after the
>> Hadrian merge and now apparent bugs which have made it difficult to
>> migrate to the CircleCI integration we previously prepared.
>>
>> Perhaps most importantly, in our recent development priorities survey
>> our use of Phabricator was the most common complaint by a fair margin,
>> both in case of respondents who have contributed patches and those who
>> have not. On the whole, past contributors and potential future
>> contributors alike have strongly indicated that they want a more
>> Git-like experience. Of course, this wasn't terribly surprising; this
>> is just the most recent case where contributors have made this
>> preference known.
>>
>> Frankly, in a sense it is hard to blame them. The fact that users need
>> to install a PHP tool, Arcanist, to contribute anything but
>> documentation patches has always seemed like unnecessary friction to me
>> and I would be quite happy to be rid of it.
>
> [...]
>
>> Indeed we have had a quite healthy number of GitHub documentation
>> patches since we started accepting them.
>
>
> While I do agree that Phabricator's impedance mismatch with Git idioms
> has bugged me ever since we started using it (I even started
> implementing https://github.com/haskell-infra/arc-lite as a proof of
> concept but ran out of time), I still consider some of its features
> unparalleled in PR-based workflows as provided by GitHub or GitLab.
>
> For example, to me the support for stacked diffs outweights any
> subjective inconvenience brought forward against Phabricator
>
While I agree that stacked diffs are fantastic (I have used this feature
often) and without match in the PR model, there are two problems with
this argument:

 1. In practice they are rarely used in GHC's case. There are a few
 reasons for this:

   * many patches are really a single atomic change or otherwise too
     small to benefit from 

   * contributors are often unaware of the feature, typically coming
     from a PR-centric model

   * maintaining (more specifically, rebasing) stacks of differentials
     is a real headache with arcanist, which leaves the entirety of the
     work to be performed manually by the user. This is a serious
     problem since long-lived, large patches are precisely when you want
     to use a stack.

   * Phabricator's CI model doesn't properly account for stacking (e.g.
     test the patch by applying each patch in succession to the base
     commit), meaning the manual steps described above don't even get
     properly checked by CI.

 2. With a slight change in thinking it's possible to get most of the
    benefit under a PR model. Namely, consider a PR to be a stack of
    differentials, with each commit being an atomic change in that stack.
    I have started using this model in another GHC-related project
    I have been working on and it works quite well, especially since
    GitLab has good support for reviewing commit-by-commit.

> https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/?fbclid=IwAR3JyQP5uCn6ENiHOTWd41y5D-U0_CCJ55_23nzKeUYTjgLASHu2dq5QCc0
>
> The PR workflow is perfectly well-suited for trivial documentation
> patches to a project; but that's for contributions that frankly are of
> minor importance; they're surely nice to have, but they're not the kind
> of contributions that are essential to GHC's long-term sustainability
> IMO.
>
> The reality IMO is that everybody tends to come up with this or that
> complaint about a tool which isn't their favorite one, but it's hardly a
> real barrier to contribution. In fact, I bet the majority of the people
> that now complain about phabricator not being GitHub will be vocally
> unhappy about having to create a GitLab account and that GitLab is not
> GitHub...
>
Indeed this is true; I fully suspect that even if we did move away from
Phabrictor there would still be detractor to whatever tool we choose.
However, I do expect that number to be a drastic reduction from the
number of we hear complaints about Phabricator.

Moveover, given the number of documentation contributions that
materialized since we started accepting pull requests I am hopeful that
a reduction in on-boarding friction will lead to an uptick in larger
contributions as well. No doubt it won't be as large as the
uptick in documentation contributions, but I think there is a non-zero
number of potential contributors who can do useful things around the
compiler who are currently scared away by Phabricator.

> Sure, by not trying to make everyone (specifically non-MVP contributors)
> happy we might loose some typo fixes or whatever, but do we really want
> to optimize the workflows for casual drive-by contributors which may
> contribute a couple of trivial patches and then never be seen again, at
> the expense of making life harder for what is already a complex enough
> task: managing and reviewing complex patches to GHC where it's paramount
> to use the best possible code-review facilities, and not shift the cost
> from contributors to the even more important people, the ones maintaing
> the projects as well as having intimate knowledge about the internals of
> GHCs (but unfortunately have a very tight time & cognitive time budget
> to spend on GHC, and which are the ones we really want to be able to
> review those patches with as little cognitive overhead as possible.
>
My point is that in my experience GitLab's review experience has been
close enough to Phabricator's functionality that it hasn't affected my
review productivity too much on patches of significant size.

There are of course things I miss in Phabricator:

 * MRs are still an approximation of stacked diffs

 * Differential's ability to leave comment on lines untouched by the
   patch under review

On the other hand, GitLab has features that Phabricator lacks,

 * the ability to merge a MR after CI finishes

 * Merge-after-CI-finishes

> So yes, phabricator is optimized for reviewers, and that's IMO a very
> good thing and outweights the benefit of trying to bend over backwards
> to make as many contributors as possible happy, of which there are
> orders of magnitudes more than there GHC maintainers&experts.
>
I really don't view this as a matter of optimising for contributors over
reviewers. As someone who does a significant amount of reviewing I have
found GitLab to be good enough. It isn't as aenemic as GitHub and has
gained most of the features that makes Differential usable (namely,
reasonable preservation of context in the face of rebasing, the ability
to diff iterations of an MR, a file tree interface to orient yourself in
a large patch during review, intelligent highlighting of differences
within a line, and transactional reviews). In practice I've not found
myself in terrible want for anything in particular when doing review on
GitLab.

Cheers,

- Ben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 487 bytes
Desc: not available
URL: <http://mail.haskell.org/pipermail/ghc-devs/attachments/20181030/7faac7d7/attachment.sig>