Re: Notes from Ben's "contribute to ghc" discussion

On Mon, Sep 26, 2016 at 12:40 PM, Simon Marlow wrote:

On 26 September 2016 at 20:13, Ben Gamari wrote:

...

Simon Marlow 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@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs