llvm dev - Nov 2019 - Enable Contributions Through Pull-request For LLVM

Hi all,

Now that we're on GitHub, we can discuss about pull-requests.
I'd like to propose to enable pull-request on GitHub, as a first step as an
experimental channel alongside the existing methods for contributing to
LLVM.
This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests
without committing to switching to pull-requests immediately. The community
should evaluate after a few months what would the next steps be.

GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to
disable it!

The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be
the following:

  - Contributors should use their own fork and push a branch in their fork.
  - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
  - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate
review threads, and thus separate pull-requests.
This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and
allow to visualize the incremental diff across revision of the
pull-request.
  - Upon “merge” of a pull-request: *history is linear* and a single commit
lands in master after review is completed.

As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with
for example) in the repo. In this case, we would propose to begin with the
MLIR project (as soon as it gets integrated in the monorepo). This would be
a good candidate to be the guinea pig for this process since it does not
yet have a wide established community of contributors, and the current
contributors are already exclusively using pull-requests
<https://github.com/tensorflow/mlir/pulls>.

Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI

Cheers,

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191106/564cd2ee/attachment.html>

> On Nov 6, 2019, at 9:32 PM, Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi all,
> 
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
> 
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
> 
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
> 
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
This sounds great, and +1 for ’squash and merge’ as the only option.

I’m a huge fan of incremental public development, and would prefer not to have
people incentivized to do “lots of incremental changes in their local fork, then
merge the huge thing” while claiming to abide by the idea of incremental
development.  Such an approach doesn’t get the CI or review benefits, and
eliminating this from the workflow seems like a very clean solution.  To be
clear, I have no problem with people doing whatever local development policies
they prefer, I just think they should be squashed out as an implementation
detail when the aggregate patch gets merged.
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests
<https://github.com/tensorflow/mlir/pulls>.
> 
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
<https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI>
Overall this plan sounds good to me.  Please keep the “how to contribute” doc
and the project description (which currently says 'Note: the repository does
not accept github pull requests at this moment.’) up to date as processes
evolve! :-)

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191106/d93a4e40/attachment.html>

Strong -1 personally.

* What is the endgoal? To fully kill phab and move to github pullrequests?
  it might be best to discuss *that* first. (did i miss an RFC?)
* Separation of attention - does everyone who cares
  now has to also look at pull requests for reviews;
  or should they be exempt from general review attention?
* For me personally after using phabricator, github now seems
  extremely crude, laggy, limited. To name a few:
  * There is no way to see previous version of the patch.
    I don't think there is any way to disable force-push for PR's.
    While this is only 'slightly' limiting for the reviewer,
    this can be more limiting for the author - how do i go back
    to previous revision? I can't, i need to maintain a copy
    of every branch i pushed manually.
  * Impossible to chain reviews - a PR diff can only be made
    on top of git master branch. Any non-trivial change consists of
    separable PR's. Now either one will only be able to submit
    dependent PR after the prereqs are merged, or the diff will be
    impossible to read.
  * Does not load large diffs by default.
    That caught me by surprise several times
    when i was searching for something.
  * No diffs in mail - *super* inconvenient.
    One has to open each pr in browser (or fetch via git etc etc)
  * Github is an official US-based commercial organisation.
    It *has* to follow U.S. export law. In particular i'm thinking of
      https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/
      https://github.com/tkashkin/GameHub/issues/289
    Does phabricator already have such restrictions, blocks?
    If not, wouldn't you say adding such restrictions is not being
    more open for contributors?
    What happens when, in not so long in the future, the entirety of, say,
    china or russian federation is blocked as such?
  * Same question can be asked about internet "iron" curtains
    certain (*cough*) countries are raising. That also has already happened
    before (and *will* happen again), and i was personally affected:
      https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia
    I don't recall that happening to phabricator yet.
    I fail to see how that is more contributor-friendly.
  * Not sure anyone cares, but while using github as main git
    repository "mirror" is perfectly fine - git is distributed, only
canonical
    write-repo would be affected anything bad happen. But that isn't the
case
    for reviews, issues; as it has been discussed in the "let's migrate
bugzilla
    to github issues", it is far more involved.
  * What about DMCA? Not sure how this is currently handled.
  * UI feels laggy. Not much to add here, pretty subjective.
  * I'm sure i'm missing a few.

The github does come with it's benefits, sure:
* It is *simpler* to preserve git commit author.
  Currently one has to ask the author for the "Author: e at ma.il"
line,
  and do `git commit --amend --author="<>"`.
* @mention is wide-r-reaching - whole github, not just llvm phabricator
* No more "phabricator disk full" issues
* ???

TLDR: such migration lowers the bar for new, first time,
unestabilished contributors, but i personally feel it *significantly*
raises the bar for the existing contributors, reviewers.
We don't live in perfect world. Aspirational goals are aspirational.
They should be attempted to be reached, but they shouldn't shadow and
overweight, take over the main goal of the LLVM project.

Personally, i don't see that benefits out-/over- weight the drawbacks.

Roman.

On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Hi all,
>
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
>
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
>
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
>
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
>
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
>
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
>
> Cheers,
>
> --
> Mehdi
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Hi Mehdi,

I support the idea of running some experiments to gain more insights.

If you're interested: I can offer to add the pre-merge tests to the github
pull-requests as well. This way we can also see what the CI integration in
github would look like.

On Thu, Nov 7, 2019 at 9:09 AM Roman Lebedev via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Strong -1 personally.
>
> * What is the endgoal? To fully kill phab and move to github pullrequests?
>   it might be best to discuss *that* first. (did i miss an RFC?)
> * Separation of attention - does everyone who cares
>   now has to also look at pull requests for reviews;
>   or should they be exempt from general review attention?
> * For me personally after using phabricator, github now seems
>   extremely crude, laggy, limited. To name a few:
>   * There is no way to see previous version of the patch.
>     I don't think there is any way to disable force-push for PR's.
>     While this is only 'slightly' limiting for the reviewer,
>     this can be more limiting for the author - how do i go back
>     to previous revision? I can't, i need to maintain a copy
>     of every branch i pushed manually.
>   * Impossible to chain reviews - a PR diff can only be made
>     on top of git master branch. Any non-trivial change consists of
>     separable PR's. Now either one will only be able to submit
>     dependent PR after the prereqs are merged, or the diff will be
>     impossible to read.
>   * Does not load large diffs by default.
>     That caught me by surprise several times
>     when i was searching for something.
>   * No diffs in mail - *super* inconvenient.
>     One has to open each pr in browser (or fetch via git etc etc)
>   * Github is an official US-based commercial organisation.
>     It *has* to follow U.S. export law. In particular i'm thinking of
>       https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/
>       https://github.com/tkashkin/GameHub/issues/289
>     Does phabricator already have such restrictions, blocks?
>     If not, wouldn't you say adding such restrictions is not being
>     more open for contributors?
>     What happens when, in not so long in the future, the entirety of, say,
>     china or russian federation is blocked as such?
>   * Same question can be asked about internet "iron" curtains
>     certain (*cough*) countries are raising. That also has already happened
>     before (and *will* happen again), and i was personally affected:
>       https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia
>     I don't recall that happening to phabricator yet.
>     I fail to see how that is more contributor-friendly.
>   * Not sure anyone cares, but while using github as main git
>     repository "mirror" is perfectly fine - git is distributed,
only
> canonical
>     write-repo would be affected anything bad happen. But that isn't
the
> case
>     for reviews, issues; as it has been discussed in the "let's
migrate
> bugzilla
>     to github issues", it is far more involved.
>   * What about DMCA? Not sure how this is currently handled.
>   * UI feels laggy. Not much to add here, pretty subjective.
>   * I'm sure i'm missing a few.
>
> The github does come with it's benefits, sure:
> * It is *simpler* to preserve git commit author.
>   Currently one has to ask the author for the "Author: e at
ma.il" line,
>   and do `git commit --amend --author="<>"`.
> * @mention is wide-r-reaching - whole github, not just llvm phabricator
> * No more "phabricator disk full" issues
> * ???
>
> TLDR: such migration lowers the bar for new, first time,
> unestabilished contributors, but i personally feel it *significantly*
> raises the bar for the existing contributors, reviewers.
> We don't live in perfect world. Aspirational goals are aspirational.
> They should be attempted to be reached, but they shouldn't shadow and
> overweight, take over the main goal of the LLVM project.
>
> Personally, i don't see that benefits out-/over- weight the drawbacks.
>
> Roman.
>
> On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Hi all,
> >
> > Now that we're on GitHub, we can discuss about pull-requests.
> > I'd like to propose to enable pull-request on GitHub, as a first
step as
> an experimental channel alongside the existing methods for contributing to
> LLVM.
> > This would allow to find workflow issues and address them, and also
LLVM
> contributors in general to start getting familiar with pull-requests
> without committing to switching to pull-requests immediately. The community
> should evaluate after a few months what would the next steps be.
> >
> > GitHub pull-requests is the natural way to contribute to project
hosted
> on GitHub: this feature is so core to GitHub that there is no option to
> disable it!
> >
> > The current proposal is to allow to integrate contributions to the
LLVM
> project directly from pull-requests. In particular the exact setup would be
> the following:
> >
> >   - Contributors should use their own fork and push a branch in their
> fork.
> >   - Reviews can be performed on GitHub. The canonical tools are still
> the mailing-list and Phabricator: a reviewer can request the review to move
> to Phabricator.
> >   - The only option available will be to “squash and merge”. This mode
> of review matches the closest our current workflow (both phabricator and
> mailing-list): conceptually independent contributions belongs to separate
> review threads, and thus separate pull-requests.
> > This also allow the round of reviews to not force-push the original
> branch and accumulate commits: this keeps the contextual history of
> comments and allow to visualize the incremental diff across revision of the
> pull-request.
> >   - Upon “merge” of a pull-request: history is linear and a single
> commit lands in master after review is completed.
> >
> > As an alternative staging proposal: we could enable pull-requests only
> for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin
> with for example) in the repo. In this case, we would propose to begin with
> the MLIR project (as soon as it gets integrated in the monorepo). This
> would be a good candidate to be the guinea pig for this process since it
> does not yet have a wide established community of contributors, and the
> current contributors are already exclusively using pull-requests.
> >
> > Here is a more complete doc on the topic:
>
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> >
> > Cheers,
> >
> > --
> > Mehdi
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

-- 
Best,
Christian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191107/9d71d91f/attachment.html>

-1 from me as well. GitHub UI feels extremely sluggish with large
projects, forks feel like needless hurdles to jump over,
especially for small patches - having to fork the entire monorepo,
just to do a GitHub PR would be extremely cumbersome.

Just recently I submitted a very tiny PR for PcapPlusPlus on GitHub
which involved:
1). Forking the repository
2). Cloning the fork
3). Applying the patch
4). Pushing it and creating a PR.
5). Deleting the fork (Since I didn't want it cluttering up my account
and I'm not a regular contributor to PcapPlusPlus).

Neither the pull request mechanism nor GitHub issues feel like they're
suitable for large projects since it's common to see large GitHub
projects with ignored issues and worse filtering/categorizing. And PRs
as a review mechanism are far worse than Phabricator or any other
dedicated code review software, and I think a lot of GitHub projects
use internal or separate code review tools as-is, with PRs being
second class citizens and are more prone to getting derailed. It's not
uncommon for GitHub issue or PR discussions to turn into massive
threads that quickly turn into bikeshedding and off-topic conversations.

It also feels like this will inevitably force people to use external
tools to handle it, instead of just being able to use Git on its
own (something like `git cl` comes to mind, which some may like and
some may not).

And Roman raised a lot of good points regarding other, more
bureaucratic aspects of GitHub and it raising the bar for existing
contributors.

Just my two cents in.

On Thu, Nov 7, 2019 at 8:09 AM Roman Lebedev via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Strong -1 personally.
>
> * What is the endgoal? To fully kill phab and move to github pullrequests?
>   it might be best to discuss *that* first. (did i miss an RFC?)
> * Separation of attention - does everyone who cares
>   now has to also look at pull requests for reviews;
>   or should they be exempt from general review attention?
> * For me personally after using phabricator, github now seems
>   extremely crude, laggy, limited. To name a few:
>   * There is no way to see previous version of the patch.
>     I don't think there is any way to disable force-push for PR's.
>     While this is only 'slightly' limiting for the reviewer,
>     this can be more limiting for the author - how do i go back
>     to previous revision? I can't, i need to maintain a copy
>     of every branch i pushed manually.
>   * Impossible to chain reviews - a PR diff can only be made
>     on top of git master branch. Any non-trivial change consists of
>     separable PR's. Now either one will only be able to submit
>     dependent PR after the prereqs are merged, or the diff will be
>     impossible to read.
>   * Does not load large diffs by default.
>     That caught me by surprise several times
>     when i was searching for something.
>   * No diffs in mail - *super* inconvenient.
>     One has to open each pr in browser (or fetch via git etc etc)
>   * Github is an official US-based commercial organisation.
>     It *has* to follow U.S. export law. In particular i'm thinking of
>       https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/
>       https://github.com/tkashkin/GameHub/issues/289
>     Does phabricator already have such restrictions, blocks?
>     If not, wouldn't you say adding such restrictions is not being
>     more open for contributors?
>     What happens when, in not so long in the future, the entirety of, say,
>     china or russian federation is blocked as such?
>   * Same question can be asked about internet "iron" curtains
>     certain (*cough*) countries are raising. That also has already happened
>     before (and *will* happen again), and i was personally affected:
>       https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia
>     I don't recall that happening to phabricator yet.
>     I fail to see how that is more contributor-friendly.
>   * Not sure anyone cares, but while using github as main git
>     repository "mirror" is perfectly fine - git is distributed,
only canonical
>     write-repo would be affected anything bad happen. But that isn't
the case
>     for reviews, issues; as it has been discussed in the "let's
migrate bugzilla
>     to github issues", it is far more involved.
>   * What about DMCA? Not sure how this is currently handled.
>   * UI feels laggy. Not much to add here, pretty subjective.
>   * I'm sure i'm missing a few.
>
> The github does come with it's benefits, sure:
> * It is *simpler* to preserve git commit author.
>   Currently one has to ask the author for the "Author: e at
ma.il" line,
>   and do `git commit --amend --author="<>"`.
> * @mention is wide-r-reaching - whole github, not just llvm phabricator
> * No more "phabricator disk full" issues
> * ???
>
> TLDR: such migration lowers the bar for new, first time,
> unestabilished contributors, but i personally feel it *significantly*
> raises the bar for the existing contributors, reviewers.
> We don't live in perfect world. Aspirational goals are aspirational.
> They should be attempted to be reached, but they shouldn't shadow and
> overweight, take over the main goal of the LLVM project.
>
> Personally, i don't see that benefits out-/over- weight the drawbacks.
>
> Roman.
>
> On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Hi all,
> >
> > Now that we're on GitHub, we can discuss about pull-requests.
> > I'd like to propose to enable pull-request on GitHub, as a first
step as an experimental channel alongside the existing methods for contributing
to LLVM.
> > This would allow to find workflow issues and address them, and also
LLVM contributors in general to start getting familiar with pull-requests
without committing to switching to pull-requests immediately. The community
should evaluate after a few months what would the next steps be.
> >
> > GitHub pull-requests is the natural way to contribute to project
hosted on GitHub: this feature is so core to GitHub that there is no option to
disable it!
> >
> > The current proposal is to allow to integrate contributions to the
LLVM project directly from pull-requests. In particular the exact setup would be
the following:
> >
> >   - Contributors should use their own fork and push a branch in their
fork.
> >   - Reviews can be performed on GitHub. The canonical tools are still
the mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
> >   - The only option available will be to “squash and merge”. This mode
of review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> > This also allow the round of reviews to not force-push the original
branch and accumulate commits: this keeps the contextual history of comments and
allow to visualize the incremental diff across revision of the pull-request.
> >   - Upon “merge” of a pull-request: history is linear and a single
commit lands in master after review is completed.
> >
> > As an alternative staging proposal: we could enable pull-requests only
for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with
for example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
> >
> > Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> >
> > Cheers,
> >
> > --
> > Mehdi
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Huge +1 from my side.

There are couple of small patches I want to submit, but the fact that I need to
deal with phabricator just stops me from doing this.

Looking at the other comments against this step I'm wondering if the goal of
the project is to keep existing (dozens?) contributors happy, or to embrace more
(hundreds?) people to join the project?

====
I'd love to leave some comments on other comments as I've been using
Github for OSS/Commercial development for many years now and I find it to be one
of the best things that can happen to an open source project.
> "Github UI is ugly/slaggish/bad"
that's obviously subjective, and I can only say the same about phabricator.
> Geopolitics/laws/whatnot
this is orthogonal to the subject, right?
>  There is no way to see previous version of the patch.
>  I don't think there is any way to disable force-push for PR's.
>  While this is only 'slightly' limiting for the reviewer,
>  this can be more limiting for the author - how do i go back
>  to previous revision? I can't, i need to maintain a copy
>  of every branch i pushed manually.
Seeing previous version is only possible if the author uses different commits,
but is it not possible to enforce it.
Force pushes can only be disabled per repo, so if we allow to submit PRs without
forks, then it is possible to solve this issue as well.
>  Impossible to chain reviews - a PR diff can only be made
>  on top of git master branch.
This is also true only for forked repositories.
Though, imo, chained PRs (feature branch based development) contradicts to the
trunk-based approach LLVM project is using.
Correct me if I'm wrong.
>  Just recently I submitted a very tiny PR for PcapPlusPlus on GitHub
>  which involved:
>  1). Forking the repository
>  2). Cloning the fork
>  3). Applying the patch
>  4). Pushing it and creating a PR.
>  5). Deleting the fork (Since I didn't want it cluttering up my account
>  and I'm not a regular contributor to PcapPlusPlus).
2) and 3) are not necessary since the repo can have multiple remotes.
For some very simple cases one doesn't even need to leave github UI: it is
possible to send patches right from the website.
> On 7. Nov 2019, at 06:32, Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi all,
> 
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
> 
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
> 
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
> 
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
> 
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests
<https://github.com/tensorflow/mlir/pulls>.
> 
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
<https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI>
> 
> Cheers,
> 
> -- 
> Mehdi
> 
> _______________________________________________
> 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/20191107/5cfe08fb/attachment.html>

On Thu, Nov 7, 2019 at 3:09 AM Roman Lebedev via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Strong -1 personally.
Likewise, for many of the same reasons detailed below.

~Aaron
> * What is the endgoal? To fully kill phab and move to github pullrequests?
>   it might be best to discuss *that* first. (did i miss an RFC?)
> * Separation of attention - does everyone who cares
>   now has to also look at pull requests for reviews;
>   or should they be exempt from general review attention?
> * For me personally after using phabricator, github now seems
>   extremely crude, laggy, limited. To name a few:
>   * There is no way to see previous version of the patch.
>     I don't think there is any way to disable force-push for PR's.
>     While this is only 'slightly' limiting for the reviewer,
>     this can be more limiting for the author - how do i go back
>     to previous revision? I can't, i need to maintain a copy
>     of every branch i pushed manually.
>   * Impossible to chain reviews - a PR diff can only be made
>     on top of git master branch. Any non-trivial change consists of
>     separable PR's. Now either one will only be able to submit
>     dependent PR after the prereqs are merged, or the diff will be
>     impossible to read.
>   * Does not load large diffs by default.
>     That caught me by surprise several times
>     when i was searching for something.
>   * No diffs in mail - *super* inconvenient.
>     One has to open each pr in browser (or fetch via git etc etc)
>   * Github is an official US-based commercial organisation.
>     It *has* to follow U.S. export law. In particular i'm thinking of
>       https://techcrunch.com/2019/07/29/github-ban-sanctioned-countries/
>       https://github.com/tkashkin/GameHub/issues/289
>     Does phabricator already have such restrictions, blocks?
>     If not, wouldn't you say adding such restrictions is not being
>     more open for contributors?
>     What happens when, in not so long in the future, the entirety of, say,
>     china or russian federation is blocked as such?
>   * Same question can be asked about internet "iron" curtains
>     certain (*cough*) countries are raising. That also has already happened
>     before (and *will* happen again), and i was personally affected:
>       https://en.wikipedia.org/wiki/Censorship_of_GitHub#Russia
>     I don't recall that happening to phabricator yet.
>     I fail to see how that is more contributor-friendly.
>   * Not sure anyone cares, but while using github as main git
>     repository "mirror" is perfectly fine - git is distributed,
only canonical
>     write-repo would be affected anything bad happen. But that isn't
the case
>     for reviews, issues; as it has been discussed in the "let's
migrate bugzilla
>     to github issues", it is far more involved.
>   * What about DMCA? Not sure how this is currently handled.
>   * UI feels laggy. Not much to add here, pretty subjective.
>   * I'm sure i'm missing a few.
>
> The github does come with it's benefits, sure:
> * It is *simpler* to preserve git commit author.
>   Currently one has to ask the author for the "Author: e at
ma.il" line,
>   and do `git commit --amend --author="<>"`.
> * @mention is wide-r-reaching - whole github, not just llvm phabricator
> * No more "phabricator disk full" issues
> * ???
>
> TLDR: such migration lowers the bar for new, first time,
> unestabilished contributors, but i personally feel it *significantly*
> raises the bar for the existing contributors, reviewers.
> We don't live in perfect world. Aspirational goals are aspirational.
> They should be attempted to be reached, but they shouldn't shadow and
> overweight, take over the main goal of the LLVM project.
>
> Personally, i don't see that benefits out-/over- weight the drawbacks.
>
> Roman.
>
> On Thu, Nov 7, 2019 at 8:32 AM Mehdi AMINI via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > Hi all,
> >
> > Now that we're on GitHub, we can discuss about pull-requests.
> > I'd like to propose to enable pull-request on GitHub, as a first
step as an experimental channel alongside the existing methods for contributing
to LLVM.
> > This would allow to find workflow issues and address them, and also
LLVM contributors in general to start getting familiar with pull-requests
without committing to switching to pull-requests immediately. The community
should evaluate after a few months what would the next steps be.
> >
> > GitHub pull-requests is the natural way to contribute to project
hosted on GitHub: this feature is so core to GitHub that there is no option to
disable it!
> >
> > The current proposal is to allow to integrate contributions to the
LLVM project directly from pull-requests. In particular the exact setup would be
the following:
> >
> >   - Contributors should use their own fork and push a branch in their
fork.
> >   - Reviews can be performed on GitHub. The canonical tools are still
the mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
> >   - The only option available will be to “squash and merge”. This mode
of review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> > This also allow the round of reviews to not force-push the original
branch and accumulate commits: this keeps the contextual history of comments and
allow to visualize the incremental diff across revision of the pull-request.
> >   - Upon “merge” of a pull-request: history is linear and a single
commit lands in master after review is completed.
> >
> > As an alternative staging proposal: we could enable pull-requests only
for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with
for example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
> >
> > Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> >
> > Cheers,
> >
> > --
> > Mehdi
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

+1 to the pull request model in general. The pull request model is the more
conventional way of using git in my experience and I'm generally in favour
of a conventional workflow. I can't really assess the quality of
GitHub's code review tools though as I haven't used theirs for several
years. I'm willing to try them on a significant patch but I'd like to do
that before I +1 for GitHub specifically or decide I prefer to keep Phabricator
in the process. That said, I think it's ok to allow them and see if the
community shifts to them in the long run. That's pretty much how our usage
of Phabricator took off after all.
> On Nov 6, 2019, at 21:32, Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi all,
> 
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
> 
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
> 
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
> 
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
-1 to "squash and merge" being the only option if rebase+push
(--ff-only) is possible. I'd rather we use our judgement to decide
what's appropriate for the pull request at hand rather than have a blanket
rule.

Personally, if I have multiple commits (typically 2-5) in a pull request
it's because there's incremental steps that I'd like to preserve.
For example:
1. Add assembler support for X
2. Add codegen support for X

1. Move X to libY. NFC
2. Add support for Z
We could do each step in individual pull requests but it's unnecessary
overhead.

on the other hand, something like:
1. Add support for X
2. clang-format
3. typo
4. another typo
5. fix the build
6. wip
7. Fix test failure in target Y
should be tidied up before merging (squashed in this case but `rebase -i` is
sometimes appropriate).
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
Personally, I'd like us to drop the linear history requirement but I know
there's some strong opposition there. The main reason I'd like us to
accept non-linear history in conjunction with pull requests is that it means
that the commits that were tested (whether by the author or by CI) still exist
in the repository after the merge. If we rebase+push like we do today or
squash+merge as in this proposal, then the information that a particular commit
passed testing (or failed a particular test) gets lost.
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests
<https://github.com/tensorflow/mlir/pulls>.
> 
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
<https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI>
> 
> Cheers,
> 
> -- 
> Mehdi
> 
> _______________________________________________
> 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/20191107/e80ace9f/attachment.html>

> On Nov 7, 2019, at 5:54 PM, Daniel Sanders via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> -1 to "squash and merge" being the only option if rebase+push
(--ff-only) is possible. I'd rather we use our judgement to decide
what's appropriate for the pull request at hand rather than have a blanket
rule.
> 
> Personally, if I have multiple commits (typically 2-5) in a pull request
it's because there's incremental steps that I'd like to preserve.
For example:
> 1. Add assembler support for X
> 2. Add codegen support for X
> 
> 1. Move X to libY. NFC
> 2. Add support for Z
> We could do each step in individual pull requests but it's unnecessary
overhead.
I agree that pull requests add additional overhead, but they can be scripted to
reduce it.  My primary goal is to achieve these things set out in the developer
policy:
http://llvm.org/docs/DeveloperPolicy.html#incremental-development
<http://llvm.org/docs/DeveloperPolicy.html#incremental-development>

By making each of these steps explicit PR’s, they get individual testing (when
we have integrated presubmit testing, which we’ll hopefully get to), code
reviewers are reviewing each atomic step (and not the result of a bunch of
walk), etc.

In my opinion, bundling up many small patches into a single large PR subverts a
lot of the goals of incremental development.  It is better to propose the small
patches as individual patches.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191107/42b90ae8/attachment.html>

On Thu, Nov 7, 2019 at 1:44 AM Alex Denisov via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Huge +1 from my side.
>
> There are couple of small patches I want to submit, but the fact that I
need to deal with phabricator just stops me from doing this.
>
> Looking at the other comments against this step I'm wondering if the
goal of the project is to keep existing (dozens?) contributors happy, or to
embrace more (hundreds?) people to join the project?
Understanding why people get frustrated with Phabricator will be an
important topic. For arcanist users, creating a Phabricator
differential is as easy as one command invocation: `arc diff 'HEAD^'`.
One obstacle is that people likely have a Github account but they just
don't want to learn a new system.

The lack of a Herald replacement may be quite serious. There is no
good way to subscribe certain topics based on file paths or commit
descriptions. CODEOWNERS exists but sometimes people just want to
serve as a gatekeeper of some component but don't feel confident
enough to act as a code owner. Moreover, making a commit modifying a
CODEOWNERS file to express subscription does not sound like a good
idea.

> ====>
> I'd love to leave some comments on other comments as I've been
using Github for OSS/Commercial development for many years now and I find it to
be one of the best things that can happen to an open source project.
>
> > "Github UI is ugly/slaggish/bad"
>
> that's obviously subjective, and I can only say the same about
phabricator.
>
> > Geopolitics/laws/whatnot
>
> this is orthogonal to the subject, right?
>
> >  There is no way to see previous version of the patch.
> >  I don't think there is any way to disable force-push for
PR's.
> >  While this is only 'slightly' limiting for the reviewer,
> >  this can be more limiting for the author - how do i go back
> >  to previous revision? I can't, i need to maintain a copy
> >  of every branch i pushed manually.
>
> Seeing previous version is only possible if the author uses different
commits, but is it not possible to enforce it.
> Force pushes can only be disabled per repo, so if we allow to submit PRs
without forks, then it is possible to solve this issue as well.
>
> >  Impossible to chain reviews - a PR diff can only be made
> >  on top of git master branch.
>
> This is also true only for forked repositories.
> Though, imo, chained PRs (feature branch based development) contradicts to
the trunk-based approach LLVM project is using.
> Correct me if I'm wrong.
>
> >  Just recently I submitted a very tiny PR for PcapPlusPlus on GitHub
> >  which involved:
> >  1). Forking the repository
> >  2). Cloning the fork
> >  3). Applying the patch
> >  4). Pushing it and creating a PR.
> >  5). Deleting the fork (Since I didn't want it cluttering up my
account
> >  and I'm not a regular contributor to PcapPlusPlus).
>
> 2) and 3) are not necessary since the repo can have multiple remotes.
> For some very simple cases one doesn't even need to leave github UI: it
is possible to send patches right from the website.
>
> On 7. Nov 2019, at 06:32, Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>
> Hi all,
>
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
>
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
>
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
>
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
>
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
>
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
>
> Cheers,
>
> --
> Mehdi
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


-- 
宋方睿

Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Hi all,
>
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an
> experimental channel alongside the existing methods for contributing to
> LLVM.
+1.  I would not want to see a dual PR/Phab situation last for long,
though.  Looking in two places is a lot more work.

The main reasons I see for moving to GitHub PRs:

- Lowers the barrier for new contributors

- Is a de-facto industry standard; lots of tools know how to work with
  GitHub PRs. almost none with Phab

- Less server maintenance by the project

I am sympathetic to Roman's concerns.  I haven't used GitHub extensively
myself.  On the couple of projects I've contributed to I see e-mail from
GitHub issues but I can't recall seeing one from pull requests (though
the projects are so small maybe the PRs don't have any comments).  It
would be great if GitHub could send e-mails on PR comments and process
replies as new comments on the PR.  Maybe this is something the GitHub
developers could look into.

As for export controls, firewalling, etc. that's way beyond my
understanding but it would seem to come with any move from project-run
servers to servers we don't directly control.  I would think just using
GitHub to host the sources already gets us into those waters.

                         -David

Very strong -1 at the moment.  I think we need to let some of the other
efforts (e.g. issues vs bugzilla) settle out first.

Weak -1 in general.  I'm not strongly opposed, but I see very little
value in this migration and a lot of headache.  Others have explained
this well, so I'll mostly skip that.  I particular dislike the assumed
fork model; I work in patches, so that's a ton of overhead process wise. 

One exception for me would be docs.  If we could open pull requests -
and possibly the web-edit tool for folks with commit access? - for the
docs subtree I could see a lot of advantage in making those easy for new
contributors to update.  It might also be a good proving ground for the
workflow as a whole.

Philip

On 11/6/19 9:32 PM, Mehdi AMINI via llvm-dev wrote:
> Hi all,
>
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
> as an experimental channel alongside the existing methods for
> contributing to LLVM.
> This would allow to find workflow issues and address them, and also
> LLVM contributors in general to start getting familiar with
> pull-requests without committing to switching to pull-requests
> immediately. The community should evaluate after a few months what
> would the next steps be.
>
> GitHub pull-requests is the natural way to contribute to project
> hosted on GitHub: this feature is so core to GitHub that there is no
> option to disable it! 
>
> The current proposal is to allow to integrate contributions to the
> LLVM project directly from pull-requests. In particular the exact
> setup would be the following:
>
>   - Contributors should use their own fork and push a branch in their
> fork.
>   - Reviews can be performed on GitHub. The canonical tools are still
> the mailing-list and Phabricator: a reviewer can request the review to
> move to Phabricator.
>   - The only option available will be to “squash and merge”. This mode
> of review matches the closest our current workflow (both phabricator
> and mailing-list): conceptually independent contributions belongs to
> separate review threads, and thus separate pull-requests. 
> This also allow the round of reviews to not force-push the original
> branch and accumulate commits: this keeps the contextual history of
> comments and allow to visualize the incremental diff across revision
> of the pull-request. 
>   - Upon “merge” of a pull-request: *history is linear* and a single
> commit lands in master after review is completed.
>
> As an alternative staging proposal: we could enable pull-requests only
> for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to
> begin with for example) in the repo. In this case, we would propose to
> begin with the MLIR project (as soon as it gets integrated in the
> monorepo). This would be a good candidate to be the guinea pig for
> this process since it does not yet have a wide established community
> of contributors, and the current contributors are already exclusively
> using pull-requests <https://github.com/tensorflow/mlir/pulls>.
>
> Here is a more complete doc on the topic:
>
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
>
> Cheers,
>
> -- 
> Mehdi
>
>
> _______________________________________________
> 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/20191108/afb106a1/attachment.html>

On 08.11.2019 19:06, David Greene via llvm-dev wrote:
>> Now that we're on GitHub, we can discuss about pull-requests.
>> I'd like to propose to enable pull-request on GitHub, as a first
step as an
>> experimental channel alongside the existing methods for contributing to
>> LLVM.
> 
> +1.  I would not want to see a dual PR/Phab situation last for long,
> though.  Looking in two places is a lot more work.
> 
> The main reasons I see for moving to GitHub PRs:
> 
> - Lowers the barrier for new contributors
> 
There are more higher barriers to new contributors than Phabricator.
I also don't understand what's so complex with it?
Other than the necessity to add -U9999 to git diff ;)

On Fri, Nov 8, 2019 at 8:08 AM Philip Reames <listmail at
philipreames.com>
wrote:
> Very strong -1 at the moment.  I think we need to let some of the other
> efforts (e.g. issues vs bugzilla) settle out first.
>
Sorry I really don’t see a relationship with issues, this seems entirely
independent to me. I even actually see this as easier than issues.

Weak -1 in general.  I'm not strongly opposed, but I see very little
value
> in this migration and a lot of headache.  Others have explained this well,
> so I'll mostly skip that.
>
I am not sure what « headache » your referring to at the moment.

  I particular dislike the assumed fork model; I work in patches, so
that's
> a ton of overhead process wise.
>
>
Sorry but pull request and forks are strictly less overhead process wise,
especially compared to working with patches.
What you’re writing comes across to me as « not having switched to git yet
».

This is also the reason why we need months of trial for everyone who hasn’t
used it extensively before to be able to give it a complete and honest try.

Again I’d be perfectly happy to be ginea pig in our sub-project as well to
begin with: it may even bring people to contribute just for trying
pull-requests :)


—
Mehdi


One exception for me would be docs.  If we could open pull requests -
and
> possibly the web-edit tool for folks with commit access? - for the docs
> subtree I could see a lot of advantage in making those easy for new
> contributors to update.  It might also be a good proving ground for the
> workflow as a whole.
>
> Philip
> On 11/6/19 9:32 PM, Mehdi AMINI via llvm-dev wrote:
>
> Hi all,
>
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as
> an experimental channel alongside the existing methods for contributing to
> LLVM.
> This would allow to find workflow issues and address them, and also LLVM
> contributors in general to start getting familiar with pull-requests
> without committing to switching to pull-requests immediately. The community
> should evaluate after a few months what would the next steps be.
>
> GitHub pull-requests is the natural way to contribute to project hosted on
> GitHub: this feature is so core to GitHub that there is no option to
> disable it!
>
> The current proposal is to allow to integrate contributions to the LLVM
> project directly from pull-requests. In particular the exact setup would be
> the following:
>
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
> mailing-list and Phabricator: a reviewer can request the review to move to
> Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
> review matches the closest our current workflow (both phabricator and
> mailing-list): conceptually independent contributions belongs to separate
> review threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
> and accumulate commits: this keeps the contextual history of comments and
> allow to visualize the incremental diff across revision of the
> pull-request.
>   - Upon “merge” of a pull-request: *history is linear* and a single
> commit lands in master after review is completed.
>
> As an alternative staging proposal: we could enable pull-requests only for
> a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with
> for example) in the repo. In this case, we would propose to begin with the
> MLIR project (as soon as it gets integrated in the monorepo). This would be
> a good candidate to be the guinea pig for this process since it does not
> yet have a wide established community of contributors, and the current
> contributors are already exclusively using pull-requests
> <https://github.com/tensorflow/mlir/pulls>.
>
> Here is a more complete doc on the topic:
>
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
>
> Cheers,
>
> --
> Mehdi
>
>
> _______________________________________________
> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://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/20191108/e7405c6c/attachment.html>

Daniel Sanders via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Personally, I'd like us to drop the linear history requirement but I
> know there's some strong opposition there. The main reason I'd like
us
> to accept non-linear history in conjunction with pull requests is that
> it means that the commits that were tested (whether by the author or
> by CI) still exist in the repository after the merge. If we
> rebase+push like we do today or squash+merge as in this proposal, then
> the information that a particular commit passed testing (or failed a
> particular test) gets lost.
-1 from me.  We have one project where we decided to "keep the original
commits" as you say above and another where we enforce linear history.
The linear history project is *much* easier to follow.  With merge
commits you start running into issues like foxtrot commits
(https://blog.developer.atlassian.com/stop-foxtrots-now) and the whole
thing easily becomes a mess.  There may be various rules/plugins/etc.
that attempt to enforce "good" behavior but in the long run my
experience has been that simpler rules make things simpler.

                     -David

Philip Reames via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Weak -1 in general.  I'm not strongly opposed, but I see very little
> value in this migration and a lot of headache.  Others have explained
> this well, so I'll mostly skip that.  I particular dislike the assumed
> fork model; I work in patches, so that's a ton of overhead process
wise. 
Can you explain this a bit more?  The way I anticipate doing this:

1. Create fork once for all time
2. Set a remote for the fork in my local clone once for all time

while ((I am employed or financially independent) and working on LLVM)
  1. Make a commit
  2. Push to fork remote
  3. Open PR (probably right in emacs using Magit)
  4. Respond to comments, push further commits, squash as needed, etc.
  5. Declare done and request CI/merge
elihw

I would never delete the fork or the fork remote.
> One exception for me would be docs.  If we could open pull requests -
> and possibly the web-edit tool for folks with commit access? - for the
> docs subtree I could see a lot of advantage in making those easy for new
> contributors to update.  It might also be a good proving ground for the
> workflow as a whole.
That's a really great idea!

                  -David

I have a few reactions to this proposal

The first is that I do think changing too many parts of the project
infrastructure in a short space of time is not sensible. LLVM has just finished
a big SVN to git migration, which for builders and other tooling hasn’t been
particularly easy. It’s not clear that the tooling and documentation has
entirely caught up with this migration, but I’m sure it will. There has been a
proposal to move away from bugzilla, which does seem like a pressing concern, of
more importance.

Secondly, I do not think the github pull request system is nearly as good as
phabricator’s patch review system. I use “patch history” during every single
review (something github entirely lacks), as well as the ability to mark parent
and child patches. I also find the comment interface on phabricator to
accurately show all the discussion and feedback, both in context (inline
comments), and chronologically. In contrast, I regularly find github decides not
to load comments, or marks them as resolved before they are (and hides them). I
do not wish the quality of LLVM’s codebase to be beholden to the choices github
makes around which parts of the discussion to show me. And the issues with
subscribing to github issues carry over to subscribing to github pull requests.

I understand that this proposal, in its original form, does not deprecate
Phabricator, but it seems like the discussion has moved far closer towards this
point.

In a more general point, some of these major decisions are very hard to gauge
the sentiment through the mailing list. I know that some other communities run
yearly surveys to understand the state of the ecosystem and their community’s
feelings on some of the more far-reaching changes to the project - maybe now is
a good moment for one, given we have so many proposals up for consideration
(this one, github issues, the build system simplification)?

Sam
> On 7 Nov 2019, at 5:32 am, Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi all,
> 
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
> 
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
> 
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
> 
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
> 
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
> 
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> 
> Cheers,
> 
> -- 
> Mehdi
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
--
Sam Elliott
Software Developer - LLVM
lowRISC CIC
--

Mehdi and team, thanks for taking the time to make process improvements
like this one. I hope we are able to find compromise or alleviate concerns
raised in this thread because I think it's a significant step forward to
switch to PRs. I look forward to having a streamlined way to stage my
patches for review and pre merge test.

I'd like to request that if we end up moving to PRs that we consider a long
term roadmap that allows for devs to summon pre merge test easily if not
automatically. And it would be ideal if I could maximize the scope of tests
(even if that means my commit would be optimistically included in a batch
for in-depth tests, even if maximal testing is opt-in).

Independent testing of my commit puts me at ease about submitting content
upstream and I suspect I'm not alone.  Reducing the likelihood of post
merge buildbot fallout means I would have to babysit my commits less (when
is it safe to log off and assume my commit has run the gauntlet?). Also I
really hate being responsible for a regression. Seems like some bots easily
catch "simple" things that are also pretty easy to forget to check.

Anyways, thanks again to the team for the tireless effort to make progress
here.  It's really appreciated!


On Wed, Nov 6, 2019, 11:32 PM Mehdi AMINI via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi all,
>
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as
> an experimental channel alongside the existing methods for contributing to
> LLVM.
> This would allow to find workflow issues and address them, and also LLVM
> contributors in general to start getting familiar with pull-requests
> without committing to switching to pull-requests immediately. The community
> should evaluate after a few months what would the next steps be.
>
> GitHub pull-requests is the natural way to contribute to project hosted on
> GitHub: this feature is so core to GitHub that there is no option to
> disable it!
>
> The current proposal is to allow to integrate contributions to the LLVM
> project directly from pull-requests. In particular the exact setup would be
> the following:
>
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
> mailing-list and Phabricator: a reviewer can request the review to move to
> Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
> review matches the closest our current workflow (both phabricator and
> mailing-list): conceptually independent contributions belongs to separate
> review threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
> and accumulate commits: this keeps the contextual history of comments and
> allow to visualize the incremental diff across revision of the
> pull-request.
>   - Upon “merge” of a pull-request: *history is linear* and a single
> commit lands in master after review is completed.
>
> As an alternative staging proposal: we could enable pull-requests only for
> a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with
> for example) in the repo. In this case, we would propose to begin with the
> MLIR project (as soon as it gets integrated in the monorepo). This would be
> a good candidate to be the guinea pig for this process since it does not
> yet have a wide established community of contributors, and the current
> contributors are already exclusively using pull-requests
> <https://github.com/tensorflow/mlir/pulls>.
>
> Here is a more complete doc on the topic:
>
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
>
> Cheers,
>
> --
> Mehdi
>
> _______________________________________________
> 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/20191112/b8f0dc8e/attachment.html>

On Tue, Nov 12, 2019 at 7:03 PM Brian Cain <brian.cain at gmail.com>
wrote:
> Mehdi and team, thanks for taking the time to make process improvements
> like this one. I hope we are able to find compromise or alleviate concerns
> raised in this thread because I think it's a significant step forward
to
> switch to PRs. I look forward to having a streamlined way to stage my
> patches for review and pre merge test.
>
> I'd like to request that if we end up moving to PRs that we consider a
> long term roadmap that allows for devs to summon pre merge test easily if
> not automatically. And it would be ideal if I could maximize the scope of
> tests (even if that means my commit would be optimistically included in a
> batch for in-depth tests, even if maximal testing is opt-in).
>
You may be pleased to hear that this is already a work-in-progress, and it
is decoupled from Phabricator vs Pull-requests: while GitHub has a lot of
nice integration (and the mass effects makes it a natural fit for
third-party to write integration tool as "GitHub
Pull-requests"-first),
Phabricator also has APIs that allow to plug pre-merge testing (it is
annoying though that the little engineer resources we have to dedicate to
infrastructure would be sunk into implementing things that "just
works" on
GitHub though).

Here is the public repo for the prototype:
https://github.com/google/llvm-premerge-checks ; feel free to contact
Christian
<http://lists.llvm.org/pipermail/llvm-dev/2019-November/136584.html> if
you'd like to beta-test or get more info on this!

Best,

-- 
Mehdi


>
> Independent testing of my commit puts me at ease about submitting content
> upstream and I suspect I'm not alone.  Reducing the likelihood of post
> merge buildbot fallout means I would have to babysit my commits less (when
> is it safe to log off and assume my commit has run the gauntlet?). Also I
> really hate being responsible for a regression. Seems like some bots easily
> catch "simple" things that are also pretty easy to forget to
check.
>
> Anyways, thanks again to the team for the tireless effort to make progress
> here.  It's really appreciated!
>
>
> On Wed, Nov 6, 2019, 11:32 PM Mehdi AMINI via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi all,
>>
>> Now that we're on GitHub, we can discuss about pull-requests.
>> I'd like to propose to enable pull-request on GitHub, as a first
step as
>> an experimental channel alongside the existing methods for contributing
to
>> LLVM.
>> This would allow to find workflow issues and address them, and also
LLVM
>> contributors in general to start getting familiar with pull-requests
>> without committing to switching to pull-requests immediately. The
community
>> should evaluate after a few months what would the next steps be.
>>
>> GitHub pull-requests is the natural way to contribute to project hosted
>> on GitHub: this feature is so core to GitHub that there is no option to
>> disable it!
>>
>> The current proposal is to allow to integrate contributions to the LLVM
>> project directly from pull-requests. In particular the exact setup
would be
>> the following:
>>
>>   - Contributors should use their own fork and push a branch in their
>> fork.
>>   - Reviews can be performed on GitHub. The canonical tools are still
the
>> mailing-list and Phabricator: a reviewer can request the review to move
to
>> Phabricator.
>>   - The only option available will be to “squash and merge”. This mode
of
>> review matches the closest our current workflow (both phabricator and
>> mailing-list): conceptually independent contributions belongs to
separate
>> review threads, and thus separate pull-requests.
>> This also allow the round of reviews to not force-push the original
>> branch and accumulate commits: this keeps the contextual history of
>> comments and allow to visualize the incremental diff across revision of
the
>> pull-request.
>>   - Upon “merge” of a pull-request: *history is linear* and a single
>> commit lands in master after review is completed.
>>
>> As an alternative staging proposal: we could enable pull-requests only
>> for a small subset of sub-projects in LLVM (i.e. not LLVM/clang to
begin
>> with for example) in the repo. In this case, we would propose to begin
with
>> the MLIR project (as soon as it gets integrated in the monorepo). This
>> would be a good candidate to be the guinea pig for this process since
it
>> does not yet have a wide established community of contributors, and the
>> current contributors are already exclusively using pull-requests
>> <https://github.com/tensorflow/mlir/pulls>.
>>
>> Here is a more complete doc on the topic:
>>
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
>>
>> Cheers,
>>
>> --
>> Mehdi
>>
>> _______________________________________________
>> 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/20191112/351c6506/attachment.html>

+1 from me, if just for the sake of lowering the entry barrier.

As for some of the discussion this thread has originated (apologies if I missed
some of the conversation around them):

* Phabricator vs GitHub: they both have good and bad features. It will always be
a matter of personal preference. IMHO, more solid criterias for a choice are 1)
keep everything (or as much as we can) in a single place, and as I already said
2) lower the entry barrier.

* Concerns around export control of GitHub to certain countries. Surely in such
case the major concerns wouldn’t be for doing the code reviews, but for getting
access to the code? Speaking off which: do we mirror the monorepo somewhere else
other than just having it on GitHub?

* Low code quality of new contributions, and possible breakage of other people
builds: can we have CI integrated with GitHub? That should lower a lot the risk
of having incoming things that break things.

Thanks,

Francesco

> On Nov 6, 2019, at 11:32 PM, Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi all,
> 
> Now that we're on GitHub, we can discuss about pull-requests.
> I'd like to propose to enable pull-request on GitHub, as a first step
as an experimental channel alongside the existing methods for contributing to
LLVM.
> This would allow to find workflow issues and address them, and also LLVM
contributors in general to start getting familiar with pull-requests without
committing to switching to pull-requests immediately. The community should
evaluate after a few months what would the next steps be.
> 
> GitHub pull-requests is the natural way to contribute to project hosted on
GitHub: this feature is so core to GitHub that there is no option to disable it!
> 
> The current proposal is to allow to integrate contributions to the LLVM
project directly from pull-requests. In particular the exact setup would be the
following:
> 
>   - Contributors should use their own fork and push a branch in their fork.
>   - Reviews can be performed on GitHub. The canonical tools are still the
mailing-list and Phabricator: a reviewer can request the review to move to
Phabricator.
>   - The only option available will be to “squash and merge”. This mode of
review matches the closest our current workflow (both phabricator and
mailing-list): conceptually independent contributions belongs to separate review
threads, and thus separate pull-requests.
> This also allow the round of reviews to not force-push the original branch
and accumulate commits: this keeps the contextual history of comments and allow
to visualize the incremental diff across revision of the pull-request.
>   - Upon “merge” of a pull-request: history is linear and a single commit
lands in master after review is completed.
> 
> As an alternative staging proposal: we could enable pull-requests only for
a small subset of sub-projects in LLVM (i.e. not LLVM/clang to begin with for
example) in the repo. In this case, we would propose to begin with the MLIR
project (as soon as it gets integrated in the monorepo). This would be a good
candidate to be the guinea pig for this process since it does not yet have a
wide established community of contributors, and the current contributors are
already exclusively using pull-requests.
> 
> Here is a more complete doc on the topic:
https://docs.google.com/document/d/1DSHQrfydSjoqU9zEnj3rIcds6YN59Jxc37MdiggOyaI
> 
> Cheers,
> 
> -- 
> Mehdi
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev