[llvm][docs] Add notes on upstreaming code from downstream projects

[banach-space]

For context, see:

• https://discourse.llvm.org/t/code-review-process-when-upstreaming-patches

[chelini]

Thanks!

[hubert-reinterpretcast]

The straight-forward reading of this does not cover the situation where an employee develops code for an employer and the employer directs another employee to work on the upstreaming (potentially after the employee who wrote the code is not longer employed with the employer).

[hubert-reinterpretcast]

Similarly to the employer/employee scenario, a contributor to a Downstream Community project may have distanced themselves from said Downstream Community. Said Downstream Community, if their licensing is compatible, need not be beholden to the individual contributor as to whether or not the code is upstreamed.

[banach-space]

Thanks for raising this! Admittedly, I hadn’t really considered the employer-employee relationship in this context. That said, should LLVM’s Code Review policies (or any other Open Source project) mandate, guide, or influence what that relationship should be?

Let me clarify where I was coming from when writing this. My focus was on cases where some code exists in an Open Source repository, and a community volunteer decides to move that code from the downstream project to LLVM - either with or without additional changes. To me, it seems natural that the original author should be consulted, and that any changes should be properly attributed in such cases.

Regarding your point about employer-employee relationships, I’m not sure what we could add to make this clearer, as it seems highly context-dependent. And very different to what I originally had in mind. Are there any specific licensing and/or copyright issues that we should consider? I’d hope that the existing LLVM Developer Policy is sufficient here.

Ping @kbeyls , who has spent a considerable amount of time on LLVM's relicensing and is my go-to expert this kind of things.

[banach-space]

While waiting for Kristof to chime in, do you have any suggestions how to re-phrase this? Here's what I have in mind:

Ensure that there are no obstacles to upstreaming the code. In some cases, this simply means checking with the original author(s) to ensure they are aware of and approve the upstreaming.

Would that address your concerns? From my perspective, this conveys everything that I originally intended.

[hubert-reinterpretcast]

Would that address your concerns? From my perspective, this conveys everything that I originally intended.

Yes, I believe that addresses my original concerns. It leaves me wondering what obstacles there may be though. Perhaps append:

In other cases, licensing considerations may be more complex.

[banach-space]

Updated.

[hubert-reinterpretcast]

The straight-forward reading of this precludes approval of code where all of the qualified experts were involved in the development of the code. Is a code owner for a component or the lead for a specific platform (e.g., for Clang driver changes for that platform) unable to give approval to land a change if they developed part of it?

[banach-space]

Is a code owner for a component or the lead for a specific platform (e.g., for Clang driver changes for that platform) unable to give approval to land a change if they developed part of it?

Wouldn’t this essentially equate to self-approving your own code?

LLVM's developer policy already allows one to commit patches without review in certain cases:

• https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

You are allowed to commit patches without approval which you think are obvious.

When we do this, it's effectively self-approval, but the intent is clear - we typically justify direct pushes with reasons like "trivial typo" or "fix broken bot to unblock people."

I’d prefer to avoid creating a new category of "code-owner reviewing their own code submitted by another community member." That seems like an unnecessary distinction and could blur the lines of our review process.

[hubert-reinterpretcast]

It's not necessarily the same as self-approval though. For example, the person could have been involved in the downstream development, but perhaps only for a limited part of the whole patch.

If the community relies upon or generally defers to said person for the approval of patches in the area being modified, it stands to reason that, absent comments from other reviewers within a reasonable wait period, their approval should be considered valid for landing the change (if they indicate that to be their intent).

[banach-space]

For example, the person could have been involved in the downstream development, but perhaps only for a limited part of the whole patch.

Sure, but that still makes them a co-author, right? I believe we should always list all co-authors and avoid introducing overly refined rules in LLVM.

For instance, a rule like:

"Only add a person as a co-author if they contributed > N lines of code"

would be arbitrary and hard to enforce. I also wouldn’t know how to define a reasonable threshold. I'd prefer to avoid such distinctions altogether.

If the community relies upon or generally defers to said person for the approval of patches in the area being modified, it stands to reason that, absent comments from other reviewers within a reasonable wait period, their approval should be considered valid for landing the change (if they indicate that to be their intent).

Agreed—this does happen occasionally, and as you mentioned, transparency about intent is key. My suggestion primarily applies to cases where, for example, a patch has two co-authors, but no attempt is made to involve a third reviewer. In such cases, I’d expect co-authors to proactively seek additional reviewers (within reason).

By the way, if there's an area in LLVM that relies solely on one person to review all patches, that’s a separate issue worth discussing elsewhere. Even in those cases, I’d still expect a clear expression of intent from the co-author before landing a change.

[hubert-reinterpretcast]

I think we are on the same page with respect to what we believe is acceptable practice. I just think that the proposed wording is too definitive. What do you think about wording it as a recommendation instead?:

Specifically, an LGTM from a (co-)author does not constitute should not be taken as approval to land a change.

[banach-space]

Thank you for suggesting this. Let me incorporate it.

[banach-space]

I've added Kristof as a reviewer. Kristof added the existing note on using GitHub's Co-authored-by:

• llvm-project/llvm/docs/DeveloperPolicy.rst

  Line 409 in cd3acd1

  In the rare situation where there are multiple authors, please use the `git

[hubert-reinterpretcast]

FWIW, this LGTM (with minor comment).

[banach-space]

FWIW, this LGTM (with minor comment).

Updated. Could you approve it using the GitHub UI? Once that’s done, I’ll post an update on Discourse and invite more reviewers if anyone wishes to comment.

[hubert-reinterpretcast]

LGTM