Phabricator for patches and code review

Sat Jun 7 08:17:43 UTC 2014

Well, I'm convinced now that you have researched this thoroughly. Thanks 
for doing so and addressing mine and Manuel's concerns.

Cheers,
Arash

On 2014-06-07 09:24, Austin Seipp wrote:
> This always gets brought up, but I (still) think there are several
> reasons to prefer our own infrastructure over GitHub:
>
>   - Phab is far more flexible, especially for review. GitHub doesn't
> even have side-by-side diffs (a massive improvement), much less the
> suite of tools that make code review easy. I cannot set emails to
> notify me when someone touches something I am the owner of, for
> example, it's just blind emails all the time. This applies to incoming
> patches (pre-commit review), and changes merged to the tree
> (post-commit review). And both of these workflows are useful ones for
> GHC I think.
>
>   - I also now think it is *easier* to submit changes for review with
> Phabricator having done it, because there is a simple command line
> utility to do so. This utility can easily be integrated into the GHC
> tree. It is also easier for me to manage patches, or for anyone to
> merge patches! It's just one more command.
>
>   And the reality is that most commits need to be validated before we
> commit them. This has been the standard for a long time - GitHub's
> "automated workflows" don't accommodate this. I would rarely, if ever,
> hit the "merge button" on GitHub for GHC for example. You must run the
> tests for foreign patches you are incorporating - always.
>
>   - I don't think maintenance is an issue. We've been using it for
> Haskell.org ticket tracking since we needed a replacement for some of
> our old infrastructure (including private support tickets which GitHub
> does not support), since we're consolidating it all. The admin team
> has about 3 people working on it (including me), and we've been doing
> upgrades, migrating things, automating them, and generally making the
> servers more redundant and simpler.
>
>   - I think code review, in general, massively increases the 'shared
> knowledge' pool for developers, and a dedicated tool really, really
> helps. I can only point to my experience introducing dedicated tools
> multiple times in my career in the past as examples. Most people
> besides me don't really review patches unless I ask them to, and a
> good tool of pending things that can notify you if you might be
> interested is really useful.
>
> As for GitHub and Trac in general:
>
>   - GitHub lacks several things we already use. For example, there is
> no way to add commit hooks to repositories that ban commits containing
> whitespace, trailing spaces, and other 'lint' errors. git.haskell.org
> automatically enforces this to help keep new code tab-free. GitHub has
> no alternative to this.
>
>   - We also use this facility to keep submodules sane: as of today,
> git.haskell.org will not let you commit a 'dangling submodule
> reference' to ghc.git. You must push the corresponding submodule code
> first, so the top-level repository never breaks. This is also not
> possible with GitHub and has been a historical error source for
> developers.
>
>   - Speaking of builds, any kind of integration with a CI system, at
> some level, is going to require custom infrastructure on our side,
> GitHub or not, so there's no clear advantage here. Travis-CI is simply
> not going to be long-term acceptable for GHC - we are within 5m or so
> of the build limit, and it only builds GHC with some conservative
> settings. Also Travis-CI does not allow custom infrastructure, like
> ARM buildbots, or multiple differently-versioned OS X buildbots.
> Gabor's nightly servers for example allow all of this. Joachim's
> builds are very useful though, but we need more.
>
>   - Speaking of that, we need to maintain our own server so that Git
> pushes can interface with Trac, and the mailing notifier.
>
>   - And then you might ask, well we could just migrate it all to
> GitHub. That's a _huge_ amount of work. GHC is probably one of the
> largest Trac installations around at nearly 10,000 tickets, a gigantic
> wiki, and tons of metadata and a *lot* of users. Preserving the
> necessary metadata, rewriting intra-wiki links, references, and
> preserving everything is just going to be a ton of work. GitHub
> doesn't even have an 'import' facility for example, just the raw API,
> so this would involve a lot of local processing to 'fix' everything.
> This includes moving labels, etc.
>
>   I strictly shot down that idea, because it's time I don't think
> anyone wants to dedicate to it. Dropping all the data is out of the
> question. Trac is well understood at least and has many valuable
> aspects, and a migration should be considered a very, very serious
> matter after a lot more discussion. Phabricator would only be used for
> GHC code review.
>
> On Sat, Jun 7, 2014 at 12:21 AM, Manuel M T Chakravarty
> <chak at cse.unsw.edu.au> wrote:
>> So, why not put everything on GutHub and use pull requests and so on?
>>
>> SimonM writes that Phabricator is better than GitHub. I’m happy to believe that, but he also writes that using it requires installing local software and quite a bit of work. Moreover, I like to add that lots of people already know how to use GitHub and probably few know Phabricator.
>>
>> So, we are talking about having a somewhat better tool in return for three very significant disadvantages: (1) local installation, (2) work to set up and maintain Phabricator, and (3) effort by many people to learn to use it.
>>
>> We also have a constant lack of sufficient men power. So, why spend effort on building our own infrastructure, which will only increase the hurdle for contributors (as they have to deal with an unknown system)? Let’s outsource the effort to GitHub.
>>
>> Manuel
>>
>> Simon Peyton Jones <simonpj at microsoft.com>:
>>> At the moment GHC's main sources aren't on github, which means that that (in my highly imperfect understanding) people can't submit pull requests or use their code review mechanisms.  Moreover, most people don't have commit rights on the main GHC server, so if someone wants to offer a patch they can really only do so in textual form attached to Trac.  People with commit rights can make a branch, but there's a danger that over a decade we'll accumulate zillions of dead branches which people forgot to delete.  I think on github the branch is in a different repo, belonging to the patch author.
>>>
>>> So we really don't have a good work flow for creating, reviewing, modifying, and finally apply patches.  I am no expert on these matters. If Phabricator would help with that I'm all for it.  But perhaps there are other alternatives?  Or is Phab the lead thing.  Will it stay around?
>>>
>>> Also before going too far I'd really like someone to document the workflow carefully, and make sure it works from Windows equally well.
>>>
>>> I'm not too stressed out about losing the review trail of a patch.  Much of it will be commenting on stuff that no longer appears in the final patch.  Anything that's important should appear in a Note in the source code; even the commit messages are invisible until you really start digging.
>>>
>>> Simon
>>>
>>> | -----Original Message-----
>>> | From: ghc-devs [mailto:ghc-devs-bounces at haskell.org] On Behalf Of Austin
>>> | Seipp
>>> | Sent: 06 June 2014 05:06
>>> | To: ghc-devs at haskell.org
>>> | Subject: RFC: Phabricator for patches and code review
>>> |
>>> | Hello all,
>>> |
>>> | Recently, while doing server maintenance, several of the administrators
>>> | for Haskell.org set up an instance of Phabricator[1], located at
>>> | https://phabricator.haskell.org
>>> |
>>> | For those who aren't aware, Phabricator (or "Phab") is a suite of tools
>>> | for software development. Think of it like a polished, semi-private
>>> | GitHub with a lot of applications and tools for all kinds of needs.
>>> | We've been using it to do issue tracking for Haskell.org maintenance and
>>> | like it a lot so far.
>>> |
>>> | One very nice aspect of Phabricator though is it has a very nice code
>>> | review tool, called 'Differential', that is very useful. For people who
>>> | have used a tool like Review Board, it's similar. Furthermore, it has a
>>> | very convenient userland tool called 'Arcanist' which makes it easy for
>>> | newcomers to post a review and get it merged when it's ready all from
>>> | the command line.
>>> |
>>> | I'd like to see if people are interested in using Phab _strictly_ for
>>> | code review of GHC patches. It is a dedicated tool specifically for
>>> | this, and I think it works much better than Trac or inline GitHub
>>> | comments.
>>> |
>>> | Also, Phab can also support post-commit reviews. So if I touch something
>>> | in the runtime system and just push, perhaps Simon or Edward would like
>>> | to look, and they can be alerted right when I do this, and then yell if
>>> | I did something stupid.
>>> |
>>> | Before I go much further, I'd like to ask: is there *any* interest in
>>> | this? Or are people satisifed with Trac? The primary motivations are
>>> | roughly, in no particular order:
>>> |
>>> |  1) Code review is good for everyone, a good way for people to learn the
>>> | code and ask questions, and useful to give feedback to newcomers.
>>> | And even experienced GHC hackers can learn things from reading code, as
>>> | we all do regularly, or find things that need cleanup.
>>> |
>>> |  2) Phabricator in particular makes it very easy to submit patches for
>>> | review. To submit a patch, I just run the command 'arc diff' and it Does
>>> | The Right Thing. It also makes it easy to ensure people are
>>> | *alerted* when a patch might be relevant to them.
>>> |
>>> |  3) They can be uploaded and created from the command line, and merged
>>> | easily afterwords the same way. This is particularly useful for
>>> | newcomers, and for me. :)
>>> |
>>> |  4) Differential is dedicated to code review, and much better at it than
>>> | just reading patches on Trac IMO.
>>> |
>>> |  5) It supports both post-commit code review, as well as pre-commit
>>> | review. Post commit would be especially useful for us too, I think.
>>> |
>>> | Point #2 and #3 are mostly relevant for me, because I mostly handle
>>> | incoming patches. But I think in general it would be nice, and make it a
>>> | lot easier for newcomers to submit patches, and us to look over them.
>>> |
>>> | Here's an example of a Differential code review:
>>> |
>>> | https://phabricator.haskell.org/D4
>>> |
>>> | This is a demo using my 'wip/ermsb' patch. You'll need to create an
>>> | account to login, but it shouldn't be much trouble, you can login
>>> | several ways. I'll fix the login requirement soon. Feel free to read the
>>> | code, comment on it, and play around. It's more of a demonstration, but
>>> | real code review would be welcome too. :)
>>> |
>>> | If people are interested in doing this, I can add notes to the wiki
>>> | pages for newcomers, and I'll send another email about Phab so people
>>> | can understand it a little better. But I want to ask first.
>>> |
>>> | There is an argument that our team is so small, code review has
>>> | unnecessary burdens. But I think Phab could help a lot with tracking
>>> | outside patches and getting good reviews for incoming patches, and it'll
>>> | make it easier for newcomers. And experienced pros can probably learn a
>>> | thing as well.
>>> |
>>> | Again, to be clear, I don't propose we migrate anything to Phabricator
>>> | from, say, Trac. There's no real pressure to do so and it would be tons
>>> | of work. I only propose we use it for code review, which is perfectly
>>> | fine, and how other projects like LLVM do code review (they use
>>> | Bugzilla).
>>> |
>>> | I also don't think the usage of Phabricator should be mandatory (unless
>>> | we decide that later because we like it), but I would like to see people
>>> | use it if possible.
>>> |
>>> | [1] http://phabricator.org
>>> |
>>> | --
>>> | Regards,
>>> |
>>> | Austin Seipp, Haskell Consultant
>>> | Well-Typed LLP, http://www.well-typed.com/
>>> | _______________________________________________
>>> | ghc-devs mailing list
>>> | ghc-devs at haskell.org
>>> | http://www.haskell.org/mailman/listinfo/ghc-devs
>>> _______________________________________________
>>> ghc-devs mailing list
>>> ghc-devs at haskell.org
>>> http://www.haskell.org/mailman/listinfo/ghc-devs
>>
>
>