opensource.google.com

Menu

Docs

Third-Party Reviewers

go/thirdpartyreviewers

This guide provides instructions for reviewers of third-party CLs including what to look for during reviews and how to escalate issues. If you’re just looking for instructions on submitting third-party code to google3, see go/thirdparty.

What’s involved and why would I want to help?

Reviewing additions to Google’s large cache of third-party code is rewarding and fun. It helps Google ensure we are staying in compliance with and being respectful of other authors’ licenses, and it also helps find security problems with outside code faster. It helps your fellow Googlers save time by using third-party code where it’s appropriate.

Depending on which reviewer groups you’re on (there are several, see http://linkremoved/), you may receive about a dozen or so CLs a week assigned to you for review. In the common case, they take only a few minutes to review. Some esoteric packages may require more review and/or escalation for more thorough legal review.

How reviewers are assigned

Third-party reviewers handle reviewing changes made in piper (most notably in //piper/third_party, but also a few other locations) as well as in various Gerrit-on-Borg hosts (go/gob).

A presubmit defined in //piper/third_party/METADATA ensures that any CLs that touch one of a few files containing package metadata automatically CC emailremoved@. A similar mechanism in Gerrit adds a special “Third-Party” label to any changes that touch third-party packages. Periodically (currently every 5 minutes), a job runs that assigns a reviewer to these CLs, pulling from the membership of http://linkremoved/ in a round robin fashion.

This mechanism is intentionally aggressive about assigning a reviewer to any changes that might have license compliance implications. As a result, a reviewer may be assigned to a change that doesn’t in fact have any compliance implications (such as simple package upgrades or deletions), in which case the reviewer leaves the CL to be reviewed by the “custodial owner”.

Getting started

If you’d like to help with third-party reviews, email emailremoved@ and let them know. If you’ve already done that and have been instructed to move forward, then follow these steps to set things up.

  1. Familiarize yourself with the relevant documents. In addition to this page, also read over:

    • The guidance for all third-party submitters at go/thirdparty
    • The Googley taxonomy of common open source licenses at go/thirdpartylicenses
    • Notes on vendor_src and google_vendor_src_branch at go/vendorsrc
  2. Submit a proposal to be added to http://linkremoved/ (or other review group if applicable). Once added, you should start having CLs assigned to you within a day or so. You can view a dashboard of your current assignments and manage inactivity periods at http://linkremoved/.

Your first few reviews

For your first few reviews, use Critique to add emailremoved@ to the R= line as soon as the review is assigned to you. He will help check your work. You can and should still go through the process as described below; just wait for emailremoved@’s LGTM before approving.

Doing reviews

We review changes submitted through critique as well as Gerrit-on-Borg (go/gob). Most of the things we check for are the same, though there are a few procedural differences.

Many of these checks are performed by the compliance linter, which runs on every //third_party CL, but we rely on the third-party reviewer to make a second pass and ensure that any identified problems are corrected. You can normally just click the “please fix” link on the suggestions that are left directly inline in Critique like any other linter warning.

Is this review really for you?

Sometimes you will be assigned a review for a CL that doesn’t involve any new third-party code, but is rather a local modification to, or removal of, an existing package. In this case, what you do will depend on where the code is being committed:

  • If the code is going into google3, you should not give an LGTM or approve the CL in this case. Doing so could allow the CL author to submit the code. You should allow the code’s real owners to do a review.
  • If the code is going into a Gerrit host, you should give a +1 for Third-Party. Gerrit supports multiple approval dimensions, and unless the host is configured otherwise, a +1 on Third-Party is required but not sufficient to allow submission.

However, you should at least take a very brief look to make sure that METADATA looks about how go/thirdparty says it should (of course, this doesn’t apply if the entire package is being deleted). If it looks very strange, you may be looking at a package so old that the rules for third-party code have changed since its addition. In those cases, you should tell the submitter to look at go/thirdparty and bring the package’s metadata up to date. Otherwise, just leave a comment that says “no license compliance implications; custodial owners should review and approve” or something like that, and let the package’s local owners do the review.

Another time you might need to do the review yourself is if the package’s real owners have all left the company. In this case, you will have to do a traditional Google code review, and can pretty much disregard the rest of this page. If this happens, you should insist that the submitter find two real OWNERS for the package—one good suggestion is the submitter themselves, of course!

LICENSE file

There should be a file called LICENSE containing a copyright statement and license. This file must be named exactly LICENSE, not LICENSE.txt, COPYING, LICENCE, or anything like that (these alternatives may be present in addition if they were part of the upstream distribution).

The vast majority of licenses are one of a few very common types (BSD, MIT, Apache, or GPL), and you will learn to recognize these licenses quickly. In other cases, you should always check go/thirdpartylicenses to see what type of license it is. If you don’t see the exact license listed there, do not attempt to figure it out yourself. Add emailremoved@ as a reviewer on the CL to have a lawyer review it.

WARNING: Code released under the GNU AGPL, which is different than the normal GPL, is not allowed to be used in Google products, ever. If you get a review for a CL adding a package that’s AGPL licensed (unless it is dual-licensed under less-restrictive terms, which is highly unlikely due to the AGPL’s nature and purpose), do not approve or LGTM it. Instruct the submitter to drop the CL and delete the code from their machine (see more at go/agpl).

METADATA file

Things to check for in the METADATA file:

  • A name field naming the code.
  • A description field, giving a short description of the code.
  • A third_party structure field, with the appropriate sections in it.
    • A url field. See go/thirdparty/metadata#url for examples of uses of this field.
    • A version field, if the url type isn’t PIPER’.
    • A last_upgrade_date field, if the url type isn’t PIPER.

BUILD file in Piper

NOTE: The following guidelines apply only to third-party code being checked into Piper. Third party libraries in Gerrit repositories do not need a BUILD file.

A BUILD must be present, even if the package doesn’t build with Blaze, and must include, at a minimum, exports_files('LICENSE') and licenses directives. The licenses directive should specify the type of license listed in go/thirdpartylicenses, with a comment indicating the specific license. For example,

# Foobar, a library for frobbing whoosies.
licenses(['restricted'])  # GPLv2
exports_files(['LICENSE'])

OWNERS file in Piper

NOTE: The following guidelines apply only to third-party code being checked into Piper; Third-party libraries in Gerrit repositories do not need an OWNERS file.

The OWNERS file must list at least two full-time Googlers explicitly as the first two owners. That is, they need to be real usernames as opposed to file: or group: directives. When you view the CL in Critique, the usernames will be links to http://linkremoved/ You should verify that the first two people are full-timers, not interns (especially relevant during intern season). File and group directives are permitted but the explicit people need to come first in the file.

The compliance linter will automatically detect if there are not two full-time Googlers listed as the first two owners in the OWNERS file. If the linter produces a warning and the author of the CL has not addressed it, please point them to go/thirdparty/where.

WARNING: Under no circumstances may an OWNERS file under //third_party include the line set noparent.

See if the code needs a language reviewer

An http://linkremoved/ review checks for license compliance issues, but for many projects there are additional guidelines to follow related to having code in google3. There are special pools of volunteer reviewers for (most of) these, so that each third-party reviewer doesn’t have to learn the rules for every language or platform Google uses. When there is such a language review needed, the rule is that both the auto-assigned //third_party owner and the language reviewer must look over the CL and give an LGTM. Whoever gives the second LGTM should give the required g4 approval. This requires slightly more care on your part but is necessary because there is no way to require multiple OWNERS to approve something in g4.

  • Languages that have their own reviewer group should be automatically added to relevant CLs. You can see the full list of language reviewer groups at http://linkremoved/ (look for groups named *removed*). If it looks like a group didn’t get added to a CL when it should have been, you can manually add them as a reviewer. Reviewers from these groups will be automatically assigned in the same way as ****************** reviewers, and will be identified in the CL comment left by change2review.
  • If both a ****************** reviewer and a language reviewer has been assigned, the first reviewer should only LGTM, but not approve the CL. Typically, reviewers leave a comment saying “LGTM for compliance, ********************* will approve”, or something similar. The second reviewer should both LGTM and Approve once everything looks good.
  • Some languages, such as C/C++ and Emacs Lisp, have some language-specific guidance, but not dedicated language reviewer group. See the language-specific pages in the sidebar of go/thirdparty to see what you should check for.
  • If the code is written in Javascript, there are some guidelines that are license compliance issues that need to be checked by //third_party/OWNERS related to the @license tag being present in JSDoc /** style comments that contain license or copyright notices.

Multiple versions of the same package

NOTE: Packages that have migrated to the new METADATA format will not have a MULTIPLE_VERSIONS_* file. Instead, this data is captured directly in the METADATA file.

Multiple simultaneous versions are highly discouraged (see go/oneversion). Any exceptions to this rule always need to be documented with either a MULTIPLE_VERSIONS_TEMPORARY (including a transition plan with timetables and dates) or MULTIPLE_VERSIONS_OK (with justification accepted by the reviewers). These files must also include a link to the emailremoved@ thread where this multiple version exception was discussed and approved by the third-party team.

Except as otherwise noted, the content of this page is licensed under CC-BY-4.0 license. Third-party product names and logos may be the trademarks of their respective owners.