From acd10385b4c954c9681147b7e43901110b3b8dff Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 28 Mar 2026 00:51:55 -0700 Subject: [PATCH] ci: run argument-comment-lint through bazel package tests --- .github/actions/setup-bazel-ci/action.yml | 8 +- .github/workflows/bazel.yml | 2 + .github/workflows/rust-ci.yml | 65 +++++--- codex-rs/BUILD.bazel | 14 ++ defs.bzl | 84 ++++++++++ justfile | 17 +- tools/argument-comment-lint/BUILD.bazel | 19 +++ tools/argument-comment-lint/README.md | 5 +- tools/argument-comment-lint/bazel_runner.rs | 148 ++++++++++++++++++ .../test_wrapper_common.py | 20 +++ tools/argument-comment-lint/wrapper_common.py | 5 +- 11 files changed, 361 insertions(+), 26 deletions(-) create mode 100644 tools/argument-comment-lint/BUILD.bazel create mode 100644 tools/argument-comment-lint/bazel_runner.rs diff --git a/.github/actions/setup-bazel-ci/action.yml b/.github/actions/setup-bazel-ci/action.yml index 0775f928a5..34bbd40b19 100644 --- a/.github/actions/setup-bazel-ci/action.yml +++ b/.github/actions/setup-bazel-ci/action.yml @@ -57,9 +57,11 @@ runs: if: runner.os == 'Windows' shell: pwsh run: | - # Use a very short path to reduce argv/path length issues, but avoid the - # drive root because some Windows test launchers mis-handle MANIFEST paths there. - "BAZEL_OUTPUT_USER_ROOT=D:\b" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append + # Use the shortest available drive to reduce argv/path length issues, + # but avoid the drive root because some Windows test launchers mis-handle + # MANIFEST paths there. + $bazelOutputUserRoot = if (Test-Path 'D:\') { 'D:\b' } else { 'C:\b' } + "BAZEL_OUTPUT_USER_ROOT=$bazelOutputUserRoot" | Out-File -FilePath $env:GITHUB_ENV -Encoding utf8 -Append - name: Enable Git long paths (Windows) if: runner.os == 'Windows' diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index 2636213af9..a6c9277b61 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -81,6 +81,7 @@ jobs: --use-node-test-env \ -- \ test \ + --test_tag_filters=-argument-comment-lint \ --test_verbose_timeout_warnings \ --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ -- \ @@ -98,6 +99,7 @@ jobs: key: bazel-cache-${{ matrix.target }}-${{ hashFiles('MODULE.bazel', 'codex-rs/Cargo.lock', 'codex-rs/Cargo.toml') }} clippy: + timeout-minutes: 30 strategy: fail-fast: false matrix: diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index f993bccbe7..424fb82187 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -93,9 +93,16 @@ jobs: steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 - uses: dtolnay/rust-toolchain@a0b273b48ed29de4470960879e8381ff45632f26 # 1.93.0 - with: - toolchain: nightly-2025-09-18 - components: llvm-tools-preview, rustc-dev, rust-src + - name: Install nightly argument-comment-lint toolchain + shell: bash + run: | + rustup toolchain install nightly-2025-09-18 \ + --profile minimal \ + --component llvm-tools-preview \ + --component rustc-dev \ + --component rust-src \ + --no-self-update + rustup default nightly-2025-09-18 - name: Cache cargo-dylint tooling id: cargo_dylint_cache uses: actions/cache@668228422ae6a00e4ad889ee87cd7109ec5666a7 # v5 @@ -121,6 +128,7 @@ jobs: argument_comment_lint_prebuilt: name: Argument comment lint - ${{ matrix.name }} runs-on: ${{ matrix.runs_on || matrix.runner }} + timeout-minutes: ${{ matrix.timeout_minutes }} needs: changed if: ${{ needs.changed.outputs.argument_comment_lint == 'true' || needs.changed.outputs.workflows == 'true' }} strategy: @@ -129,15 +137,22 @@ jobs: include: - name: Linux runner: ubuntu-24.04 + timeout_minutes: 120 - name: macOS runner: macos-15-xlarge + timeout_minutes: 60 - name: Windows runner: windows-x64 + timeout_minutes: 120 runs_on: group: codex-runners labels: codex-windows-x64 steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + - uses: ./.github/actions/setup-bazel-ci + with: + target: ${{ runner.os }} + install-test-prereqs: true - name: Install Linux sandbox build dependencies if: ${{ runner.os == 'Linux' }} shell: bash @@ -145,24 +160,36 @@ jobs: sudo DEBIAN_FRONTEND=noninteractive apt-get update sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends pkg-config libcap-dev - uses: dtolnay/rust-toolchain@a0b273b48ed29de4470960879e8381ff45632f26 # 1.93.0 - with: - toolchain: nightly-2025-09-18 - components: llvm-tools-preview, rustc-dev, rust-src - - uses: facebook/install-dotslash@1e4e7b3e07eaca387acb98f1d4720e0bee8dbb6a # v2 - - name: Run argument comment lint on codex-rs - if: ${{ runner.os == 'macOS' }} + - name: Install nightly argument-comment-lint toolchain shell: bash - run: python3 ./tools/argument-comment-lint/run-prebuilt-linter.py - # Linux still uses the default-targets-only form for now, but PRs run the - # released linter on all three platforms so wrapper regressions surface pre-merge. - - name: Run argument comment lint on codex-rs (default targets only) - if: ${{ runner.os == 'Linux' }} + run: | + rustup toolchain install nightly-2025-09-18 \ + --profile minimal \ + --component llvm-tools-preview \ + --component rustc-dev \ + --component rust-src \ + --no-self-update + rustup default nightly-2025-09-18 + - name: Run argument comment lint on codex-rs via Bazel shell: bash - run: python3 ./tools/argument-comment-lint/run-prebuilt-linter.py -- --lib --bins - - name: Run argument comment lint on codex-rs - if: ${{ runner.os == 'Windows' }} - shell: bash - run: python ./tools/argument-comment-lint/run-prebuilt-linter.py + run: | + ./.github/scripts/run-bazel-ci.sh \ + --print-failed-test-logs \ + -- \ + test \ + --build_tests_only \ + --local_test_jobs=4 \ + --test_tag_filters=argument-comment-lint \ + --test_env=CARGO_HOME \ + --test_env=GITHUB_RUN_ATTEMPT \ + --test_env=GITHUB_RUN_ID \ + --test_env=HOME \ + --test_env=PATH \ + --test_env=RUSTUP_HOME \ + --test_env=USERPROFILE \ + --build_metadata=COMMIT_SHA=${GITHUB_SHA} \ + -- \ + //codex-rs/... # --- Gatherer job that you mark as the ONLY required status ----------------- results: diff --git a/codex-rs/BUILD.bazel b/codex-rs/BUILD.bazel index b6711d9b81..66c7ebcb61 100644 --- a/codex-rs/BUILD.bazel +++ b/codex-rs/BUILD.bazel @@ -2,3 +2,17 @@ exports_files([ "clippy.toml", "node-version.txt", ]) + +filegroup( + name = "workspace-files", + srcs = glob( + [ + "*", + ".cargo/**", + ], + exclude = [ + "BUILD.bazel", + ], + ), + visibility = ["//visibility:public"], +) diff --git a/defs.bzl b/defs.bzl index 02cbbc6295..e6dbc4aa8f 100644 --- a/defs.bzl +++ b/defs.bzl @@ -57,6 +57,9 @@ def _workspace_root_test_impl(ctx): ) runfiles = ctx.runfiles(files = [test_bin, workspace_root_marker]).merge(ctx.attr.test_bin[DefaultInfo].default_runfiles) + for data_dep in ctx.attr.data: + runfiles = runfiles.merge(ctx.runfiles(files = data_dep[DefaultInfo].files.to_list())) + runfiles = runfiles.merge(data_dep[DefaultInfo].default_runfiles) return [ DefaultInfo( @@ -73,6 +76,9 @@ workspace_root_test = rule( implementation = _workspace_root_test_impl, test = True, attrs = { + "data": attr.label_list( + allow_files = True, + ), "env": attr.string_dict(), "test_bin": attr.label( cfg = "target", @@ -98,6 +104,48 @@ workspace_root_test = rule( }, ) +def _workspace_dep_packages(package_name): + package_data = DEP_DATA.get(package_name, {}) + dependency_labels = [] + + for key in ["deps", "build_deps", "dev_deps"]: + dependency_labels += package_data.get(key, []) + + for key in ["deps_by_platform", "build_deps_by_platform", "dev_deps_by_platform"]: + for labels in package_data.get(key, {}).values(): + dependency_labels += labels + + workspace_packages = [] + for dependency_label in dependency_labels: + if dependency_label.startswith("//codex-rs/"): + workspace_packages.append(Label(dependency_label).package) + + return workspace_packages + +def _argument_comment_lint_data(package_name): + closure = {package_name: True} + frontier = [package_name] + + for _ in range(len(DEP_DATA)): + next_frontier = [] + for current_package in frontier: + for dependency_package in _workspace_dep_packages(current_package): + if dependency_package not in closure: + closure[dependency_package] = True + next_frontier.append(dependency_package) + + if not next_frontier: + break + frontier = next_frontier + + return [ + "//codex-rs:workspace-files", + "//tools/argument-comment-lint:runtime-files", + ] + [ + "//{}:package-files".format(closure_package) + for closure_package in sorted(closure.keys()) + ] + def codex_rust_crate( name, crate_name, @@ -157,10 +205,29 @@ def codex_rust_crate( "INSTA_SNAPSHOT_PATH": "src", } + native.filegroup( + name = "package-files", + srcs = native.glob( + ["**"], + exclude = [ + "**/BUILD.bazel", + "BUILD.bazel", + "target/**", + ], + allow_empty = True, + ), + visibility = ["//visibility:public"], + ) + rustc_env = { "BAZEL_PACKAGE": native.package_name(), } | rustc_env + manifest_relpath = native.package_name() + if manifest_relpath.startswith("codex-rs/"): + manifest_relpath = manifest_relpath[len("codex-rs/"):] + manifest_path = manifest_relpath + "/Cargo.toml" + binaries = DEP_DATA.get(native.package_name())["binaries"] lib_srcs = crate_srcs or native.glob(["src/**/*.rs"], exclude = binaries.values(), allow_empty = True) @@ -278,3 +345,20 @@ def codex_rust_crate( env = cargo_env, tags = test_tags, ) + + workspace_root_test( + name = name + "-argument-comment-lint", + data = _argument_comment_lint_data(native.package_name()), + env = { + "ARGUMENT_COMMENT_LINT_MANIFEST": manifest_path, + }, + # Package-scoped Dylint runs vary widely by crate and platform; the + # default 5-minute Bazel test timeout is too tight for slower crates. + tags = [ + "argument-comment-lint", + "no-sandbox", + ], + test_bin = "//tools/argument-comment-lint:argument-comment-lint-bazel-runner", + timeout = "long", + workspace_root_marker = "//codex-rs/utils/cargo-bin:repo_root.marker", + ) diff --git a/justfile b/justfile index 39a65df46d..fcac32f824 100644 --- a/justfile +++ b/justfile @@ -67,13 +67,26 @@ bazel-lock-check: ./scripts/check-module-bazel-lock.sh bazel-test: - bazel test //... --keep_going + bazel test --test_tag_filters=-argument-comment-lint //... --keep_going bazel-clippy: bazel build --config=clippy -- //codex-rs/... -//codex-rs/v8-poc:all +[no-cd] +bazel-argument-comment-lint: + bazel test \ + --build_tests_only \ + --local_test_jobs=4 \ + --test_tag_filters=argument-comment-lint \ + --test_env=CARGO_HOME \ + --test_env=HOME \ + --test_env=PATH \ + --test_env=RUSTUP_HOME \ + --test_env=USERPROFILE \ + //codex-rs/... + bazel-remote-test: - bazel test //... --config=remote --platforms=//:rbe --keep_going + bazel test --test_tag_filters=-argument-comment-lint //... --config=remote --platforms=//:rbe --keep_going build-for-release: bazel build //codex-rs/cli:release_binaries --config=remote diff --git a/tools/argument-comment-lint/BUILD.bazel b/tools/argument-comment-lint/BUILD.bazel new file mode 100644 index 0000000000..8bc0475dd5 --- /dev/null +++ b/tools/argument-comment-lint/BUILD.bazel @@ -0,0 +1,19 @@ +load("@rules_rust//rust:defs.bzl", "rust_binary") + +filegroup( + name = "runtime-files", + srcs = [ + "argument-comment-lint", + "run-prebuilt-linter.py", + "wrapper_common.py", + ], + visibility = ["//visibility:public"], +) + +rust_binary( + name = "argument-comment-lint-bazel-runner", + crate_name = "argument_comment_lint_bazel_runner", + crate_root = "bazel_runner.rs", + srcs = ["bazel_runner.rs"], + visibility = ["//visibility:public"], +) diff --git a/tools/argument-comment-lint/README.md b/tools/argument-comment-lint/README.md index 92b0739601..bb453681a1 100644 --- a/tools/argument-comment-lint/README.md +++ b/tools/argument-comment-lint/README.md @@ -85,7 +85,9 @@ 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 +`just clippy` and `just argument-comment-lint`. Bazel-backed CI now drives the +same wrapper through package-owned test targets tagged `argument-comment-lint`. +The source-build path remains available in `run.py` for people iterating on the lint crate itself. @@ -128,6 +130,7 @@ Run the lint against `codex-rs` from the repo root: ```bash ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core just argument-comment-lint -p codex-core +bazel test --build_tests_only --test_tag_filters=argument-comment-lint //codex-rs/... ``` If no package selection is provided, `run-prebuilt-linter.py` defaults to checking the diff --git a/tools/argument-comment-lint/bazel_runner.rs b/tools/argument-comment-lint/bazel_runner.rs new file mode 100644 index 0000000000..ac36900832 --- /dev/null +++ b/tools/argument-comment-lint/bazel_runner.rs @@ -0,0 +1,148 @@ +use std::env; +use std::ffi::OsString; +use std::hash::DefaultHasher; +use std::hash::Hash; +use std::hash::Hasher; +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; +use std::process::ExitCode; + +const TOOLCHAIN_CHANNEL: &str = "nightly-2025-09-18"; +#[cfg(target_os = "linux")] +const TOOLCHAIN_TRIPLE: &str = "x86_64-unknown-linux-gnu"; +#[cfg(all(target_os = "macos", target_arch = "aarch64"))] +const TOOLCHAIN_TRIPLE: &str = "aarch64-apple-darwin"; +#[cfg(all(target_os = "macos", target_arch = "x86_64"))] +const TOOLCHAIN_TRIPLE: &str = "x86_64-apple-darwin"; +#[cfg(target_os = "windows")] +const TOOLCHAIN_TRIPLE: &str = "x86_64-pc-windows-msvc"; + +fn main() -> ExitCode { + match run() { + Ok(code) => code, + Err(err) => { + eprintln!("{err}"); + ExitCode::from(1) + } + } +} + +fn run() -> Result { + let manifest = env::var("ARGUMENT_COMMENT_LINT_MANIFEST") + .map_err(|_| "ARGUMENT_COMMENT_LINT_MANIFEST must be set".to_string())?; + let workspace_dir = env::current_dir().map_err(|err| format!("failed to get cwd: {err}"))?; + let wrapper = find_repo_root(&workspace_dir)? + .join("tools") + .join("argument-comment-lint") + .join("run-prebuilt-linter.py"); + + let python = if cfg!(windows) { "python" } else { "python3" }; + let mut command = Command::new(python); + command.arg(&wrapper); + command.arg("--manifest-path"); + command.arg(&manifest); + + // Keep Linux on the narrower target set for now to match the current CI + // rollout, while macOS and Windows continue to exercise all targets. + if cfg!(target_os = "linux") { + command.args(["--", "--lib", "--bins"]); + } + + if env::var_os("CARGO_TARGET_DIR").is_none() { + command.env("CARGO_TARGET_DIR", shared_target_dir(&workspace_dir)); + } + + command.env("RUSTUP_TOOLCHAIN", TOOLCHAIN_CHANNEL); + command.env("RUSTUP_AUTO_INSTALL", "0"); + if env::var_os("RUSTUP_HOME").is_none() + && let Some(rustup_home) = infer_rustup_home() + { + command.env("RUSTUP_HOME", rustup_home); + } + + let status = command + .status() + .map_err(|err| format!("failed to execute {python}: {err}"))?; + Ok(status + .code() + .and_then(|code| u8::try_from(code).ok()) + .map_or_else(|| ExitCode::from(1), ExitCode::from)) +} + +fn find_repo_root(cwd: &Path) -> Result<&Path, String> { + if cwd + .join("tools") + .join("argument-comment-lint") + .join("run-prebuilt-linter.py") + .is_file() + { + return Ok(cwd); + } + + let Some(parent) = cwd.parent() else { + return Err(format!( + "argument-comment wrapper not found relative to {}", + cwd.display() + )); + }; + if parent + .join("tools") + .join("argument-comment-lint") + .join("run-prebuilt-linter.py") + .is_file() + { + return Ok(parent); + } + + Err(format!( + "argument-comment wrapper not found relative to {}", + cwd.display() + )) +} + +fn shared_target_dir(workspace_dir: &Path) -> PathBuf { + let namespace = if let Some(run_id) = env::var_os("GITHUB_RUN_ID") { + let mut namespace = run_id; + if let Some(run_attempt) = env::var_os("GITHUB_RUN_ATTEMPT") { + namespace.push("-"); + namespace.push(run_attempt); + } + namespace + } else { + let mut hasher = DefaultHasher::new(); + workspace_dir.hash(&mut hasher); + OsString::from(format!("{:016x}", hasher.finish())) + }; + + env::temp_dir() + .join("argument-comment-lint") + .join(namespace) + .join(format!("{TOOLCHAIN_CHANNEL}-{TOOLCHAIN_TRIPLE}")) +} + +fn infer_rustup_home() -> Option { + if let Some(rustup_home) = env::var_os("RUSTUP_HOME") { + return Some(rustup_home); + } + + if let Some(cargo_home) = env::var_os("CARGO_HOME") + && let Some(home_dir) = Path::new(&cargo_home).parent() + { + let rustup_home = home_dir.join(".rustup"); + if rustup_home.is_dir() { + return Some(rustup_home.into_os_string()); + } + } + + for var in ["HOME", "USERPROFILE"] { + if let Some(home) = env::var_os(var) { + let rustup_home = Path::new(&home).join(".rustup"); + if rustup_home.is_dir() { + return Some(rustup_home.into_os_string()); + } + } + } + + None +} diff --git a/tools/argument-comment-lint/test_wrapper_common.py b/tools/argument-comment-lint/test_wrapper_common.py index a3a57dfe34..3da6da604b 100644 --- a/tools/argument-comment-lint/test_wrapper_common.py +++ b/tools/argument-comment-lint/test_wrapper_common.py @@ -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() diff --git a/tools/argument-comment-lint/wrapper_common.py b/tools/argument-comment-lint/wrapper_common.py index 3202c22381..75e03ecfbe 100644 --- a/tools/argument-comment-lint/wrapper_common.py +++ b/tools/argument-comment-lint/wrapper_common.py @@ -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