mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
build: migrate argument-comment-lint to a native Bazel aspect (#16106)
## Why `argument-comment-lint` had become a PR bottleneck because the repo-wide lane was still effectively running a `cargo dylint`-style flow across the workspace instead of reusing Bazel's Rust dependency graph. That kept the lint enforced, but it threw away the main benefit of moving this job under Bazel in the first place: metadata reuse and cacheable per-target analysis in the same shape as Clippy. This change moves the repo-wide lint onto a native Bazel Rust aspect so Linux and macOS can lint `codex-rs` without rebuilding the world crate-by-crate through the wrapper path. ## What Changed - add a nightly Rust toolchain with `rustc-dev` for Bazel and a dedicated crate-universe repo for `tools/argument-comment-lint` - add `tools/argument-comment-lint/driver.rs` and `tools/argument-comment-lint/lint_aspect.bzl` so Bazel can run the lint as a custom `rustc_driver` - switch repo-wide `just argument-comment-lint` and the Linux/macOS `rust-ci` lanes to `bazel build --config=argument-comment-lint //codex-rs/...` - keep the Python/DotSlash wrappers as the package-scoped fallback path and as the current Windows CI path - gate the Dylint entrypoint behind a `bazel_native` feature so the Bazel-native library avoids the `dylint_*` packaging stack - update the aspect runtime environment so the driver can locate `rustc_driver` correctly under remote execution - keep the dedicated `tools/argument-comment-lint` package tests and wrapper unit tests in CI so the source and packaged entrypoints remain covered ## Verification - `python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'` - `cargo test` in `tools/argument-comment-lint` - `bazel build //tools/argument-comment-lint:argument-comment-lint-driver --@rules_rust//rust/toolchain/channel=nightly` - `bazel build --config=argument-comment-lint //codex-rs/utils/path-utils:all` - `bazel build --config=argument-comment-lint //codex-rs/rollout:rollout` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/16106). * #16120 * __->__ #16106
This commit is contained in:
Michael Bolin
committed by
GitHub
Unverified
GitHub
parent
65f631c3d6
commit
fce0f76d57
@@ -0,0 +1,32 @@
|
||||
load("@rules_rust//rust:defs.bzl", "rust_binary", "rust_library")
|
||||
|
||||
exports_files(["lint_aspect.bzl"])
|
||||
|
||||
rust_library(
|
||||
name = "argument-comment-lint-lib",
|
||||
crate_name = "argument_comment_lint",
|
||||
crate_features = ["bazel_native"],
|
||||
crate_root = "src/lib.rs",
|
||||
srcs = [
|
||||
"src/comment_parser.rs",
|
||||
"src/lib.rs",
|
||||
],
|
||||
edition = "2024",
|
||||
deps = ["@argument_comment_lint_crates//:clippy_utils"],
|
||||
tags = ["manual"],
|
||||
visibility = ["//visibility:public"],
|
||||
)
|
||||
|
||||
rust_binary(
|
||||
name = "argument-comment-lint-driver",
|
||||
crate_name = "argument_comment_lint_driver",
|
||||
crate_root = "driver.rs",
|
||||
srcs = ["driver.rs"],
|
||||
edition = "2024",
|
||||
deps = [
|
||||
":argument-comment-lint-lib",
|
||||
"@zlib//:z",
|
||||
],
|
||||
tags = ["manual"],
|
||||
visibility = ["//visibility:public"],
|
||||
)
|
||||
@@ -8,6 +8,9 @@ publish = false
|
||||
[lib]
|
||||
crate-type = ["cdylib"]
|
||||
|
||||
[features]
|
||||
bazel_native = []
|
||||
|
||||
[dependencies]
|
||||
clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "20ce69b9a63bcd2756cd906fe0964d1e901e042a" }
|
||||
dylint_linting = "5.0.0"
|
||||
|
||||
@@ -85,9 +85,11 @@ rustup toolchain install nightly-2025-09-18 \
|
||||
|
||||
The checked-in DotSlash file lives at `tools/argument-comment-lint/argument-comment-lint`.
|
||||
`run-prebuilt-linter.py` resolves that file via `dotslash` and is the path used by
|
||||
`just clippy`, `just argument-comment-lint`, and the Rust CI job. The
|
||||
source-build path remains available in `run.py` for people
|
||||
iterating on the lint crate itself.
|
||||
targeted package runs such as `just argument-comment-lint -p codex-core`.
|
||||
Repo-wide runs now go through a native Bazel aspect that invokes a custom
|
||||
`rustc_driver` and reuses Bazel-managed Rust dependency metadata instead of
|
||||
spawning `cargo dylint` once per crate. The source-build path remains available
|
||||
in `run.py` for people iterating on the lint crate itself.
|
||||
|
||||
The Unix archive layout is:
|
||||
|
||||
@@ -126,15 +128,17 @@ If you are changing the lint crate itself, use the source-build wrapper:
|
||||
Run the lint against `codex-rs` from the repo root:
|
||||
|
||||
```bash
|
||||
just argument-comment-lint
|
||||
bazel build --config=argument-comment-lint -- //codex-rs/...
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core
|
||||
just argument-comment-lint -p codex-core
|
||||
```
|
||||
|
||||
If no package selection is provided, `run-prebuilt-linter.py` defaults to checking the
|
||||
`codex-rs` workspace with `--workspace --no-deps`.
|
||||
For non-`--fix` runs, both wrappers also default the underlying Cargo
|
||||
invocation to `--all-targets` unless you explicitly narrow the target set, so
|
||||
workspace and package lint runs both cover test-only call sites by default.
|
||||
If no package selection is provided, `just argument-comment-lint` now defaults
|
||||
to the Bazel aspect path over `//codex-rs/...`. The Python wrappers remain the
|
||||
package-scoped escape hatch and still default the underlying Cargo invocation
|
||||
to `--all-targets` unless you explicitly narrow the target set, so targeted
|
||||
wrapper runs cover test-only call sites by default.
|
||||
|
||||
Repo runs also promote `uncommented_anonymous_literal_argument` to an error by
|
||||
default:
|
||||
|
||||
@@ -0,0 +1,43 @@
|
||||
#![feature(rustc_private)]
|
||||
|
||||
extern crate rustc_driver;
|
||||
extern crate rustc_interface;
|
||||
|
||||
use std::env;
|
||||
use std::ffi::OsString;
|
||||
use std::path::Path;
|
||||
|
||||
fn main() {
|
||||
let mut callbacks = Callbacks;
|
||||
let args = rustc_args(env::args_os().skip(1).collect());
|
||||
rustc_driver::run_compiler(&args, &mut callbacks);
|
||||
}
|
||||
|
||||
struct Callbacks;
|
||||
|
||||
impl rustc_driver::Callbacks for Callbacks {
|
||||
fn config(&mut self, config: &mut rustc_interface::Config) {
|
||||
let previous = config.register_lints.take();
|
||||
config.register_lints = Some(Box::new(move |sess, lint_store| {
|
||||
if let Some(previous) = &previous {
|
||||
previous(sess, lint_store);
|
||||
}
|
||||
argument_comment_lint::register_lints(sess, lint_store);
|
||||
}));
|
||||
}
|
||||
}
|
||||
|
||||
fn rustc_args(args: Vec<OsString>) -> Vec<String> {
|
||||
let mut rustc_args: Vec<String> = args
|
||||
.into_iter()
|
||||
.map(|arg| arg.to_string_lossy().into_owned())
|
||||
.collect();
|
||||
if rustc_args.first().is_none_or(|arg| !is_rustc(arg)) {
|
||||
rustc_args.insert(0, "rustc".to_string());
|
||||
}
|
||||
rustc_args
|
||||
}
|
||||
|
||||
fn is_rustc(arg: &str) -> bool {
|
||||
Path::new(arg).file_stem().and_then(|stem| stem.to_str()) == Some("rustc")
|
||||
}
|
||||
@@ -0,0 +1,187 @@
|
||||
"""Bazel aspect for running argument-comment-lint on Rust targets."""
|
||||
|
||||
load("@rules_rust//rust:defs.bzl", "rust_common")
|
||||
load("@rules_rust//rust/private:rust.bzl", "RUSTC_ATTRS")
|
||||
load(
|
||||
"@rules_rust//rust/private:rustc.bzl",
|
||||
"collect_deps",
|
||||
"collect_inputs",
|
||||
"construct_arguments",
|
||||
)
|
||||
load(
|
||||
"@rules_rust//rust/private:utils.bzl",
|
||||
"determine_output_hash",
|
||||
"find_cc_toolchain",
|
||||
"find_toolchain",
|
||||
)
|
||||
|
||||
_STRICT_LINT_FLAGS = [
|
||||
"-Duncommented-anonymous-literal-argument",
|
||||
"-Aunknown-lints",
|
||||
]
|
||||
|
||||
def _find_rustc_driver_library(toolchain):
|
||||
for file in toolchain.rustc_lib.to_list():
|
||||
if file.basename.startswith("librustc_driver-") or file.basename.startswith("rustc_driver-"):
|
||||
return file
|
||||
return None
|
||||
|
||||
def _prepend_runtime_path(env, key, path, separator):
|
||||
previous = env.get(key)
|
||||
env[key] = "{}{}{}".format(path, separator, previous) if previous else path
|
||||
|
||||
def _set_driver_runtime_env(env, toolchain):
|
||||
driver_library = _find_rustc_driver_library(toolchain)
|
||||
if not driver_library:
|
||||
return
|
||||
|
||||
library_dir = driver_library.dirname
|
||||
if driver_library.basename.endswith(".dll"):
|
||||
_prepend_runtime_path(env, "PATH", library_dir, ";")
|
||||
return
|
||||
|
||||
# The lint driver runs in exec configuration. Under remote execution the
|
||||
# exec OS can differ from the Rust target OS, so populate both Unix loader
|
||||
# variables from the located driver library instead of keying off target_os.
|
||||
_prepend_runtime_path(env, "LD_LIBRARY_PATH", library_dir, ":")
|
||||
_prepend_runtime_path(env, "DYLD_LIBRARY_PATH", library_dir, ":")
|
||||
|
||||
def _get_argument_comment_lint_ready_crate_info(target, aspect_ctx):
|
||||
if target.label.workspace_root.startswith("external"):
|
||||
return None
|
||||
|
||||
if aspect_ctx:
|
||||
ignore_tags = [
|
||||
"no_argument_comment_lint",
|
||||
"no-lint",
|
||||
"no_lint",
|
||||
"nolint",
|
||||
]
|
||||
for tag in aspect_ctx.rule.attr.tags:
|
||||
if tag.replace("-", "_").lower() in ignore_tags:
|
||||
return None
|
||||
|
||||
if rust_common.crate_info in target:
|
||||
return target[rust_common.crate_info]
|
||||
if rust_common.test_crate_info in target:
|
||||
return target[rust_common.test_crate_info].crate
|
||||
return None
|
||||
|
||||
def _rust_argument_comment_lint_aspect_impl(target, ctx):
|
||||
if OutputGroupInfo in target and hasattr(target[OutputGroupInfo], "argument_comment_lint_checks"):
|
||||
return []
|
||||
|
||||
crate_info = _get_argument_comment_lint_ready_crate_info(target, ctx)
|
||||
if not crate_info:
|
||||
return []
|
||||
|
||||
toolchain = find_toolchain(ctx)
|
||||
cc_toolchain, feature_configuration = find_cc_toolchain(ctx)
|
||||
|
||||
dep_info, build_info, _ = collect_deps(
|
||||
deps = crate_info.deps.to_list(),
|
||||
proc_macro_deps = crate_info.proc_macro_deps.to_list(),
|
||||
aliases = crate_info.aliases,
|
||||
)
|
||||
|
||||
compile_inputs, out_dir, build_env_files, build_flags_files, linkstamp_outs, ambiguous_libs = collect_inputs(
|
||||
ctx,
|
||||
ctx.rule.file,
|
||||
ctx.rule.files,
|
||||
depset([]),
|
||||
toolchain,
|
||||
cc_toolchain,
|
||||
feature_configuration,
|
||||
crate_info,
|
||||
dep_info,
|
||||
build_info,
|
||||
[],
|
||||
)
|
||||
|
||||
success_marker = ctx.actions.declare_file(
|
||||
ctx.label.name + ".argument_comment_lint.ok",
|
||||
sibling = crate_info.output,
|
||||
)
|
||||
|
||||
args, env = construct_arguments(
|
||||
ctx = ctx,
|
||||
attr = ctx.rule.attr,
|
||||
file = ctx.file,
|
||||
toolchain = toolchain,
|
||||
tool_path = ctx.executable._driver.path,
|
||||
cc_toolchain = cc_toolchain,
|
||||
feature_configuration = feature_configuration,
|
||||
crate_info = crate_info,
|
||||
dep_info = dep_info,
|
||||
linkstamp_outs = linkstamp_outs,
|
||||
ambiguous_libs = ambiguous_libs,
|
||||
output_hash = determine_output_hash(crate_info.root, ctx.label),
|
||||
rust_flags = [],
|
||||
out_dir = out_dir,
|
||||
build_env_files = build_env_files,
|
||||
build_flags_files = build_flags_files,
|
||||
emit = ["dep-info", "metadata"],
|
||||
skip_expanding_rustc_env = True,
|
||||
)
|
||||
|
||||
if crate_info.is_test:
|
||||
args.rustc_flags.add("--test")
|
||||
|
||||
args.process_wrapper_flags.add("--touch-file", success_marker)
|
||||
args.rustc_flags.add_all(_STRICT_LINT_FLAGS)
|
||||
_set_driver_runtime_env(env, toolchain)
|
||||
|
||||
driver_runfiles = ctx.attr._driver[DefaultInfo].default_runfiles.files
|
||||
action_inputs = depset(
|
||||
transitive = [
|
||||
compile_inputs,
|
||||
driver_runfiles,
|
||||
toolchain.rustc_lib,
|
||||
],
|
||||
)
|
||||
|
||||
ctx.actions.run(
|
||||
executable = ctx.executable._process_wrapper,
|
||||
inputs = action_inputs,
|
||||
outputs = [success_marker],
|
||||
env = env,
|
||||
tools = [ctx.executable._driver],
|
||||
execution_requirements = {
|
||||
"no-sandbox": "1",
|
||||
},
|
||||
arguments = args.all,
|
||||
mnemonic = "ArgumentCommentLint",
|
||||
progress_message = "ArgumentCommentLint %{label}",
|
||||
toolchain = "@rules_rust//rust:toolchain_type",
|
||||
)
|
||||
|
||||
return [OutputGroupInfo(argument_comment_lint_checks = depset([success_marker]))]
|
||||
|
||||
rust_argument_comment_lint_aspect = aspect(
|
||||
implementation = _rust_argument_comment_lint_aspect_impl,
|
||||
fragments = ["cpp"],
|
||||
attrs = {
|
||||
"_driver": attr.label(
|
||||
default = Label("//tools/argument-comment-lint:argument-comment-lint-driver"),
|
||||
executable = True,
|
||||
cfg = "exec",
|
||||
),
|
||||
} | RUSTC_ATTRS,
|
||||
toolchains = [
|
||||
str(Label("@rules_rust//rust:toolchain_type")),
|
||||
config_common.toolchain_type("@bazel_tools//tools/cpp:toolchain_type", mandatory = False),
|
||||
],
|
||||
required_providers = [
|
||||
[rust_common.crate_info],
|
||||
[rust_common.test_crate_info],
|
||||
],
|
||||
doc = """\
|
||||
Runs argument-comment-lint on Rust targets using Bazel's Rust dependency graph.
|
||||
|
||||
Example:
|
||||
|
||||
```output
|
||||
$ bazel build --config=argument-comment-lint //codex-rs/...
|
||||
```
|
||||
""",
|
||||
)
|
||||
@@ -31,6 +31,7 @@ use rustc_span::Span;
|
||||
use crate::comment_parser::parse_argument_comment;
|
||||
use crate::comment_parser::parse_argument_comment_prefix;
|
||||
|
||||
#[cfg(not(feature = "bazel_native"))]
|
||||
dylint_linting::dylint_library!();
|
||||
|
||||
#[unsafe(no_mangle)]
|
||||
|
||||
@@ -83,6 +83,26 @@ class WrapperCommonTest(unittest.TestCase):
|
||||
],
|
||||
)
|
||||
|
||||
def test_explicit_package_manifest_does_not_force_workspace(self) -> None:
|
||||
parsed = wrapper_common.parse_wrapper_args(
|
||||
[
|
||||
"--manifest-path",
|
||||
"/tmp/custom/Cargo.toml",
|
||||
]
|
||||
)
|
||||
final_args = wrapper_common.build_final_args(parsed, Path("/repo/codex-rs/Cargo.toml"))
|
||||
|
||||
self.assertEqual(
|
||||
final_args,
|
||||
[
|
||||
"--no-deps",
|
||||
"--manifest-path",
|
||||
"/tmp/custom/Cargo.toml",
|
||||
"--",
|
||||
"--all-targets",
|
||||
],
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
@@ -107,7 +107,7 @@ def build_final_args(parsed: ParsedWrapperArgs, manifest_path: Path) -> list[str
|
||||
|
||||
if not parsed.has_manifest_path:
|
||||
final_args.extend(["--manifest-path", str(manifest_path)])
|
||||
if not parsed.has_package_selection:
|
||||
if not parsed.has_package_selection and not parsed.has_manifest_path:
|
||||
final_args.append("--workspace")
|
||||
if not parsed.has_no_deps:
|
||||
final_args.append("--no-deps")
|
||||
@@ -205,6 +205,9 @@ def ensure_source_prerequisites(env: MutableMapping[str, str]) -> None:
|
||||
|
||||
|
||||
def prefer_rustup_shims(env: MutableMapping[str, str]) -> None:
|
||||
if env.get("CODEX_ARGUMENT_COMMENT_LINT_SKIP_RUSTUP_SHIMS") == "1":
|
||||
return
|
||||
|
||||
rustup = shutil.which("rustup", path=env.get("PATH"))
|
||||
if rustup is None:
|
||||
return
|
||||
|
||||
Reference in New Issue
Block a user