Skip to content

crate_universe: crates_vendor runs buildifier against the consuming repo's .buildifier.json, breaking vendoring #4125

Description

@alexeagle

What happens

crates_vendor formats its generated BUILD files by shelling out to buildifier in crate_universe/src/cli/vendor.rs:

fn buildifier_format(bin: &Path, content: &str, path: &Path) -> anyhow::Result<String> {
    let mut child = process::Command::new(bin)
        .args(["-lint=fix", "-mode=fix", "-warnings=all"])
        .arg(format!("--path={}", path.display()))
        ...

It passes -warnings=all, sets no BUILDIFIER_CONFIG / -config, and sets no current_dir. buildifier's config discovery walks up from the process cwd (unconditionally, regardless of stdin vs file input), so when the consuming repo has a .buildifier.json at its workspace root, buildifier auto-discovers it and applies that repo's lint configuration to rules_rust's generated files.

That breaks bazel run //<pkg>:<crates_vendor target> (i.e. repinning) in any repo that has a .buildifier.json. We hit all three of these failure modes:

  1. A repo warningsList merges with the -warnings=all flag → buildifier gets ["<repo warning>", "all"], and "all" may not appear in a list with other warnings → unexpected warning "all".
  2. If the repo config enables a warning name newer than the buildifier version crate_universe bundles (for us, allowed-symbol-load-locations), the bundled buildifier rejects the unknown name → unexpected warning "allowed-symbol-load-locations".
  3. A relative tables / addTables path in the repo config is resolved against buildifier's cwd rather than the config file's directory (older buildifier), so the referenced tables file isn't found.

Each surfaces as Failed to run buildifier on <generated file> and the vendor run exits non-zero.

Why this is wrong

Generated vendored BUILD files are not authored source — they shouldn't be subject to the consuming repo's lint policy, and the repo's config can legitimately contain things that are invalid to combine with -warnings=all (a warningsList), reference newer warning names than the bundled buildifier knows, or use paths relative to the repo root. The current code implicitly assumes the consuming repo's buildifier config is compatible with formatting rules_rust's output, which isn't a safe assumption outside a single, uniformly-configured workspace.

Note the asymmetry within the same file: bzlmod_tidy and BazelInfo::try_new both call .current_dir(workspace_dir), but buildifier_format takes no step to control where/how buildifier resolves config.

Reproduction

In any repo consuming crate_universe, add a workspace-root .buildifier.json such as:

{ "type": "auto", "lint": "warn", "warningsList": ["module-docstring"] }

then bazel run //<pkg>:<crates_vendor target>. It fails with unexpected warning "all" while formatting the first generated file. (Using a warning name newer than the bundled buildifier, or a relative addTables, reproduces modes 2 and 3.)

Suggested fix

Isolate crates_vendor's buildifier from the consuming repo's config — e.g. run it with BUILDIFIER_CONFIG pointed at a neutral/empty config (or -config set to one), so generated files are always formatted with buildifier's built-in rules, independent of the workspace .buildifier.json. This preserves the existing -warnings=all cleanup and produces no change to generated output.

Versions

Reproduced on rules_rust 0.58.0. Still present on main: buildifier_format continues to pass -warnings=all with no config isolation.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions