llvm dev - Jan 2020 - Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Hi,

One takeaway for me from the recent Phabricator vs Github PR discussions was
that arc (arcanist) can be a pain to set up and may pose a hurdle for some
contributors.

I think those points could be addressed relatively easily by adding a llvm-patch
script (or an even better name) that allows users to create and apply patches
from reviews.llvm.org using Phabricators API. In my experience, the three most
common uses cases are:

1. Create a new review from the current HEAD commit in the working directory and
let me optionally add subscribers/reviewers
2. Update the diff for an existing review with the current HEAD commit in the
working directory
3. Download the latest diff for a revision and apply it to the working
directory, using the commit message from Phabricator.

Those should be fairly easy to implement and as a proof-of-concept I went ahead
and put up a patch implementing 3. from the list above:
https://reviews.llvm.org/D73075 .

 Please note that the script is probably a bit rough around the edges and I
probably won’t have time to implement 1. and 2. in the near future on my own,
but maybe someone would be interested in helping out.

I think that could improve the experience for new contributors substantially: 
 * Create a new patch: `llvm-patch upload`
 * Apply a patch from a review: `llvm-patch apply D12345`. 

WDYT?

Cheers,
Florian

Hi,

although I think making the arcanist workflow more accessible is an excellent
idea I'm not entirely sure if providing a custom script is the best solution
(yet).

A lower hanging fruit would be to have a good and comprehensive documentation,
ideally including some example workflows, for using arcanist with LLVM. While
the Phabricator documentation on arcanist is quite helpful for getting started
it's not very detailed. Our own docs on the topic are basically
non-existent.

The downsides of providing a custom script are obvious: Someone has to write it
and someone has to maintain it. Moreover I don't think that arcanist is so
complicated that a custom script would be a huge improvement (you'd have to
read the documentation of the new tool anyway).

If a well organized documentation still doesn't resolve the problem of
arcanist being too confusing (uncommon, ...) we could definitely think about
providing a custom script, I just think it's not the first logical step.

Thanks,
David
> On 21. Jan 2020, at 03:15, Florian Hahn via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi,
> 
> One takeaway for me from the recent Phabricator vs Github PR discussions
was that arc (arcanist) can be a pain to set up and may pose a hurdle for some
contributors.
> 
> I think those points could be addressed relatively easily by adding a
llvm-patch script (or an even better name) that allows users to create and apply
patches from reviews.llvm.org using Phabricators API. In my experience, the
three most common uses cases are:
> 
> 1. Create a new review from the current HEAD commit in the working
directory and let me optionally add subscribers/reviewers
> 2. Update the diff for an existing review with the current HEAD commit in
the working directory
> 3. Download the latest diff for a revision and apply it to the working
directory, using the commit message from Phabricator.
> 
> Those should be fairly easy to implement and as a proof-of-concept I went
ahead and put up a patch implementing 3. from the list above:
https://reviews.llvm.org/D73075 .
> 
> Please note that the script is probably a bit rough around the edges and I
probably won’t have time to implement 1. and 2. in the near future on my own,
but maybe someone would be interested in helping out.
> 
> I think that could improve the experience for new contributors
substantially:
> * Create a new patch: `llvm-patch upload`
> * Apply a patch from a review: `llvm-patch apply D12345`. 
> 
> WDYT?
> 
> Cheers,
> Florian
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Tue, 21 Jan 2020, David Tellenbach via llvm-dev wrote:
> The downsides of providing a custom script are obvious: Someone has to 
> write it and someone has to maintain it. Moreover I don't think that 
> arcanist is so complicated that a custom script would be a huge 
> improvement (you'd have to read the documentation of the new tool 
> anyway).
To me, one upside would be if it simply would do less; if I want to apply 
a patch using arcanist, it will automatically change branches to apply the 
patch on the right base revision (or maybe just fetch the very latest 
master and apply it on top of that). As the llvm-project repo is fairly 
large, rebuilds are costly, and updating the git tree to a new version is 
something I want to schedule myself, not have a tool do for me behind my 
back when I want to try out a patch (that most probably would work just 
fine on my couple-days-old tree).

I would much prefer a tool that simply tries to apply the change on the 
current branch that I happen to have checked out at the moment - 
essentially "git am". I've tried to look for arcanist flags to
achieve
this, but didn't find any when I looked.

So a simple standalone tool (without the php dependency) that does less 
automatic stuff and just downloads and applies the patch would be great to 
me. Bonus points if it would set the git author field properly based on 
the patch author's phabricator user name/mail address.

// Martin

> On Jan 20, 2020, at 22:09, David Tellenbach <david.tellenbach at
me.com> wrote:
> 
> Hi,
> 
> although I think making the arcanist workflow more accessible is an
excellent idea I'm not entirely sure if providing a custom script is the
best solution (yet).
> 
> A lower hanging fruit would be to have a good and comprehensive
documentation, ideally including some example workflows, for using arcanist with
LLVM. While the Phabricator documentation on arcanist is quite helpful for
getting started it's not very detailed. Our own docs on the topic are
basically non-existent.
> 
> The downsides of providing a custom script are obvious: Someone has to
write it and someone has to maintain it. Moreover I don't think that
arcanist is so complicated that a custom script would be a huge improvement
(you'd have to read the documentation of the new tool anyway).
> 
I agree, improving the documentation will also go a long way! But there’s still
the PHP issue that some people run into. Also, the script would be much more
focused, which also should make —help more useful than arc’s, which shows you
the 20+ different commands.

To clarify, I am not proposing to get rid of arc or switching the docs to the
custom script. I’d just like to highlight that writing and maintaining such a
script for the most common uses cases should not be much work, in the grand
scheme of things. If people are interested in collaborating on such a script, I
think the burden of adding it would be quite low.

I like the concept, and a quick glance at the script is essentially what
I'd expect, though I haven't looked at it in depth. I'm quite okay
with
jumping through the current hoops needed to do those three items, but they
are also my most common operations, so a script would definitely simplify
things for me. Unfortunately, I don't currently have the time to invest in
it myself.

On Tue, 21 Jan 2020 at 02:16, Florian Hahn via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> One takeaway for me from the recent Phabricator vs Github PR discussions
> was that arc (arcanist) can be a pain to set up and may pose a hurdle for
> some contributors.
>
> I think those points could be addressed relatively easily by adding a
> llvm-patch script (or an even better name) that allows users to create and
> apply patches from reviews.llvm.org using Phabricators API. In my
> experience, the three most common uses cases are:
>
> 1. Create a new review from the current HEAD commit in the working
> directory and let me optionally add subscribers/reviewers
> 2. Update the diff for an existing review with the current HEAD commit in
> the working directory
> 3. Download the latest diff for a revision and apply it to the working
> directory, using the commit message from Phabricator.
>
> Those should be fairly easy to implement and as a proof-of-concept I went
> ahead and put up a patch implementing 3. from the list above:
> https://reviews.llvm.org/D73075 .
>
>  Please note that the script is probably a bit rough around the edges and
> I probably won’t have time to implement 1. and 2. in the near future on my
> own, but maybe someone would be interested in helping out.
>
> I think that could improve the experience for new contributors
> substantially:
>  * Create a new patch: `llvm-patch upload`
>  * Apply a patch from a review: `llvm-patch apply D12345`.
>
> WDYT?
>
> Cheers,
> Florian
> _______________________________________________
> 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/20200121/69af413b/attachment.html>

I'd rather we decided on whether to accept GitHub PRs or not first (in the
other thread). I would bet that everyone who has troubles with arc / is not
allowed to use arc would happily use GitHub PRs instead.

Worst case scenario if the community decides that we don't want to accept
GitHub PRs then this sort of script would be a useful time sink.

Cheers,
-Neil.

On Tue, Jan 21, 2020 at 9:19 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I like the concept, and a quick glance at the script is essentially what
> I'd expect, though I haven't looked at it in depth. I'm quite
okay with
> jumping through the current hoops needed to do those three items, but they
> are also my most common operations, so a script would definitely simplify
> things for me. Unfortunately, I don't currently have the time to invest
in
> it myself.
>
> On Tue, 21 Jan 2020 at 02:16, Florian Hahn via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi,
>>
>> One takeaway for me from the recent Phabricator vs Github PR
discussions
>> was that arc (arcanist) can be a pain to set up and may pose a hurdle
for
>> some contributors.
>>
>> I think those points could be addressed relatively easily by adding a
>> llvm-patch script (or an even better name) that allows users to create
and
>> apply patches from reviews.llvm.org using Phabricators API. In my
>> experience, the three most common uses cases are:
>>
>> 1. Create a new review from the current HEAD commit in the working
>> directory and let me optionally add subscribers/reviewers
>> 2. Update the diff for an existing review with the current HEAD commit
in
>> the working directory
>> 3. Download the latest diff for a revision and apply it to the working
>> directory, using the commit message from Phabricator.
>>
>> Those should be fairly easy to implement and as a proof-of-concept I
went
>> ahead and put up a patch implementing 3. from the list above:
>> https://reviews.llvm.org/D73075 .
>>
>>  Please note that the script is probably a bit rough around the edges
and
>> I probably won’t have time to implement 1. and 2. in the near future on
my
>> own, but maybe someone would be interested in helping out.
>>
>> I think that could improve the experience for new contributors
>> substantially:
>>  * Create a new patch: `llvm-patch upload`
>>  * Apply a patch from a review: `llvm-patch apply D12345`.
>>
>> WDYT?
>>
>> Cheers,
>> Florian
>> _______________________________________________
>> 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/20200121/cedfe1a6/attachment.html>

I went for years without using arc.  The workflow was basically:

1) git format-patch -U999999 HEAD~1
2) Go to the phabricator website and click Create Patch
3) Click Browse and find the patch file that was emitted in step 1.
4) Copy/paste the description of the patch from step 1 into the box.
5) Add reviewers.

It doesn't sound like the proposed script saves you all that many steps.

On Mon, Jan 20, 2020 at 6:16 PM Florian Hahn via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> One takeaway for me from the recent Phabricator vs Github PR discussions
> was that arc (arcanist) can be a pain to set up and may pose a hurdle for
> some contributors.
>
> I think those points could be addressed relatively easily by adding a
> llvm-patch script (or an even better name) that allows users to create and
> apply patches from reviews.llvm.org using Phabricators API. In my
> experience, the three most common uses cases are:
>
> 1. Create a new review from the current HEAD commit in the working
> directory and let me optionally add subscribers/reviewers
> 2. Update the diff for an existing review with the current HEAD commit in
> the working directory
> 3. Download the latest diff for a revision and apply it to the working
> directory, using the commit message from Phabricator.
>
> Those should be fairly easy to implement and as a proof-of-concept I went
> ahead and put up a patch implementing 3. from the list above:
> https://reviews.llvm.org/D73075 .
>
>  Please note that the script is probably a bit rough around the edges and
> I probably won’t have time to implement 1. and 2. in the near future on my
> own, but maybe someone would be interested in helping out.
>
> I think that could improve the experience for new contributors
> substantially:
>  * Create a new patch: `llvm-patch upload`
>  * Apply a patch from a review: `llvm-patch apply D12345`.
>
> WDYT?
>
> Cheers,
> Florian
> _______________________________________________
> 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/20200121/f100b176/attachment.html>

+1 to this. I will not deny that, for whatever reason, people don't seem to
use Arcanist. Using PHP as the scripting language seems to be a major
sticking point for people, since it is not typically preinstalled or
required for normal LLVM development, in the way that Python is. I've done
it, and it works for me.

I think it makes more sense to try and standardize on the existing tool
rather than building our own. If that means writing three docs with steps
for Win, Linux, and Mac, so be it, it will cost less to maintain than
something custom written against the Phab APIs.

On Mon, Jan 20, 2020 at 10:09 PM David Tellenbach via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> although I think making the arcanist workflow more accessible is an
> excellent idea I'm not entirely sure if providing a custom script is
the
> best solution (yet).
>
> A lower hanging fruit would be to have a good and comprehensive
> documentation, ideally including some example workflows, for using arcanist
> with LLVM. While the Phabricator documentation on arcanist is quite helpful
> for getting started it's not very detailed. Our own docs on the topic
are
> basically non-existent.
>
> The downsides of providing a custom script are obvious: Someone has to
> write it and someone has to maintain it. Moreover I don't think that
> arcanist is so complicated that a custom script would be a huge improvement
> (you'd have to read the documentation of the new tool anyway).
>
> If a well organized documentation still doesn't resolve the problem of
> arcanist being too confusing (uncommon, ...) we could definitely think
> about providing a custom script, I just think it's not the first
logical
> step.
>
> Thanks,
> David
>
> > On 21. Jan 2020, at 03:15, Florian Hahn via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >
> > Hi,
> >
> > One takeaway for me from the recent Phabricator vs Github PR
discussions
> was that arc (arcanist) can be a pain to set up and may pose a hurdle for
> some contributors.
> >
> > I think those points could be addressed relatively easily by adding a
> llvm-patch script (or an even better name) that allows users to create and
> apply patches from reviews.llvm.org using Phabricators API. In my
> experience, the three most common uses cases are:
> >
> > 1. Create a new review from the current HEAD commit in the working
> directory and let me optionally add subscribers/reviewers
> > 2. Update the diff for an existing review with the current HEAD commit
> in the working directory
> > 3. Download the latest diff for a revision and apply it to the working
> directory, using the commit message from Phabricator.
> >
> > Those should be fairly easy to implement and as a proof-of-concept I
> went ahead and put up a patch implementing 3. from the list above:
> https://reviews.llvm.org/D73075 .
> >
> > Please note that the script is probably a bit rough around the edges
and
> I probably won’t have time to implement 1. and 2. in the near future on my
> own, but maybe someone would be interested in helping out.
> >
> > I think that could improve the experience for new contributors
> substantially:
> > * Create a new patch: `llvm-patch upload`
> > * Apply a patch from a review: `llvm-patch apply D12345`.
> >
> > WDYT?
> >
> > Cheers,
> > Florian
> > _______________________________________________
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200121/9c0ffcb5/attachment.html>

On Tue, Jan 21, 2020 at 09:39:10AM -0800, Zachary Turner via llvm-dev
wrote:
> I went for years without using arc.  The workflow was basically:
> 
> 1) git format-patch -U999999 HEAD~1
> 2) Go to the phabricator website and click Create Patch
> 3) Click Browse and find the patch file that was emitted in step 1.
> 4) Copy/paste the description of the patch from step 1 into the box.
> 5) Add reviewers.
> 
> It doesn't sound like the proposed script saves you all that many
steps.
Coming from a regular user of "hg phabsend": it sums up, especially if
it can also handle updating the diff.

Joerg

On Mon, Jan 20, 2020 at 06:15:58PM -0800, Florian Hahn via llvm-dev
wrote:
> I think those points could be addressed relatively easily by adding a
> llvm-patch script (or an even better name) that allows users to create
> and apply patches from reviews.llvm.org using Phabricators API.
Anyone from the Mozilla community to shim in on what they are using for
git users? I would hope they have a solution at hand already, hopefully
not involving arcanist.

Joerg

Zachary Turner via llvm-dev <llvm-dev at lists.llvm.org> writes:
> I went for years without using arc.  The workflow was basically:
>
> 1) git format-patch -U999999 HEAD~1
> 2) Go to the phabricator website and click Create Patch
> 3) Click Browse and find the patch file that was emitted in step 1.
> 4) Copy/paste the description of the patch from step 1 into the box.
> 5) Add reviewers.
>
> It doesn't sound like the proposed script saves you all that many
steps.
For me it's more steps because the machine where I have llvm-project
checked out is not my primary browsing machine.  I have to get the patch
file from the remote machine to the local machine.  Admittedly, it's not
hard but it is extra steps.

And no, things like sshfs are not allowed.

                     -David

> On 22 Jan 2020, at 17:39, Joerg Sonnenberger via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On Mon, Jan 20, 2020 at 06:15:58PM -0800, Florian Hahn via llvm-dev wrote:
>> I think those points could be addressed relatively easily by adding a
>> llvm-patch script (or an even better name) that allows users to create
>> and apply patches from reviews.llvm.org using Phabricators API.
> 
> Anyone from the Mozilla community to shim in on what they are using for
> git users? I would hope they have a solution at hand already, hopefully
> not involving arcanist.

I believe someone mentioned that Mozilla has
https://github.com/mozilla-conduit/review