ContextQMD
Back to Chromium

Chromium Review Checklist for `//third_party/rust`

third_party/rust/OWNERS-review-checklist.md

149.0.7827.25.2 KB
Original Source

Chromium Review Checklist for //third_party/rust

//third_party/rust/OWNERS review crates in this directory (e.g. when importing a new crate, or when updating an existing crate to a new version).

This document provides a checklist that

  1. helps the reviewers remember what to look for, and
  2. helps to set the expectations so that the reviews provide the right level of assurance and take the right amount of reviewer time (not too little, but also not too much).

These are very high level guidelines that explicitly avoid trying to codify what exactly to look for and how to do the reviews, because ultimately we trust the reviewer’s judgement here.

Review checklist

  • ATL approval: Is it an import of a new crate? If so, then check if 1) ATL approval has been obtained, and 2) that an OWNERS file for the new crate is present (unless it is a foundational, shared-ownership crate).

    • High-level goal: Ensure that an LGTM from //third_party/rust/OWNERS doesn’t accidentally bypass the requirement for a generic third-party review by //third_party/OWNERS.
    • If the new crate is a new transitive dependency of an already approved crate, then we should send an FYI email to ATLs (as discussed here).

  • unsafe code: Does the crate use unsafe Rust? If so, then check that the unsafe code is small, encapsulated, and documented as explained in the rule-of-2.md doc.

    • High-level goals:
      • Ensure that one of //third_party/rust/OWNERS looks at each unsafe block of code.
      • Introduce some friction for importing libraries with worrying or unnecessary use of unsafe.
    • Not a goal:
      • Leaving a written trail that for each unsafe block explains reviewer's thinking why the safety requirements are met.
      • Spending more than a minute or two per unsafe block

  • Proc macros and build.rs: Does the crate execute any code at build time? If so, then check if the build is still deterministic and hermetic.

    • High-level goal: Avoid surprises and/or identify extra complexity early. We want to preemptively review this aspect even though in theory non-hermetic or non-deterministic builds should be caught by Chromium CI at a later point, and depending on crates like cc wouldn’t work with Chromium’s build system at all.

  • Other concerns: Do you have any (generic or specific) concerns or doubts about some aspects of the crate? If so, please don’t hesitate to escalate and discuss with others, and then LGTM (or not) based on the discussion.

Other notes

  • The review can be quite minimal or lightweight if Chromium already has a trust relationship with the crate authors. For example, we already trust:

    • Rust maintainers - e.g. we trust libc, hashbrown, and similar crates authored by https://github.com/rust-lang
    • OS SDKs - e.g. we trust windows-sys, windows_aarch64_msvc and similar crates that are authored by Microsoft and expose OS APIs to Rust
    • Google or Chromium engineers - e.g. Fontations, ICU4x, etc.

  • If needed the review can be scoped down by using ban_features in gnrt_config.toml to ensure that a given crate feature is disabled.

  • There is no need to review tests, benchmarks, nor examples.

  • To quickly check if a crate uses unsafe Rust, one can look at the value of allow_unsafe in the crate's BUILD.gn file (see an example here.

  • Tools that may be helpful during a review:

    • tools/crates/grep_for_vet_relevant_keywords.sh
    • Tools for looking at a diff when updating a crate to a new version

Explicitly not part of the review checklist

  • Licensing: gnrt already checks for known/approved licenses when generating README.chromium. If an import requires updating gnrt/lib/readme.rs to account for new license kinds, then //tools/crates/gnrt/lib/readme.rs-third-party-license-review.md can say how to review the changes.