llvm dev - Feb 2020 - Allowing PRs on GitHub for some subprojects

Hi,

I know there has been significant discussion about "moving" from
Phabricator to GitHub reviews and pull requests, etc. I'm not suggesting
that we do anything in terms of global LLVM policy. However, as a maintainer of
libc++, I commit __a lot__ of other people's code for them. It would be a
huge time saver for me if I could nicely suggest to contributors (not force
them) to use PRs instead of Phabricator for their contributions. It would also
handle commit attribution properly, which is a pain right now.

Would it be possible to allow GitHub PRs to be submitted on the monorepo so as
to let individual sub-projects deal with it however they please? I've spoken
to numerous people involved in libc++ development and they would like to start
submitting PRs (and for the others, we'll still accept Phabricator reviews).
Perhaps it is possible to setup some kind of filter such that PRs touching only
libcxx/ and libcxxabi/ can be submitted, but otherwise they're closed by the
bot?

Cheers,
Louis

Hi,

This was part of what I proposed when we integrated MLIR in LLVM (
http://lists.llvm.org/pipermail/llvm-dev/2019-November/136579.html ), we
were already using pull-requests before and had CI infrastructure already
built for it.
Ultimately we didn't press this further, divergence inside the repo /
between the subproject does not seems desirable in my opinion:
- it creates confusion: "why is this repository having pull-requests and
reviews on GitHub but my pull-request gets automatically closed?", "I
followed doc X"
- the lack of Herald on GitHub makes it so that we can't filter / subscribe
automatically to individual pull-requests: this was the major blocker in my
opinion, I couldn't find a solution to this and I believe it is critical.
- 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).

So in summary: I'd rather find a path for the full-project to do this, but
acknowledging that there are few blockers to solve before getting there (cf
other threads on the topic).

-- 
Mehdi

On Thu, Feb 20, 2020 at 10:21 AM Louis Dionne via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> I know there has been significant discussion about "moving" from
> Phabricator to GitHub reviews and pull requests, etc. I'm not
suggesting
> that we do anything in terms of global LLVM policy. However, as a
> maintainer of libc++, I commit __a lot__ of other people's code for
them.
> It would be a huge time saver for me if I could nicely suggest to
> contributors (not force them) to use PRs instead of Phabricator for their
> contributions. It would also handle commit attribution properly, which is a
> pain right now.
>
> Would it be possible to allow GitHub PRs to be submitted on the monorepo
> so as to let individual sub-projects deal with it however they please?
I've
> spoken to numerous people involved in libc++ development and they would
> like to start submitting PRs (and for the others, we'll still accept
> Phabricator reviews). Perhaps it is possible to setup some kind of filter
> such that PRs touching only libcxx/ and libcxxabi/ can be submitted, but
> otherwise they're closed by the bot?
>
> Cheers,
> Louis
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200220/3b81d74f/attachment.html>

On 02/20, Louis Dionne via llvm-dev wrote:
> I know there has been significant discussion about "moving" from
> Phabricator to GitHub reviews and pull requests, etc. I'm not
> suggesting that we do anything in terms of global LLVM policy.
> However, as a maintainer of libc++, I commit __a lot__ of other
> people's code for them. It would be a huge time saver for me if I
> could nicely suggest to contributors (not force them) to use PRs
> instead of Phabricator for their contributions. It would also handle
> commit attribution properly, which is a pain right now.
Don't take this as me telling you it is "actually simple". I am
interested what about the contribution is problematic? If the libc++
system doesn't have more requirements than the rest of LLVM there might
be ways to make it less painful. FWIW, here is what I do, and I know not
everyone wants to use `arc`. Ina script this could potentially reduce
the pain. Again, this is not meant to tell you it is simple or your
problems are not real.

arc patch DXXXX
git pull --rebase origin master
arc amend
arcfilter        // see below
git llvm push master


arcfilter () { git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1}
/Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:/' | git
commit --amend -F - }
> Would it be possible to allow GitHub PRs to be submitted on the
> monorepo so as to let individual sub-projects deal with it however
> they please? I've spoken to numerous people involved in libc++
> development and they would like to start submitting PRs (and for the
> others, we'll still accept Phabricator reviews). Perhaps it is
> possible to setup some kind of filter such that PRs touching only
> libcxx/ and libcxxabi/ can be submitted, but otherwise they're closed
> by the bot?
TBH, I feel this is yet another way of splitting the community and in
the end complicating things even more. I mean, since recently if you
want to ask a question there were the *-dev lists and the IRC. Now we
have discourse, discord on top of that with some people monitoring only
one of these and others required to monitor both. Duplicating the way we
do reviews is similarly going to require people that want to be informed
to duplicate their lookups.

Cheers,
  Johannes

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200220/0b1f4d70/attachment.sig>

On Feb 20, 2020, at 11:25 AM, Johannes Doerfert via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
>> Would it be possible to allow GitHub PRs to be submitted on the
>> monorepo so as to let individual sub-projects deal with it however
>> they please? I've spoken to numerous people involved in libc++
>> development and they would like to start submitting PRs (and for the
>> others, we'll still accept Phabricator reviews). Perhaps it is
>> possible to setup some kind of filter such that PRs touching only
>> libcxx/ and libcxxabi/ can be submitted, but otherwise they're
closed
>> by the bot?
> 
> TBH, I feel this is yet another way of splitting the community and in
> the end complicating things even more. I mean, since recently if you
> want to ask a question there were the *-dev lists and the IRC. Now we
> have discourse, discord on top of that with some people monitoring only
> one of these and others required to monitor both. Duplicating the way we
> do reviews is similarly going to require people that want to be informed
> to duplicate their lookups.
I agree with Johannes here.  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.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200220/106c88b9/attachment.html>

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.

@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. We're also too large to
follow a one-size-fits-all approach. If we agree, 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.

Best,
Christian


On Thu, Feb 20, 2020 at 7:21 PM Louis Dionne via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> I know there has been significant discussion about "moving" from
> Phabricator to GitHub reviews and pull requests, etc. I'm not
suggesting
> that we do anything in terms of global LLVM policy. However, as a
> maintainer of libc++, I commit __a lot__ of other people's code for
them.
> It would be a huge time saver for me if I could nicely suggest to
> contributors (not force them) to use PRs instead of Phabricator for their
> contributions. It would also handle commit attribution properly, which is a
> pain right now.
>
> Would it be possible to allow GitHub PRs to be submitted on the monorepo
> so as to let individual sub-projects deal with it however they please?
I've
> spoken to numerous people involved in libc++ development and they would
> like to start submitting PRs (and for the others, we'll still accept
> Phabricator reviews). Perhaps it is possible to setup some kind of filter
> such that PRs touching only libcxx/ and libcxxabi/ can be submitted, but
> otherwise they're closed by the bot?
>
> Cheers,
> Louis
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200225/2da96d9e/attachment.html>

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/20200229/8799d373/attachment-0001.html>

> On Feb 20, 2020, at 14:25, Johannes Doerfert <johannesdoerfert at
gmail.com> wrote:
> 
> On 02/20, Louis Dionne via llvm-dev wrote:
>> I know there has been significant discussion about "moving"
from
>> Phabricator to GitHub reviews and pull requests, etc. I'm not
>> suggesting that we do anything in terms of global LLVM policy.
>> However, as a maintainer of libc++, I commit __a lot__ of other
>> people's code for them. It would be a huge time saver for me if I
>> could nicely suggest to contributors (not force them) to use PRs
>> instead of Phabricator for their contributions. It would also handle
>> commit attribution properly, which is a pain right now.
> 
> Don't take this as me telling you it is "actually simple". I
am
> interested what about the contribution is problematic? If the libc++
> system doesn't have more requirements than the rest of LLVM there might
> be ways to make it less painful. FWIW, here is what I do, and I know not
> everyone wants to use `arc`. Ina script this could potentially reduce
> the pain. Again, this is not meant to tell you it is simple or your
> problems are not real.
> 
> arc patch DXXXX
> git pull --rebase origin master
> arc amend
> arcfilter        // see below
> git llvm push master
> 
> 
> arcfilter () { git log -1 --pretty=%B | awk
'/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0}
!p && !/^Summary:/' | git commit --amend -F - }
Thanks, this indeed solves some of my problems, however not entirely. When
people submit contributions without an email address, I still have to do some
digging to find out how to attribute the change. This shouldn't be something
I even have to think about.

Louis
> 
>> Would it be possible to allow GitHub PRs to be submitted on the
>> monorepo so as to let individual sub-projects deal with it however
>> they please? I've spoken to numerous people involved in libc++
>> development and they would like to start submitting PRs (and for the
>> others, we'll still accept Phabricator reviews). Perhaps it is
>> possible to setup some kind of filter such that PRs touching only
>> libcxx/ and libcxxabi/ can be submitted, but otherwise they're
closed
>> by the bot?
> 
> TBH, I feel this is yet another way of splitting the community and in
> the end complicating things even more. I mean, since recently if you
> want to ask a question there were the *-dev lists and the IRC. Now we
> have discourse, discord on top of that with some people monitoring only
> one of these and others required to monitor both. Duplicating the way we
> do reviews is similarly going to require people that want to be informed
> to duplicate their lookups.
> 
> Cheers,
>  Johannes
>