[llvm-dev] Allowing PRs on GitHub for some subprojects

Wed Mar 4 09:13:01 PST 2020

On Wed, Mar 4, 2020 at 8:14 AM Louis Dionne <ldionne at apple.com> wrote:

> Mehdi, Chris & others,
>
> I guess I did not express the main reasons for wanting to switch over very
> well in my original message.
>

You original message was about “ commit attribution”, but now it is all
about testing?

Instead of jumping to a solution (pull-request) why not expressing the
actual problem (lack of pre merge testing) and discuss it and explore all
the possible solutions?
I think the discussion would be much more productive if we take it from
first principles here.

Like Christian talked about, for me it's all about pre-commit testing. I
> believe pre-commit testing is a widely shared desire among this community.
> However, how badly it is missed depends on sub-projects, because they have
> different realities. For example, in libc++:
>
> 1. We have a lot of first-time contributors, which means that the
> maintainers end up shepherding many contributions. In particular, this
> often means fixing small breakage following their changes, which can be
> difficult for them because they can't reproduce the failures locally, and
> they might not even know where to look. While these contributors can submit
> valuable improvements and bug fixes, we can't expect them to fix every last
> platform that we support in the current state of things -- it's hard, it's
> boring, and it's stressful.
>
> 2. Our testing matrix is very large, and interactions between different
> configurations (usually #ifs/#elses) is very subtle. This means the rate of
> mistake-on-first-try is, I think, higher in libc++ than in most other LLVM
> projects. Even with careful review, I find that a large percentage of
> changes end up breaking something somewhere, and I have to fix it (usually
> quickly enough to avoid reverting).
>
> As a result, the lack of pre-commit testing is actively harming the health
> of libc++ as a project. It might be true for other projects as well, but I
> can only speak for libc++ because that's where I have first hand
> experience.
>

What changed recently that makes this suddenly critical compared to the
previous years?

Unfortunately, we currently don't have a good way of doing pre-commit
> testing on Phabricator AFAICT
>

I thought we do now? I got a bunch of libcxx failing on my revision a few
weeks ago.
The LLVM-premerge-test project recently added presubmit on Linux.
Windows will hopefully follow soon (in beta right now I believe), and Mac
afterward!  (Even though mac is lacking on the cloud provider availability)

. From the Harbormaster documentation [1]:
>
>     You'll need to write a nontrivial amount of code to get this working
> today. In the future, Harbormaster will become more powerful and have more
> builtin support for interacting with build systems.
>
> So while I appreciate all the efforts being made in this area, I still
> don't even know where to start if I want to setup pre-commit testing for
> libc++ today. However, the path is very clear with GitHub PRs and there are
> many options available.
>
> Whenever I hear arguments of dividing the community, not being able to
> share infrastructure, the lack of Herald -- those all make a lot of sense
> to me and I think they're good arguments.
>
However, it is clear that folks who even think about these arguments are
> not paying the same cost for the lack of good pre-commit testing that I'm
> paying on a weekly basis,
>

FYI that can read quite condescending...

because for me that outweighs everything else.
>

> I don't know how to come to a decision here, all I know is that libc++
> needs to get out of the status quo soon. And if the solution is that
> Harbormaster suddenly becomes usable without an unreasonable time
> investment from me, then I'm fine with that too. I'm not looking to switch
> to GitHub PRs for the sake of it, I'm looking to solve problems that are
> harming libc++ in the current system.
>
> Cheers,
> Louis
>
> [1]: https://secure.phabricator.com/book/phabricator/article/harbormaster/
>
>
> On Feb 29, 2020, at 23:06, Mehdi AMINI <joker.eph at gmail.com> wrote:
>
>
>
> On Tue, Feb 25, 2020 at 4:19 AM Christian Kühnel via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi Louis,
>>
>> I think this is a good idea. We should start with some local experiments
>> where people are willing to try it and figure out how well that works and
>> what does not. Why not allow this for "not significant" changes? They are
>> merged without review today, so we could do them with reviews (and
>> automated tests) via pull requests instead.
>>
>
> I still feel this is only a recipe for confusion if "some" pull-requests
> are accepted on Github but not all. So -1 from me on this.
>
>
>>
>> @Mehdi
>>
>>> - it does not favor to build common tooling: the recent work on enabling
>>> pre-submit CI tests on Phabricator is valuable and I'm looking forward to
>>> get this extended. But splitting the various ways of contributing to the
>>> repo just means more infrastructure to build to sustain this kind of
>>> efforts. (the infrastructure is easier built on GitHub by the way, but
>>> that is an argument in favor of migrating from Phab to GH for the
>>> full-project).
>>>
>>
>> Oh I'm happy to add Github support as soon as someone switches on PRs.
>> This is soooooo much easier to set up and maintain than the Phabricator
>> integration. And we already have builds for the release branch (
>> https://buildkite.com/llvm-project/llvm-release-builds) anyway. So we
>> could easily scale that up. And we can only get pre-merge testing on
>> Phabricator to a certain point, as it's not triggering builds for ~50% of
>> the code reviews.
>>
>> @Chris Lattner
>>
>>> Although I am one of the (many) people who would love to see us move
>>> from Phabricator to GitHub PRs, I think it is super important that we
>>> do the transition all at once to keep the LLVM community together.  I’m
>>> already concerned about the fragmentation the discourse server is causing,
>>> e.g. MLIR not using a -dev list.  I’d rather the community processes stay
>>> consistent.
>>>
>>
>> Please allow me to disagree there. IMHO we're way too large and diverse
>> of a project to do binary, overnight transitions.
>>
>
> You seem to be arguing the "how to transition" while there is no agreement
> on a transition happening in the first place.
>
>
>> We're also too large to follow a one-size-fits-all approach. If we agree,
>>
>
> I don't: we went with a monorepo because we believed that the
> one-size-fits-all would be more beneficial than splitting, both in terms of
> infrastructure, but also in terms of the practices of the community, etc.
>
> Github PRs are the right glow, why take this step-by-step. We should have
>> something like a list of important and supported use cases/interactions for
>> the infrastructure. Then we could start working on them one-by-one and
>> figure out if/how they could be implemented on Github and how we could do a
>> smooth transition between these.
>>
>> If Herald rules are important: Find a way to implement something similar
>> for Github. Maybe there is even a market for such a tool.
>> If transparency is the problem: Find a way to mirror PRs into
>> Phabricator, so people can at least see them there.
>> We're not restricted to community contributions there. We can also pay
>> someone to build the things we need.
>>
>
> One aspect here though is that we can pay someone to build the things we
> need in Phabricator, we can't change GitHub though.
> It was mentioned in the past that we should engage with GitHub and see if
> they would add the feature we're missing to their roadmap, if it hasn't
> been done I'd start there: building up this list of things that need to
> happens before we can agree towards a transition, and engaging with GitHub
> to have these.
>
> --
> Mehdi
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200304/9c738547/attachment.html>