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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Robinson, Paul via cfe-dev
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: [hidden email] 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

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

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

Robinson, Paul via cfe-dev
On 11/20/2017 05:29 PM, Ben Hamilton via cfe-dev wrote:
> 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?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Robinson, Paul via cfe-dev
In reply to this post by Robinson, Paul via cfe-dev
[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

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

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

Robinson, Paul via cfe-dev
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Robinson, Paul via cfe-dev
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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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

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

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

Robinson, Paul via cfe-dev
+llvm-dev, so we get wider input :)

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).


On Tue, Nov 21, 2017 at 5:18 PM Ben Hamilton via cfe-dev <[hidden email]> wrote:
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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

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

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

Robinson, Paul via cfe-dev

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


On Wed, Nov 22, 2017, 09:23 Manuel Klimek <[hidden email]> wrote:
+llvm-dev, so we get wider input :)

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).


On Tue, Nov 21, 2017 at 5:18 PM Ben Hamilton via cfe-dev <[hidden email]> wrote:
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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

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

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

Robinson, Paul via cfe-dev
On Thu, Nov 23, 2017 at 12:58 AM Ben Hamilton <[hidden email]> wrote:

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?
 

On Wed, Nov 22, 2017, 09:23 Manuel Klimek <[hidden email]> wrote:
+llvm-dev, so we get wider input :)

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).


On Tue, Nov 21, 2017 at 5:18 PM Ben Hamilton via cfe-dev <[hidden email]> wrote:
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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

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

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

Robinson, Paul via cfe-dev

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


On Thu, Nov 23, 2017, 02:43 Manuel Klimek <[hidden email]> wrote:
On Thu, Nov 23, 2017 at 12:58 AM Ben Hamilton <[hidden email]> wrote:

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?
 

On Wed, Nov 22, 2017, 09:23 Manuel Klimek <[hidden email]> wrote:
+llvm-dev, so we get wider input :)

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).


On Tue, Nov 21, 2017 at 5:18 PM Ben Hamilton via cfe-dev <[hidden email]> wrote:
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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

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

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

Robinson, Paul via cfe-dev
On Sun, Nov 26, 2017 at 7:19 PM Ben Hamilton <[hidden email]> wrote:

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


Sounds good!
 

On Thu, Nov 23, 2017, 02:43 Manuel Klimek <[hidden email]> wrote:
On Thu, Nov 23, 2017 at 12:58 AM Ben Hamilton <[hidden email]> wrote:

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?
 

On Wed, Nov 22, 2017, 09:23 Manuel Klimek <[hidden email]> wrote:
+llvm-dev, so we get wider input :)

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).


On Tue, Nov 21, 2017 at 5:18 PM Ben Hamilton via cfe-dev <[hidden email]> wrote:
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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

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

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

Robinson, Paul via cfe-dev
In reply to this post by Robinson, Paul via cfe-dev
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 <[hidden email]>:
OK. I confirmed that Stephan's process to send out cross-repo diffs from the monorepo is not affected by my proposal.

I'm a bit late to the party, and I might just not have comprehended this correctly. But does your example actually work? I didn't see any email about D40312 on llvm-commits, shouldn't there have been one? Also, how would arcanist pick up two callsigns here? Wouldn't it just pick the from the closest surrounding .arcconfig?
 


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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



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

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

Robinson, Paul via cfe-dev
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

On Mon, Nov 27, 2017 at 9:42 AM Philip Pfaffe <[hidden email]> wrote:
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 <[hidden email]>:
OK. I confirmed that Stephan's process to send out cross-repo diffs from the monorepo is not affected by my proposal.

I'm a bit late to the party, and I might just not have comprehended this correctly. But does your example actually work? I didn't see any email about D40312 on llvm-commits, shouldn't there have been one? Also, how would arcanist pick up two callsigns here? Wouldn't it just pick the from the closest surrounding .arcconfig?
 


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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


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

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

Robinson, Paul via cfe-dev
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

On Mon, Nov 27, 2017 at 10:00 AM Ben Hamilton <[hidden email]> wrote:
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

On Mon, Nov 27, 2017 at 9:42 AM Philip Pfaffe <[hidden email]> wrote:
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 <[hidden email]>:
OK. I confirmed that Stephan's process to send out cross-repo diffs from the monorepo is not affected by my proposal.

I'm a bit late to the party, and I might just not have comprehended this correctly. But does your example actually work? I didn't see any email about D40312 on llvm-commits, shouldn't there have been one? Also, how would arcanist pick up two callsigns here? Wouldn't it just pick the from the closest surrounding .arcconfig?
 


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):

===
% git clone https://github.com/llvm-project/llvm-project-20170507/ llvm-project
% cd llvm-project
% cp llvm/.arcconfig .
% mkdir -p .git/info
% echo .arcconfig >> .git/info/exclude
# Manually apply D40179 into the clang-tools-extra directory
% arc export --revision D40179 --git | patch -d clang-tools-extra -p1
patching file .arcconfig
% git diff
diff --git a/clang-tools-extra/.arcconfig b/clang-tools-extra/.arcconfig
index f846581763e..d4a00161bce 100644
--- a/clang-tools-extra/.arcconfig
+++ b/clang-tools-extra/.arcconfig
@@ -1,4 +1,4 @@
 {
-  "project_id" : "clang-tools-extra",
+  "repository.callsign" : "CTE",
   "conduit_uri" : "https://reviews.llvm.org/"
 }
 % git commit -a -m "Differential Revision: https://reviews.llvm.org/D40179"
 % echo "Test 1" >> llvm/README.txt                                                          
 % echo "Test 2" >> clang-tools-extra/README.txt                                             
 % git commit -a -m "[Test] Monorepo test diff crossing Differential repositories"                                                              
 % arc diff --create --reviewers sberg HEAD^
===

Ben

On Tue, Nov 21, 2017 at 8:00 AM Stephan Bergmann <[hidden email]> wrote:
On 11/21/2017 03:41 PM, Ben Hamilton wrote:
>  > 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>?

I'm using <https://github.com/llvm-project/llvm-project-20170507> (as
advertised at
<https://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo>),
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> 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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev