[llvm-dev] Enable Contributions Through Pull-request For LLVM
Neil Henning via llvm-dev
llvm-dev at lists.llvm.org
Thu Nov 7 03:12:03 PST 2019
I know from experience that many people have not filed issues due to
bugzilla, and not submitted PRs due to phabricator.
An example of where a completely new contributor (my colleague Aras who
contributed the build-cost stats to Clang for 9.0.0) intuitively used
GitHub is here https://github.com/aras-p/llvm-project-20170507/pull/2 -
which I think really awesomely shows how quick a newcomer to the ecosystem
could do something really useful and productive when they are channeled
through tools they are already used to (GitHub issues/PRs).
So from us it's a big +1.
On Thu, Nov 7, 2019 at 10:31 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Having been using Github internally for code reviews of private patches on
> LLVM, and Phabricator for upstream ones, I've found the latter to be far
> easier to use. Prior to working with LLVM, I had basically no experience
> with either, so I'd say I'm coming from a fairly neutral starting point.
> Here are some of my observations (I want to highlight 9 as a particular
> issue, due to other recent discussions I've seen on the mailing list):
>
> 1) I don't know of a natural way to chain related patches together on
> Github (aside from explicitly mentioning them), but as separate reviews.
> This is useful for bigger features/refactorings where each individual step
> provides some benefit, but seeing the bigger picture is easy. Phabricator
> has the child/parent objects option.
> 2) Phabricator allows placing comments anywhere in the diff context, which
> is useful when the commit affects lines that haven't actually been changed,
> and those lines need addressing/referencing in some way. Github doesn't
> allow comments outside the immediate context around changed files
> 3) As a +1 to Github on the other hand, commits always come with full
> context, so you don't have to remind people to include it.
> 4) Phabricator's ability to see what has changed since the previous time
> you commented seems to be much more reliable than what Github provides.
> 5) With a Github PR, if you want to include a minor change to the patch
> prior to committing, you have to commit it to the PR, if you wish to use
> the UI to do the merging. Of course, you could just not push the patch via
> Github.
> 6) I myself have on numerous occasions messed up my commit message when
> committing via the Github UI, because it's not obvious unless you are a
> seasoned user that the title of the commit appears as the first line of the
> commit message. This is important when doing a squash and merge.
> 7) The PR approach does at least allow committing via the UI, which is
> perhaps a little less fiddly in some cases.
> 8) I rarely bother creating a branch for Phabricator reviews, because I
> don't need it (I'm often not working on multiple things at once), so the
> extra hassle of creating a branch/checking it out etc that Github requires
> is annoying. On the other hand, Github PRs are generally quite easy to
> create once you have pushed a branch.
> 9) On the note of branches for PRs, don't they require users to push their
> local branches to the remote repo to create? That means we'll end up
> thousands of branches in git. Not sure that this will do performance any
> good, and I seem to remember there was general agreement that we didn't
> want people to push their branches generally. Yes, in theory branches
> should be deleted after they're merged, but I've seen that locally not
> happen regularly, and that's even assuming that all PRs get merged in (they
> won't).
> 10) Expanding context on Github is a pain: there is no option to just
> expand the whole context in a block, which means that if you need to see
> something much earlier in a large file, you have to click a LOT. Also, the
> browser view often doesn't then go where you expect it to from my
> experience. Phabricator has a "expand all N lines" option.
> 11) Small one this one, but missing new lines at end of file are much more
> obvious on Phabricator than Github.
> 12) Not sure if this is a real issue, but Github reviewers are limited in
> number (I think it's 15?). To my knowledge, there is no such limit with
> Phabricator (but then how often do you end up with 15 people marked as
> reviewers?).
>
> I'm sure I could come up with other points for/against Github PRs. On
> balance I definitely prefer Phabricator.
>
> James
>
> On Thu, 7 Nov 2019 at 09:53, Aaron Ballman via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> 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
>> _______________________________________________
>> 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
>
--
Neil Henning
Senior Software Engineer Compiler
unity.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191107/a2d1ba67/attachment.html>
More information about the llvm-dev
mailing list