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

Thu Mar 5 10:16:49 PST 2020

On Thu, Mar 5, 2020 at 8:30 AM Louis Dionne <ldionne at apple.com> wrote:

>
>
> On Mar 4, 2020, at 20:15, Mehdi AMINI <joker.eph at gmail.com> wrote:
>
>
>
> On Wed, Mar 4, 2020 at 9:42 AM Louis Dionne <ldionne at apple.com> wrote:
>
>>
>>
>> On Mar 4, 2020, at 12:13, Mehdi AMINI <joker.eph at gmail.com> wrote:
>>
>>
>>
>> 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?
>>
>>
>> It was about allowing GitHub PRs for libc++, and one of the reasons I
>> cited was commit attribution.
>>
>>
>> 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.
>>
>>
>> That's really what I've tried doing below. Quoting myself:
>>
>> > 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.
>>
>> Maybe I should have stated that earlier on.
>>
>>
>>
>> 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?
>>
>>
>> At the risk of oversharing, this has been my biggest source of pain and
>> frustration since I've became a libc++ maintainer. It's always been a
>> problem since I started about 2 years ago. I can still feel the hurt of
>> fixing the ripples of landing aligned allocation in libc++ in 2018, which
>> lasted for weeks and weeks (on and off).
>>
>> Last week, I spent about 4 days fixing the ripples of a high-quality
>> contribution that happened to break some bot configurations (and some
>> botless ones too). That was both very stressful and frustrating, and I
>> swore to myself this was going to be the last week like that.
>>
>>
>> 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.
>>
>>
>> My understanding is that Christian was the one to setup the pre-merge
>> testing we have currently (thanks!). And he says:
>>
>>     > This is soooooo much easier to set up and maintain than the
>> Phabricator integration
>>
>
> Of course it is easier to do on GitHub, but the integration on Phabricator
> can also a one-time cost for the project, which is already kind of on the
> way with https://github.com/google/llvm-premerge-checks ; then it is a
> matter of making it easier to add your own bot the pre-merge and increase
> the coverage.
>
>
> You seem to be dismissing the cost of setting up bots as being something
> that has to be paid once.
>

Sorry if I wasn't clear on this: I was referring to the difference between
integrating a bot infrastructure with Phabricator vs GitHub as a one-time
cost. The extra cost unique to Phabricator should be paid once for the
project, adding more bots to a pre-merge system that is triggered by and
report to Phab should then be fairly equivalent afterward hopefully.

> In my experience, however, this is actually a recurring cost because these
> bots need maintenance. And whenever we add a new bot, it's also a problem
> if we are confronted to this complexity again.
>
> But like I said before, I'm only here to solve my (libc++) problems. And
> right now, my problem is that I *literally can't* figure out how to setup a
> pre-merge bot on Phabricator. Do you know how to do that? In all
> seriousness, can you show me how to set up even a simple bot that would run
> the most basic libc++ configuration? I'm sure I would be able (and
> certainly willing) to extrapolate from there and set up the other missing
> configurations without your help.
>
> But if you can't help me, then who can? Honestly, I think we're all
> talking out of our hats and the only person who truly has gone through the
> process is Christian -- and you've seen his email.
>

I started working on this in September, anticipating the migration of MLIR
into the monorepo, the only reason I stopped is because I learnt that
Christian was doing it as well.
My understanding is that the goal of
https://github.com/google/llvm-premerge-checks is not to setup a rigid
one-time bot for Phabricator, but reach a point where adding a new
configuration should have a playbook similar to
https://llvm.org/docs/HowToAddABuilder.html

Best,

-- 
Mehdi

>
> Cheers,
> Louis
>
>
>
>
>> Furthermore, what libc++ needs in this area is not necessarily the same
>> as the rest of LLVM. I don't know what pre-merge bots are setup right now,
>> and it's certainly better than nothing, but we have a lot of test
>> configurations that just don't really make sense for a toolchain:
>> - test suite in c++03/c++11/c++14/etc
>> - exceptions on/off
>> - with per-TU guarantee enabled/disabled
>> - all the macOS back-deployment targets against system dylibs
>> - etc.
>>
>
> I just see this as different configurations / builders that you have to
> provide if you care about, just like our current post-merge buildbot
> infrastructure actually: http://lab.llvm.org:8011/buildslaves
> For example MLIR would want to run test on machines with GPUs (to test our
> CUDA and Vulkan paths), which are of no interest to most of the other
> projects, yet that seems fairly orthogonal to the mechanism to register a
> new bots / config with the system.
>
>
>
>>
>> I'm not expecting anyone else to set these up cause it doesn't make
>> sense. I need to be empowered to do it myself, and right now I'm not (or
>> I'm really not looking in the right spot).
>>
>> 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...
>>
>>
>> Sorry, that was really not my intent. The point I tried to make is that
>> we have different priorities because we work on different projects, which
>> have different requirements. So what may seem like just an annoyance to
>> some might be a huge quality of life and productivity issue for someone
>> else.
>>
>> Louis
>>
>>
>>
>> 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/20200305/08625e6b/attachment.html>