[Proposal] Automatically Cc: cfe-commits@ on Clang reviews

Hi folks,

I’m working with Manuel Klimek and Sam McCall to streamline the code review process for LLVM/Clang/etc. diffs.

tl;dr: I want to set up separate Differential repositories for each LLVM/Clang/etc. project, so Herald can automatically Cc: the correct list for each review request.

I set up a proof-of-concept repository for clang-tools-extra in https://reviews.llvm.org/D40179 and a Herald rule in https://reviews.llvm.org/H268 to confirm this technique works.

Top-Level Goal

When sending a code review through reviews.llvm.org, engineers currently have to remember to manually Cc: cfe-commits@lists.llvm.org on every review request.

We don’t want to unnecessarily Cc: this list for non-Clang review requests, so we need a solution to set up the proper Cc:s for each review.

Current Roadblocks

Normally, Differential’s “repositories” feature allows Herald rules to match reviews to set up Cc:s, etc. easily.

Currently, the LLVM project uses a single Differential repository for all projects, including clang, clang-tools-extra, etc. — but we don’t want to Cc: cfe-commits on review requsts for LLVM so we cannot match on repository.

If we used a monorepo, we could match on path to assign the Cc:, but we have separate .arcconfig files already at the root of each repository.

This means Differential gets project-relative paths in its review requests, which means we cannot use Herald to easily distinguish diffs meant for different projects.

For example, if a review request is for lib/CMakeLists.txt, we have no way to know if that is for llvm/lib/CMakeLists.txt or llvm/tools/clang/lib/CMakeLists.txt, etc.

Proposed Solution

Since we already have .arcconfig files at the base of each repository, we can just create separate Differential repositories corresponding to each .arcconfig, then set up Herald rules to automatically Cc: the appropriate list for each review request.

As noted in the tl;dr: above, I set up a proof-of-concept repository for clang-tools-extra in https://reviews.llvm.org/D40179 and a Herald rule in https://reviews.llvm.org/H268 to confirm this technique works.

Potential Issues

  • Separate Differential repositories require a bit more back-end storage and CPU on the Phabricator instance (but aside from LLVM and Clang, I think the other repos are small enough that this won’t be significant.)

  • Moving forward, only LLVM commits will be identified with the prefix rL (as in https://reviews.llvm.org/rL12345) — each project will get its own unique prefix, which might surprise some users or require changes to tools which bake in the rL prefix

  • Moving to a monorepo in the future won’t require too much work, but we will either need to move to a single Differential repository and re-adjust the Herald rules to handle absolute paths, or continue using separate Differential repositories even though the underlying storage is a monorepo.
    Please share thoughts and feedback, and thanks!

Ben

tl;dr: I want to set up separate Differential repositories for each LLVM/Clang/etc. project, so Herald can automatically Cc: the correct list for each review request.

[...]

  * Moving to a monorepo in the future won't require too much work, but
    we will either need to move to a single Differential repository and
    re-adjust the Herald rules to handle absolute paths, or continue
    using separate Differential repositories even though the underlying
    storage is a monorepo.

My understanding was that, while there currently is no official monorepo, it is at least possible and accepted to send single, cross-repo patches for review. (I found that beneficial in the past, and have at least one patch brewing here for which it would be beneficial to not artificially split it up into clang and compiler-rt parts for review.) That would no longer be possible, right?

[Apologies for replying to the wrong message, I somehow forgot to subscribe to cfe-dev so I didn’t get Stephan’s message in my inbox.]

My understanding was that, while there currently is no official monorepo, it is at least possible and accepted to send single, cross-repo patches for review. That would no longer be possible, right?

I assume cross-repo patches would be constructed by hand, right? Or do you mean using the unofficial monorepo from 2016?

An example of an existing cross-repo review in Differential would be nice—do you happen to have one handy?

I think either way, we’d keep the ability to send cross-repo patches to the top-level LLVM repo just the same way you could today.

I’ll confirm that if you can help me construct an example cross-repo patch for review.

Ben

> My understanding was that, while there currently is no official monorepo, it is at least possible and accepted to send single, cross-repo patches for review. That would no longer be possible, right?

I assume cross-repo patches would be constructed by hand, right? Or do you mean using the unofficial monorepo from 2016 <https://github.com/joker-eph/llvm-project&gt;?

I'm using <https://github.com/llvm-project/llvm-project-20170507&gt; (as advertised at <https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo&gt;\), where such a "cross-repo diff" is just a matter of a plain 'git diff'.

An example of an existing cross-repo review in Differential would be nice—do you happen to have one handy?

I just happened to file <https://reviews.llvm.org/D40295&gt; a moment ago, spanning clang and compiler-rt.

I think either way, we'd keep the ability to send cross-repo patches to the top-level LLVM repo just the same way you could today.

I'll confirm that if you can help me construct an example cross-repo patch for review.

Thanks,
Stephan

OK. I confirmed that Stephan’s process to send out cross-repo diffs from the monorepo is not affected by my proposal.

tl;dr: https://reviews.llvm.org/D40312 (which was sent out on top of D40179 to emulate what happens with a monorepo after my proposal lands).

Steps I took (starting with https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo):

+llvm-dev, so we get wider input :slight_smile:

Given how unfortunate reviews that are started without cc’ing the right list are (basically folks need to re-send the review from scratch), I support this idea.

Ben, couldn’t rL still be available for all projects? (and be the main project for LLVM).

Absolutely — I should have mentioned that we would keep the main rL project and continue to use it.

Absolutely — I should have mentioned that we would keep the main rL project and continue to use it.

Your original email said “Moving forward, only LLVM commits will be identified with the prefix rL (as in https://reviews.llvm.org/rL12345) — each project will get its own unique prefix, which might surprise some users or require changes to tools which bake in the rL prefix”.

That seems to not be an issue, as long as all commits for all projects are still reachable under the rL tag, right?

Right. I think the commits will show up with multiple (two, I think) projects, which should be fine.

Right. I think the commits will show up with multiple (two, I think) projects, which should be fine.

Sounds good!

Hi Ben,
(+llvm-dev since the email I’m replying to wasn’t sent there)

2017-11-21 17:18 GMT+01:00 Ben Hamilton via cfe-dev <cfe-dev@lists.llvm.org>:

Good point, and sorry for the confusion.

Following the monorepo setup instructions, diffs sent out from a monorepo will use llvm/.arcconfig.

I haven’t committed any changes to that .arcconfig yet, and in addition there aren’t yet any Herald rules which copy llvm-commits@ on review requests sent out to the LLVM project.

So, that explains why llvm-commits@ was not copied on D40312.

The previous setup made sense, since review requests for clang et al were also lumped together in the rL LLVM Diffusion repository, and we didn’t want to subscribe llvm-commits@ on those.

Once I land the set of commits so each project has its own Diffusion repository (and we give it a week or so so everyone is up to date), we can update llvm/.arcconfig to explicitly set revisions as belonging to the rL repository, and then set up a Herald rule to subscribe llvm-commits@ on those.

Does that help?

Ben

Forgot to mention:

Also, how would arcanist pick up two callsigns here? Wouldn’t it just pick the from the closest surrounding .arcconfig?

The review request will belong to a single repository, as you noticed (from the closest .arcconfig to where the arc command was invoked).

However, when commits land, they will be imported under either one (rL for LLVM diffs) or two (rL + rC for e.g. Clang diffs) repositories.

Ben

It seems like this has already been set up for all the projects except for libunwind. Can we create a Differential repository for it as well? I’ll be happy to update its .arcconfig once that’s done.

Additionally, it might be worth considering if this change means the
guidance should be updated in the docs - they currently recommend
leaving the repository field blank
http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Best,

Alex

@Petr:

Can we create a Differential repository for it as well? I’ll be happy to update its .arcconfig once that’s done.

Sorry for the oversight, Petr! I’ve created a repository for libunwind with the callsign rUNW:

https://reviews.llvm.org/source/libunwind/

@Alex:

Additionally, it might be worth considering if this change means the guidance should be updated in the docs - they currently recommend leaving the repository field blank

Ah, you mean when uploading a review request via the web interface? That does make sense to change. I’ll send an update.