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

+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>

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 <rnk at google.com> 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 <llvm-dev
at lists.llvm.org <mailto: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 <mailto: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 <http://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 <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 <mailto:llvm-dev at lists.llvm.org>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/20200123/0fa99eec/attachment.html>

On Thu, Jan 23, 2020 at 7:54 PM Florian Hahn via llvm-dev <
llvm-dev at lists.llvm.org> 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 <rnk at google.com> 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 <
> 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
>>
>
> _______________________________________________
> 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/20200123/4cb1a205/attachment.html>