Notes from Ben's "contribute to ghc" discussion

[Michael Sloan]

On Mon, Sep 26, 2016 at 12:40 PM, Simon Marlow <marlowsd at gmail.com> wrote:
> On 26 September 2016 at 20:13, Ben Gamari <ben at smart-cactus.org> wrote:
>>
>> Simon Marlow <marlowsd at gmail.com> writes:
>>
>> > I would rather we *didn't* accept contributions via github, even for
>> > small
>> > patches, and instead put more effort into streamlining the Phabricator
>> > workflow.
>> >
>> >
>> >    - Adding another input method complicates the workflow, users have to
>> >    decide which one to use
>> >
>> I think we would want to try to sell the GitHub route as "if you would
>> like to contribute then we would strongly prefer you use Phabricator,
>> but if you must and it's a small patch, we will accept it via GitHub."
>
>
> But this is opening the floodgates a crack... how do we know what a "small"
> patch is?  What happens when someone submits a patch that's too large?  The
> patches will get larger, we'll have to do code reviews on two different
> tools, and it will be really hard to go back.  I just have a bad feeling
> about this.

I agree that this would certainly happen - people would get used to
the new workflow and want to use it for large patches.  I've said
similar things in a post I made to Carter's thread about a
ghc-simple-patch-propose list.  Please consider this approach:

If a patch requires substantial revision, move it to Phabricator.  How
do we know when it requires substantial revision?  There are two
conditions for this:

1) When it is clear to the reviewer that it will take multiple iterations

2) After a single iteration of review has occurred on GitHub, and
there is still more to do.

This way, any substantial review process will be captured as a differential.

We need a way to make this as easy and efficient as possible for
maintainers and contributors alike.  In particular, maintainer /
reviewer time is very valuable.  As such, I propose the following:

1) A tool called "ghc-hub", which supports "ghc-hub migrate URL", to
migrate a PR to a Differential, without migrating any review metadata
(to keep the implementation simple).  It would also support "ghc-hub
merge URL", .  Using PR URLs on the commandline like this is inspired
by github's hub tool - https://github.com/github/hub .

2) Eventually, have a GitHub bot which does the following:

  * Notices if a patch is particularly large, and recommends using the
"ghc-hub" tool to perform the migration.

  * Notices if a patch has already gone through a review and hasn't
been merged for a while, and automatically migrates it to a
differential.

Note that having these tools ready is not necessary to begin this new
workflow, but I think they will be helpful for making this new
workflow a sustainable approach.

If you guys are on board with this approach, I would enjoy writing the
initial version of "ghc-hub". Though I cannot at this time guarantee
that I would be willing to entirely champion / maintain it - I'll need
some help.

>> >    - Github is not integrated with our other infrastructure, while
>> >    Phabricator is
>> >
>> True, but I suspect for the small documentation patches that we are
>> currently consider this shouldn't matter so much.
>>
>> >    - Mutliple sources of contributions makes life harder for maintainers
>> >
>> It does certainly put yet another task on our plates, but I would argue
>> that it's actually easier than accepting patches via Trac, which we
>> already do.
>
>
> We should stop accepting patches via Trac too :)
>
> Cheers
> Simon
>
>
>
>>
>> >    - I also like the idea of auto-push if validate succeeds.  Or a
>> > button
>> >    that you can press on the diff that would do the same thing, so you
>> > can get
>> >    code review first.
>> >
>> To be clear, I'm a bit weary of opening up the auto-push feature to new
>> contributors. While regular contributors know what changes can be safely
>> pushed and which require review, we have no guarantee that a new
>> contributor has developed these sensibilities.
>>
>> >    - +1 to making the manual easier to build.  The same goes for
>> > Haddocks;
>> >    it's really hard to make a simple patch to the docs and test it right
>> > now.
>> >
>> The users guide should be quite possible to do.
>>
>> I don't believe there is any reliable way to allow a contributor to
>> build the haddocks without having built GHC (since you need GHC master to
>> parse `base`, et al.); that being said, we could have Harbormaster
>> upload built documentation somewhere and then leave a link to it on the
>> Diff.
>>
>> > One other thing that came up but wasn't mentioned in the notes: let's be
>> > more prompt about reverting patches that break validate, even if they
>> > only
>> > break a test.  Now that we have better CI support, we can easily
>> > identify
>> > breaking patches and revert them.
>> >
>> Agreed.
>>
>> Cheers,
>>
>> - Ben
>>
>
>
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>