Create a ghc-simple-patch-propose list? Re: Notes from Ben's "contribute to ghc" discussion

Wed Sep 28 02:20:28 UTC 2016

On Tue, Sep 27, 2016 at 6:49 PM, Moritz Angermann <moritz at lichtzwerge.de> wrote:
> Hi,
>
> I think it would be great if this was proposed formally. If we could
> integrate this with the improved ghc development proposal[1], this
> would be great! Or turn it into a separate proposal and remove the
> similar parts from the one mentioned.

Cool!  Should I do the integration?  I wouldn't mind if someone else
took these thoughts and integrated them, though I'd like to see how
they get integrated!

> However, on the topic, I’d like to share a few thoughts:
>
> Do we know how many of these GitHub Pull Requests we will actually
> receive? If we migrate *everything* over to phabricator, isn’t that
> just going to increase complexity for most of the minor pull request?

We don't know how many, hopefully lots, right?

What's the usual complexity for a minor patch to GHC - does it just
skip differential and end up getting committed directly to master?  In
that case, yes this would cause there to be a lot more differentials.
Is that a bad thing?  We could certainly consider something more like
the "ghc-hub" tool I've described elsewhere.  This would give more
direct control of what happens to GitHub PRs, but would also require
manual oversight of incoming GitHub PRs.

> If someone fixes a spelling mistake or adds a short comment to
> explain something in more detail, I see no reason why we need to employ
> any heavy machinery and duplication across different tools. These, so
> I would hope, would be trivially merged by a small team of ghc devs
> watching the GitHub PR, especially if we don’t even know how much PRs
> we will receive through GitHub.
>
> On the other hand, if a PR is non-trivial (yes this is a bit of an
> ambiguous term), I’m in favor of asking the person who issued the PR
> to open an account with Phabricator[2], and run `$ arc diff origin/master`
> on his branch, or let us know that he considers this too much effort
> (and is willing to have someone else take over, should there be interest).

> The reasoning behind this is the following: first of all it’s the persons
> work, and unless someone else is willing to take it through the review
> process, this is going to end up being a dead diff in phabricator which
> adds additional overhead there. If there is not enough interest to follow
> a simple few steps document on how to push a non-trivial patch to
> phabricator, can we expect there to be sufficient interest in actually
> following up on the review process?
>
> I see this more as an onboarding problem than a continuous development
> issue. Those who are in the phabricator workflow, will likely just
> continue using it, no? I certainly would! However I might be browsing
> ghc source on GH just because I don’t have that code locally available
> right now and stumble on a spelling mistake, quickly use the editor
> provided by GH and submit one of these trivial PRs.

Exactly!  GH has great support for creating trivial PRs.  It is also a
workflow people are familiar with.  As soon as you ask people to
install a piece of software and learn to use it (arcanist), there is
immediately a cognitive barrier.  Even if the software is easy to
learn.  I recall being confused about arcanist squashing, for example.
I haven't used it in a long time, so I don't recall details.

I can see the attitude of "Well, if you're hacking on GHC, you are
probably capable of learning some new tools."  This is true, but this
stuff is adding cognitive load atop the existing cognitive load of
creating a patch for GHC.

> Those who haven’t used phabricator yet, but are accustomed to the GH
> PR Flow, would start there and then be gently guided towards using
> phabricator and arc. We can do much better at explaining and showing
> the phabricator and arc workflow.

Yup!  That's how I see this as well.  It is leveraging GH PRs as an
easy way to attract contribution, but also guiding people towards
Phabricator.  As things stand, you need to learn both Phabricator and
Arcanist at the same time.  This way, we can start with onboarding
people with phabricator, and later get them to start using archanist
directly.  Some won't make it to that stage, but it will still be
valuable to get GH PRs from them!

> In essence I’d like us see accepting GH Pull Requests and then do
> more data-driven on-demand incremental improvements, instead of trying
> to go all out with excessive tooling.

This is a wise approach!  I think it makes sense to try the manual
labor version of this process before automating it.  Automation should
be on the minds of those doing the work, though, and they can be
reassured that it will get easier once automation is in place.

> Cheers,
>  Moritz
>
>
> [1]: https://github.com/ghc-megacorp/ghc-proposals/blob/improved-ghc-development/proposals/0000-improved-ghc-development.rst
> [2]: See the contributing to ghc section in [1].
>> On Sep 28, 2016, at 5:32 AM, Michael Sloan <mgsloan at gmail.com> wrote:
>>
>> You're welcome Richard!  I look forward to helping make it happen.  In
>> the other thread, Alexander Vershilov mentioned that we might instead
>> consider the following more straightforward workflow:
>>
>> 0) Have a bot that watches github for PRs.
>> 1) Submit whatever you want to github as a PR.
>> 2) It will be automatically closed and migrated to Phabricator.  I
>> would like it to automatically create a Phabricator account if you do
>> not already have one.  The message from the bot will tell you about
>> this action, and explain how to log in, perhaps even linking to
>> resources about Phabricator.
>>
>> Is this worth it?  I think it is for the one-off cases.  However, you
>> will have to be prepared that this means that people won't have
>> arcanist setup, and therefore are less likely to actually iterate on
>> their PR.  Perhaps we should extend this to the following:
>>
>> 3) Subsequent pushes to the branch for the PR will update the
>> Phabricator differential as if you had pushed via Arcanist.  I think
>> with this in place, we would have a fully streamlined system that
>> allows people to use their familiar GitHub workflows, without needing
>> to learn Arcanist.  Interactions would then still occur on , of
>> course.
>>
>> This way, GHC HQ doesn't even need to learn to use this new "ghc-hub"
>> tool!  Could name the bot that, though!
>>
>> Thoughts?  I think it would be great for this to be proposed formally
>> soon so that we can make it happen.  I am eager to be able to use my
>> normal git workflows, as my little experience with Arcanist induced
>> some head-scratching.  Not the fault of the tool, just a result of
>> lack of familiarity.
>>
>> -Michael
>>
>> On Tue, Sep 27, 2016 at 8:46 AM, Richard Eisenberg <rae at cs.brynmawr.edu> wrote:
>>> To sum up, this proposes the following:
>>>
>>> 1. Allow PRs on GitHub.
>>>
>>> 2. Michael Sloan to write a new utility, ghc-hub, which automates tasks interfacing between GitHub and Phab. This utility would be used only by GHC HQ and not by contributors.
>>>
>>> 3. Small GitHub PRs can be merged directly, by ghc-hub.
>>>
>>> 4. Larger GitHub PRs can be migrated to Phab by ghc-hub. The contributor would be issued a polite email explaining how to set up a Phab account to continue to follow their contribution.
>>>
>>> Have I captured this accurately? If so, a resounding +1 from me. I’ve wanted exactly this for a while.
>>>
>>> Is this worth sending through ghc-proposals?
>>>
>>> Thanks for volunteering item (2), Michael!
>>>
>>> Richard
>>>
>>> -=-=-=-=-=-=-=-=-=-=-
>>> Richard A. Eisenberg
>>> Asst. Prof. of Computer Science
>>> Bryn Mawr College
>>> Bryn Mawr, PA, USA
>>> cs.brynmawr.edu/~rae
>>>
>>>> On Sep 26, 2016, at 11:09 PM, Manuel M T Chakravarty <chak at justtesting.org> wrote:
>>>>
>>>> Sounds like a great idea to me and might alleviate SimonM’s concerns about fragmentation of dev attention.
>>>>
>>>> Manuel
>>>>
>>>>> Michael Sloan <mgsloan at gmail.com>:
>>>>>
>>>>> Argh, sent too soon.  The first paragraph, revised:
>>>>>
>>>>> This sounds like an ideal solution, Ben!  As has been discussed many
>>>>> times before, GitHub has many users familiar with its interface.  By
>>>>> allowing GitHub PRs, the initial contribution barrier will be lowered. If
>>>>> there is an easy and straightforward process for shifting big patches
>>>>> to Phabricator, then people who are regularly contributing via GitHub
>>>>> PRs can be incrementally on-boarded to the Phabricator / Arcanist
>>>>> workflow.
>>>>>
>>>>> On Mon, Sep 26, 2016 at 12:07 PM, Michael Sloan <mgsloan at gmail.com> wrote:
>>>>>> This sounds like an ideal solution, Ben!  As has been discussed many
>>>>>> times before, GitHub has many users familiar with its interface.  By
>>>>>> allowing GitHub PRs, the initial contribution
>>>>>>
>>>>>> I think it would be acceptable for larger GitHub PRs to have some
>>>>>> automated boilerplate response.  Ideally this would look like:
>>>>>>
>>>>>> """
>>>>>> Thanks for making this patch!  I've turned this into a Phab
>>>>>> Differential xxx and closed this PR.  Please create a differential
>>>>>> account associated with your email address ..."
>>>>>> """
>>>>>>
>>>>>> The email address can be automatically pulled from commit metadata.
>>>>>> If one is absent, then this automated process isn't possible.  If it
>>>>>> is present and
>>>>>>
>>>>>> So, I'm imagining a utility that interfaces between both GitHub and
>>>>>> Phab,allowing the following commands:
>>>>>>
>>>>>> * "ghc-hub migrate https://github.com/ghc/ghc/pull/1" - migrates the
>>>>>> patch to differential.  It may attempt to migrate body and title of
>>>>>> the initial post, but lets not bother with migrating any review data.
>>>>>>
>>>>>> * "ghc-hub merge https://github.com/ghc/ghc/pull/1" - merges the
>>>>>> patch.  This is used for merging small patches.  It would not do an
>>>>>> automated push.  Maybe have "--push" also perform the push?  So like
>>>>>> if you are on master, then "ghc-hub merge
>>>>>> https://github.com/ghc/ghc/pull/1 --push" would merge the patches and
>>>>>> push to master.
>>>>>>
>>>>>> How does this sound?  I like the idea a lot, and would enjoy helping
>>>>>> with implementation, time permitting.  I could possibly start hacking
>>>>>> on it if others give the go ahead of "Yes, lets do that".
>>>>>>
>>>>>> -Michael
>>>>>>
>>>>>> On Mon, Sep 26, 2016 at 11:45 AM, Ben Gamari <ben at smart-cactus.org> wrote:
>>>>>>> Carter Schonwald <carter.schonwald at gmail.com> writes:
>>>>>>>
>>>>>>>> In writing the following huge wall of text, I had and idea that I think
>>>>>>>> many folks would find palatable:
>>>>>>>>
>>>>>>>> What if simple small patches (such as hypothetical drive by doc patches )
>>>>>>>> had a mailing list where folks could email the simple / small patches as
>>>>>>>> email attachments plus a body text that summarizes the patch, what it does,
>>>>>>>> and why it's simple!
>>>>>>>>
>>>>>>> I completely agree that for small (e.g. documentation) patches our
>>>>>>> current system is quite heavy. For this reason I suggested at ICFP that
>>>>>>> we simply begin accepting small patches via GitHub pull requests.
>>>>>>> Frankly, this is less work for me than merging patches from a mailing
>>>>>>> list and I believe many users feel that GitHub is more accessible than a
>>>>>>> mailing list.
>>>>>>>
>>>>>>> The problem of course is what subset of patches do we want to allow to
>>>>>>> be taken via GitHub. My suggested answer to that is any patch which, if
>>>>>>> I were to write it myself, I would feel comfortable pushing directly to
>>>>>>> the tree.
>>>>>>>
>>>>>>> Then there is the question of what do we do with pull requests opened
>>>>>>> which do not satisfy this criterion. In this case I would likely open a
>>>>>>> Phabricator Differential with the pull request and close the pull
>>>>>>> request with a link to the Diff. In the ideal case this will inspire the
>>>>>>> contributor to join the review process on Phabricator; in the worst case
>>>>>>> review turns up issues in the patch and the user gives up. Either way, at
>>>>>>> least the contributor feels his patch has been seen and given the
>>>>>>> attention it deserves.
>>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>> - Ben
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> ghc-devs mailing list
>>>>>>> ghc-devs at haskell.org
>>>>>>> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>>>>>>>
>>>>> _______________________________________________
>>>>> ghc-devs mailing list
>>>>> ghc-devs at haskell.org
>>>>> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>>>>
>>>> _______________________________________________
>>>> ghc-devs mailing list
>>>> ghc-devs at haskell.org
>>>> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>>>
>> _______________________________________________
>> ghc-devs mailing list
>> ghc-devs at haskell.org
>> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>