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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Jeremy Morse via llvm-dev
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
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 <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via 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

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


> On Jan 20, 2020, at 22:09, David Tellenbach <[hidden email]> 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.


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


> On Jan 20, 2020, at 22:44, Martin Storsjö <[hidden email]> wrote:
>
> 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.


This should be exactly what the current script in the linked patch does: download, apply and commit the latest diff for a review to the current branch, ignoring the base revision. Setting the author based on the review should be trivial to add as well.

Cheers,
Florian
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Neil Henning
Senior Software Engineer Compiler

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
On Tue, Jan 21, 2020 at 11:54 AM Neil Henning via llvm-dev <[hidden email]> wrote:
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.

I think it is useful in that it shows good-will and a way forward for fixing the issues that people seem to have with Phabricator.

I have yet to see similar initiative among those who favor GitHub PRs. That may be in part because one of the main problems of GitHub *cannot* be fixed by outside parties, but only by GitHub themselves. Being beholden to an external party like that is a bad thing, and was in fact the reason for creating Git in the first place...

Cheers,
Nicolai
 


Cheers,
-Neil.

On Tue, Jan 21, 2020 at 9:19 AM James Henderson via llvm-dev <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Neil Henning
Senior Software Engineer Compiler
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
+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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
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
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
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
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
Zachary Turner via llvm-dev <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


On 21 Jan 2020, at 02:59, Nicolai Hähnle via llvm-dev <[hidden email]> wrote:

On Tue, Jan 21, 2020 at 11:54 AM Neil Henning via llvm-dev <[hidden email]> wrote:
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.

I think it is useful in that it shows good-will and a way forward for fixing the issues that people seem to have with Phabricator.


This was indeed my intention. Mainly I wanted to show that we could provide a very streamlined way for basic patch management git <-> Phabricator (single command for both patch creation and applying a patch) in tree without too much effort.

Cheers,
Florian

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
Thanks for all the replies! 

From the responses it seems like there is a bit of interest in a solution in tree, but there are also plenty of people mostly happy with the current tools/workflows (arc / website upload). I think it is great that there are multiple tools/workflows and it’s great they work well for people. Personally I am quite happy with arc, which works fine for me after getting it set up. 

However it seems like there are at least some contributors (and potential contributors) who’s workflow would slightly benefit from having something in-tree. I realise that any work on such a script might be ‘wasted’ because there are other alternatives already or if we switch to a different review tool. But I do not think that should be a blocker for adding a script like that, if there are people willing to invest the time knowing it might be wasted in the end.

Also adding such a script should not add any maintenance burden outside the script itself. And that should be limited to the users of the script, as long as we not endorse it too much. If it turns out there are no users or no-one interested in maintaining the script, it should be easy to remove.

Personally, I think the benefits would outweigh the extra work, but if the consensus is that we shouldn’t add something custom, I am happy to abandon the patch.

As others already mentioned, I think there’s also potential for improving our docs related to Arc. I think that’s something we should do regardless of any custom script. IMO it is an advantage to provide a choice of tools.

Apologies if I missed anything in my attempt of a summary!

Cheers,
Florian



On 21 Jan 2020, at 13:53, Reid Kleckner <[hidden email]> wrote:

+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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


> On 22 Jan 2020, at 17:39, Joerg Sonnenberger via llvm-dev <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


On Thu, Jan 23, 2020 at 7:54 PM Florian Hahn via llvm-dev <[hidden email]> wrote:
Thanks for all the replies! 

From the responses it seems like there is a bit of interest in a solution in tree, but there are also plenty of people mostly happy with the current tools/workflows (arc / website upload). I think it is great that there are multiple tools/workflows and it’s great they work well for people. Personally I am quite happy with arc, which works fine for me after getting it set up. 

However it seems like there are at least some contributors (and potential contributors) who’s workflow would slightly benefit from having something in-tree. I realise that any work on such a script might be ‘wasted’ because there are other alternatives already or if we switch to a different review tool. But I do not think that should be a blocker for adding a script like that, if there are people willing to invest the time knowing it might be wasted in the end.

Also adding such a script should not add any maintenance burden outside the script itself. And that should be limited to the users of the script, as long as we not endorse it too much. If it turns out there are no users or no-one interested in maintaining the script, it should be easy to remove.

Personally, I think the benefits would outweigh the extra work, but if the consensus is that we shouldn’t add something custom, I am happy to abandon the patch.

As others already mentioned, I think there’s also potential for improving our docs related to Arc. I think that’s something we should do regardless of any custom script. IMO it is an advantage to provide a choice of tools.

If there is nothing specific to LLVM or the LLVM setup in this tool, could it live in its own GitHub project as a standalone tool?

-- 
Mehdi

 

Apologies if I missed anything in my attempt of a summary!

Cheers,
Florian



On 21 Jan 2020, at 13:53, Reid Kleckner <[hidden email]> wrote:

+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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Proposing a llvm-patch helper script in-tree to create/apply patches without arc

Jeremy Morse via llvm-dev


On 23 Jan 2020, at 20:57, Mehdi AMINI <[hidden email]> wrote:




On Thu, Jan 23, 2020 at 7:54 PM Florian Hahn via llvm-dev <[hidden email]> wrote:
Thanks for all the replies! 

From the responses it seems like there is a bit of interest in a solution in tree, but there are also plenty of people mostly happy with the current tools/workflows (arc / website upload). I think it is great that there are multiple tools/workflows and it’s great they work well for people. Personally I am quite happy with arc, which works fine for me after getting it set up. 

However it seems like there are at least some contributors (and potential contributors) who’s workflow would slightly benefit from having something in-tree. I realise that any work on such a script might be ‘wasted’ because there are other alternatives already or if we switch to a different review tool. But I do not think that should be a blocker for adding a script like that, if there are people willing to invest the time knowing it might be wasted in the end.

Also adding such a script should not add any maintenance burden outside the script itself. And that should be limited to the users of the script, as long as we not endorse it too much. If it turns out there are no users or no-one interested in maintaining the script, it should be easy to remove.

Personally, I think the benefits would outweigh the extra work, but if the consensus is that we shouldn’t add something custom, I am happy to abandon the patch.

As others already mentioned, I think there’s also potential for improving our docs related to Arc. I think that’s something we should do regardless of any custom script. IMO it is an advantage to provide a choice of tools.

If there is nothing specific to LLVM or the LLVM setup in this tool, could it live in its own GitHub project as a standalone tool?


Yes it could, but I think one of the main benefits would be to have something that works out-of-the-box without dependencies. There are already multiple tools available with an extra install step, so I don’t think another separate one would add much.


Cheers,
Florian 




-- 
Mehdi

 

Apologies if I missed anything in my attempt of a summary!

Cheers,
Florian



On 21 Jan 2020, at 13:53, Reid Kleckner <[hidden email]> wrote:

+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 <[hidden email]> 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 <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev