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

Thu Jan 23 20:57:08 PST 2020

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>