mirror of
https://github.com/openai/codex.git
synced 2026-04-27 09:51:03 +03:00
refactor: rewrite argument-comment lint wrappers in Python (#16063)
## Why The `argument-comment-lint` entrypoints had grown into two shell wrappers with duplicated parsing, environment setup, and Cargo forwarding logic. The recent `--` separator regression was a good example of the problem: the behavior was subtle, easy to break, and hard to verify. This change rewrites those wrappers in Python so the control flow is easier to follow, the shared behavior lives in one place, and the tricky argument/defaulting paths have direct test coverage. ## What changed - replaced `tools/argument-comment-lint/run.sh` and `tools/argument-comment-lint/run-prebuilt-linter.sh` with Python entrypoints: `run.py` and `run-prebuilt-linter.py` - moved shared wrapper behavior into `tools/argument-comment-lint/wrapper_common.py`, including: - splitting lint args from forwarded Cargo args after `--` - defaulting repo runs to `--manifest-path codex-rs/Cargo.toml --workspace --no-deps` - defaulting non-`--fix` runs to `--all-targets` unless the caller explicitly narrows the target set - setting repo defaults for `DYLINT_RUSTFLAGS` and `CARGO_INCREMENTAL` - kept the prebuilt wrapper thin: it still just resolves the packaged DotSlash entrypoint, keeps `rustup` shims first on `PATH`, infers `RUSTUP_HOME` when needed, and then launches the packaged `cargo-dylint` path - updated `justfile`, `rust-ci.yml`, and `tools/argument-comment-lint/README.md` to use the Python entrypoints - updated `rust-ci` so the package job runs Python syntax checks plus the new wrapper unit tests, and the OS-specific lint jobs invoke the wrappers through an explicit Python interpreter This is a follow-up to #16054: it keeps the current lint semantics while making the wrapper logic maintainable enough to iterate on safely. ## Validation - `python3 -m py_compile tools/argument-comment-lint/wrapper_common.py tools/argument-comment-lint/run.py tools/argument-comment-lint/run-prebuilt-linter.py tools/argument-comment-lint/test_wrapper_common.py` - `python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'` - `python3 ./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-terminal-detection -- --lib` - `python3 ./tools/argument-comment-lint/run.py -p codex-terminal-detection -- --lib`
This commit is contained in:
16
.github/workflows/rust-ci.yml
vendored
16
.github/workflows/rust-ci.yml
vendored
@@ -116,8 +116,10 @@ jobs:
|
||||
- name: Install cargo-dylint tooling
|
||||
if: ${{ steps.cargo_dylint_cache.outputs.cache-hit != 'true' }}
|
||||
run: cargo install --locked cargo-dylint dylint-link
|
||||
- name: Check source wrapper syntax
|
||||
run: bash -n tools/argument-comment-lint/run.sh
|
||||
- name: Check Python wrapper syntax
|
||||
run: python3 -m py_compile tools/argument-comment-lint/wrapper_common.py tools/argument-comment-lint/run.py tools/argument-comment-lint/run-prebuilt-linter.py tools/argument-comment-lint/test_wrapper_common.py
|
||||
- name: Test Python wrapper helpers
|
||||
run: python3 -m unittest discover -s tools/argument-comment-lint -p 'test_*.py'
|
||||
- name: Test argument comment lint package
|
||||
working-directory: tools/argument-comment-lint
|
||||
run: cargo test
|
||||
@@ -156,11 +158,15 @@ jobs:
|
||||
- name: Run argument comment lint on codex-rs
|
||||
if: ${{ runner.os == 'macOS' }}
|
||||
shell: bash
|
||||
run: ./tools/argument-comment-lint/run-prebuilt-linter.sh
|
||||
run: python3 ./tools/argument-comment-lint/run-prebuilt-linter.py
|
||||
- name: Run argument comment lint on codex-rs (default targets only)
|
||||
if: ${{ runner.os != 'macOS' }}
|
||||
if: ${{ runner.os == 'Linux' }}
|
||||
shell: bash
|
||||
run: ./tools/argument-comment-lint/run-prebuilt-linter.sh -- --lib --bins
|
||||
run: python3 ./tools/argument-comment-lint/run-prebuilt-linter.py -- --lib --bins
|
||||
- name: Run argument comment lint on codex-rs (default targets only)
|
||||
if: ${{ runner.os == 'Windows' }}
|
||||
shell: bash
|
||||
run: python ./tools/argument-comment-lint/run-prebuilt-linter.py -- --lib --bins
|
||||
|
||||
# --- CI to validate on different os/targets --------------------------------
|
||||
lint_build:
|
||||
|
||||
4
justfile
4
justfile
@@ -97,11 +97,11 @@ write-hooks-schema:
|
||||
# Run the argument-comment Dylint checks across codex-rs.
|
||||
[no-cd]
|
||||
argument-comment-lint *args:
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.sh "$@"
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.py "$@"
|
||||
|
||||
[no-cd]
|
||||
argument-comment-lint-from-source *args:
|
||||
./tools/argument-comment-lint/run.sh "$@"
|
||||
./tools/argument-comment-lint/run.py "$@"
|
||||
|
||||
# Tail logs from the state SQLite database
|
||||
log *args:
|
||||
|
||||
@@ -84,9 +84,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.sh` resolves that file via `dotslash` and is the path used by
|
||||
`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.sh` for people
|
||||
source-build path remains available in `run.py` for people
|
||||
iterating on the lint crate itself.
|
||||
|
||||
The Unix archive layout is:
|
||||
@@ -110,7 +110,7 @@ host-qualified nightly filename to the plain `nightly-2025-09-18` channel when
|
||||
needed, and then invokes `cargo-dylint dylint --lib-path <that-library>` with
|
||||
the repo's default `DYLINT_RUSTFLAGS` and `CARGO_INCREMENTAL=0` settings.
|
||||
|
||||
The checked-in `run-prebuilt-linter.sh` wrapper uses the fetched package
|
||||
The checked-in `run-prebuilt-linter.py` wrapper uses the fetched package
|
||||
contents directly so the current checked-in alpha artifact works the same way.
|
||||
It also makes sure the `rustup` shims stay ahead of any direct toolchain
|
||||
`cargo` binary on `PATH`, and sets `RUSTUP_HOME` from `rustup show home` when
|
||||
@@ -120,17 +120,17 @@ required for the current Windows Dylint driver path.
|
||||
If you are changing the lint crate itself, use the source-build wrapper:
|
||||
|
||||
```bash
|
||||
./tools/argument-comment-lint/run.sh -p codex-core
|
||||
./tools/argument-comment-lint/run.py -p codex-core
|
||||
```
|
||||
|
||||
Run the lint against `codex-rs` from the repo root:
|
||||
|
||||
```bash
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.sh -p codex-core
|
||||
./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.sh` defaults to checking the
|
||||
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
|
||||
@@ -140,7 +140,7 @@ Repo runs also promote `uncommented_anonymous_literal_argument` to an error by
|
||||
default:
|
||||
|
||||
```bash
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.sh -p codex-core
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core
|
||||
```
|
||||
|
||||
The wrapper does that by setting `DYLINT_RUSTFLAGS`, and it leaves an explicit
|
||||
@@ -152,11 +152,11 @@ hoc run:
|
||||
```bash
|
||||
DYLINT_RUSTFLAGS="-A uncommented-anonymous-literal-argument" \
|
||||
CARGO_INCREMENTAL=1 \
|
||||
./tools/argument-comment-lint/run.sh -p codex-core
|
||||
./tools/argument-comment-lint/run.py -p codex-core
|
||||
```
|
||||
|
||||
To override an explicitly narrow target selection, or to be explicit in scripts:
|
||||
|
||||
```bash
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.sh -p codex-core -- --all-targets
|
||||
./tools/argument-comment-lint/run-prebuilt-linter.py -p codex-core -- --all-targets
|
||||
```
|
||||
|
||||
45
tools/argument-comment-lint/run-prebuilt-linter.py
Executable file
45
tools/argument-comment-lint/run-prebuilt-linter.py
Executable file
@@ -0,0 +1,45 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import sys
|
||||
|
||||
from wrapper_common import (
|
||||
build_final_args,
|
||||
exec_command,
|
||||
fetch_packaged_entrypoint,
|
||||
find_packaged_cargo_dylint,
|
||||
normalize_packaged_library,
|
||||
parse_wrapper_args,
|
||||
prefer_rustup_shims,
|
||||
repo_root,
|
||||
set_default_lint_env,
|
||||
)
|
||||
|
||||
|
||||
def main() -> "Never":
|
||||
root = repo_root()
|
||||
parsed = parse_wrapper_args(sys.argv[1:])
|
||||
final_args = build_final_args(parsed, root / "codex-rs" / "Cargo.toml")
|
||||
|
||||
env = os.environ.copy()
|
||||
prefer_rustup_shims(env)
|
||||
set_default_lint_env(env)
|
||||
|
||||
package_entrypoint = fetch_packaged_entrypoint(
|
||||
root / "tools" / "argument-comment-lint" / "argument-comment-lint",
|
||||
env,
|
||||
)
|
||||
cargo_dylint = find_packaged_cargo_dylint(package_entrypoint)
|
||||
library_path = normalize_packaged_library(package_entrypoint)
|
||||
|
||||
command = [str(cargo_dylint), "dylint", "--lib-path", str(library_path)]
|
||||
if not parsed.has_library_selection:
|
||||
command.append("--all")
|
||||
command.extend(final_args)
|
||||
exec_command(command, env)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
@@ -1,202 +0,0 @@
|
||||
#!/usr/bin/env bash
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
|
||||
manifest_path="$repo_root/codex-rs/Cargo.toml"
|
||||
dotslash_manifest="$repo_root/tools/argument-comment-lint/argument-comment-lint"
|
||||
|
||||
has_manifest_path=false
|
||||
has_package_selection=false
|
||||
has_library_selection=false
|
||||
has_no_deps=false
|
||||
has_cargo_target_selection=false
|
||||
has_fix=false
|
||||
after_separator=false
|
||||
expect_value=""
|
||||
lint_args=()
|
||||
cargo_args=()
|
||||
|
||||
for arg in "$@"; do
|
||||
if [[ "$after_separator" == true ]]; then
|
||||
cargo_args+=("$arg")
|
||||
case "$arg" in
|
||||
--all-targets|--lib|--bins|--tests|--examples|--benches|--doc)
|
||||
has_cargo_target_selection=true
|
||||
;;
|
||||
--bin|--test|--example|--bench)
|
||||
has_cargo_target_selection=true
|
||||
;;
|
||||
--bin=*|--test=*|--example=*|--bench=*)
|
||||
has_cargo_target_selection=true
|
||||
;;
|
||||
esac
|
||||
continue
|
||||
fi
|
||||
|
||||
case "$arg" in
|
||||
--)
|
||||
after_separator=true
|
||||
continue
|
||||
;;
|
||||
esac
|
||||
|
||||
lint_args+=("$arg")
|
||||
|
||||
if [[ -n "$expect_value" ]]; then
|
||||
case "$expect_value" in
|
||||
manifest_path)
|
||||
has_manifest_path=true
|
||||
;;
|
||||
package_selection)
|
||||
has_package_selection=true
|
||||
;;
|
||||
library_selection)
|
||||
has_library_selection=true
|
||||
;;
|
||||
esac
|
||||
expect_value=""
|
||||
continue
|
||||
fi
|
||||
|
||||
case "$arg" in
|
||||
--manifest-path)
|
||||
expect_value="manifest_path"
|
||||
;;
|
||||
--manifest-path=*)
|
||||
has_manifest_path=true
|
||||
;;
|
||||
-p|--package)
|
||||
expect_value="package_selection"
|
||||
;;
|
||||
--package=*)
|
||||
has_package_selection=true
|
||||
;;
|
||||
--fix)
|
||||
has_fix=true
|
||||
;;
|
||||
--lib|--lib-path)
|
||||
expect_value="library_selection"
|
||||
;;
|
||||
--lib=*|--lib-path=*)
|
||||
has_library_selection=true
|
||||
;;
|
||||
--workspace)
|
||||
has_package_selection=true
|
||||
;;
|
||||
--no-deps)
|
||||
has_no_deps=true
|
||||
;;
|
||||
esac
|
||||
done
|
||||
|
||||
final_args=()
|
||||
if [[ "$has_manifest_path" == false ]]; then
|
||||
final_args+=(--manifest-path "$manifest_path")
|
||||
fi
|
||||
if [[ "$has_package_selection" == false ]]; then
|
||||
final_args+=(--workspace)
|
||||
fi
|
||||
if [[ "$has_no_deps" == false ]]; then
|
||||
final_args+=(--no-deps)
|
||||
fi
|
||||
if [[ "$has_fix" == false && "$has_cargo_target_selection" == false ]]; then
|
||||
cargo_args+=(--all-targets)
|
||||
fi
|
||||
if [[ ${#lint_args[@]} -gt 0 ]]; then
|
||||
final_args+=("${lint_args[@]}")
|
||||
fi
|
||||
if [[ ${#cargo_args[@]} -gt 0 ]]; then
|
||||
final_args+=(-- "${cargo_args[@]}")
|
||||
fi
|
||||
|
||||
if ! command -v dotslash >/dev/null 2>&1; then
|
||||
cat >&2 <<EOF
|
||||
argument-comment-lint prebuilt wrapper requires dotslash.
|
||||
Install dotslash, or use:
|
||||
./tools/argument-comment-lint/run.sh ...
|
||||
EOF
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if command -v rustup >/dev/null 2>&1; then
|
||||
rustup_bin_dir="$(dirname "$(command -v rustup)")"
|
||||
path_entries=()
|
||||
while IFS= read -r entry; do
|
||||
[[ -n "$entry" && "$entry" != "$rustup_bin_dir" ]] && path_entries+=("$entry")
|
||||
done < <(printf '%s\n' "${PATH//:/$'\n'}")
|
||||
PATH="$rustup_bin_dir"
|
||||
if ((${#path_entries[@]} > 0)); then
|
||||
PATH+=":$(IFS=:; echo "${path_entries[*]}")"
|
||||
fi
|
||||
export PATH
|
||||
|
||||
if [[ -z "${RUSTUP_HOME:-}" ]]; then
|
||||
rustup_home="$(rustup show home 2>/dev/null || true)"
|
||||
if [[ -n "$rustup_home" ]]; then
|
||||
export RUSTUP_HOME="$rustup_home"
|
||||
fi
|
||||
fi
|
||||
fi
|
||||
|
||||
package_entrypoint="$(dotslash -- fetch "$dotslash_manifest")"
|
||||
bin_dir="$(cd "$(dirname "$package_entrypoint")" && pwd)"
|
||||
package_root="$(cd "$bin_dir/.." && pwd)"
|
||||
library_dir="$package_root/lib"
|
||||
|
||||
cargo_dylint="$bin_dir/cargo-dylint"
|
||||
if [[ ! -x "$cargo_dylint" ]]; then
|
||||
cargo_dylint="$bin_dir/cargo-dylint.exe"
|
||||
fi
|
||||
if [[ ! -x "$cargo_dylint" ]]; then
|
||||
echo "bundled cargo-dylint executable not found under $bin_dir" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
shopt -s nullglob
|
||||
libraries=("$library_dir"/*@*)
|
||||
shopt -u nullglob
|
||||
if [[ ${#libraries[@]} -eq 0 ]]; then
|
||||
echo "no packaged Dylint library found in $library_dir" >&2
|
||||
exit 1
|
||||
fi
|
||||
if [[ ${#libraries[@]} -ne 1 ]]; then
|
||||
echo "expected exactly one packaged Dylint library in $library_dir" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
library_path="${libraries[0]}"
|
||||
library_filename="$(basename "$library_path")"
|
||||
normalized_library_path="$library_path"
|
||||
library_ext=".${library_filename##*.}"
|
||||
library_stem="${library_filename%.*}"
|
||||
if [[ "$library_stem" =~ ^(.+@nightly-[0-9]{4}-[0-9]{2}-[0-9]{2})-.+$ ]]; then
|
||||
normalized_library_filename="${BASH_REMATCH[1]}$library_ext"
|
||||
temp_dir="$(mktemp -d "${TMPDIR:-/tmp}/argument-comment-lint.XXXXXX")"
|
||||
normalized_library_path="$temp_dir/$normalized_library_filename"
|
||||
cp "$library_path" "$normalized_library_path"
|
||||
fi
|
||||
|
||||
if [[ -n "${DYLINT_RUSTFLAGS:-}" ]]; then
|
||||
if [[ "$DYLINT_RUSTFLAGS" != *"-D uncommented-anonymous-literal-argument"* ]]; then
|
||||
DYLINT_RUSTFLAGS+=" -D uncommented-anonymous-literal-argument"
|
||||
fi
|
||||
if [[ "$DYLINT_RUSTFLAGS" != *"-A unknown_lints"* ]]; then
|
||||
DYLINT_RUSTFLAGS+=" -A unknown_lints"
|
||||
fi
|
||||
else
|
||||
DYLINT_RUSTFLAGS="-D uncommented-anonymous-literal-argument -A unknown_lints"
|
||||
fi
|
||||
export DYLINT_RUSTFLAGS
|
||||
|
||||
if [[ -z "${CARGO_INCREMENTAL:-}" ]]; then
|
||||
export CARGO_INCREMENTAL=0
|
||||
fi
|
||||
|
||||
command=("$cargo_dylint" dylint --lib-path "$normalized_library_path")
|
||||
if [[ "$has_library_selection" == false ]]; then
|
||||
command+=(--all)
|
||||
fi
|
||||
command+=("${final_args[@]}")
|
||||
|
||||
exec "${command[@]}"
|
||||
35
tools/argument-comment-lint/run.py
Executable file
35
tools/argument-comment-lint/run.py
Executable file
@@ -0,0 +1,35 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import sys
|
||||
|
||||
from wrapper_common import (
|
||||
build_final_args,
|
||||
ensure_source_prerequisites,
|
||||
exec_command,
|
||||
parse_wrapper_args,
|
||||
repo_root,
|
||||
set_default_lint_env,
|
||||
)
|
||||
|
||||
|
||||
def main() -> "Never":
|
||||
root = repo_root()
|
||||
parsed = parse_wrapper_args(sys.argv[1:])
|
||||
final_args = build_final_args(parsed, root / "codex-rs" / "Cargo.toml")
|
||||
|
||||
env = os.environ.copy()
|
||||
ensure_source_prerequisites(env)
|
||||
set_default_lint_env(env)
|
||||
|
||||
command = ["cargo", "dylint", "--path", str(root / "tools" / "argument-comment-lint")]
|
||||
if not parsed.has_library_selection:
|
||||
command.append("--all")
|
||||
command.extend(final_args)
|
||||
exec_command(command, env)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
@@ -1,161 +0,0 @@
|
||||
#!/usr/bin/env bash
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"
|
||||
lint_path="$repo_root/tools/argument-comment-lint"
|
||||
manifest_path="$repo_root/codex-rs/Cargo.toml"
|
||||
toolchain_channel="nightly-2025-09-18"
|
||||
strict_lint="uncommented-anonymous-literal-argument"
|
||||
noise_lint="unknown_lints"
|
||||
|
||||
has_manifest_path=false
|
||||
has_package_selection=false
|
||||
has_no_deps=false
|
||||
has_library_selection=false
|
||||
has_cargo_target_selection=false
|
||||
has_fix=false
|
||||
after_separator=false
|
||||
expect_value=""
|
||||
lint_args=()
|
||||
cargo_args=()
|
||||
|
||||
ensure_local_prerequisites() {
|
||||
if ! command -v cargo-dylint >/dev/null 2>&1 || ! command -v dylint-link >/dev/null 2>&1; then
|
||||
cat >&2 <<EOF
|
||||
argument-comment-lint source wrapper requires cargo-dylint and dylint-link.
|
||||
Install them with:
|
||||
cargo install --locked cargo-dylint dylint-link
|
||||
EOF
|
||||
exit 1
|
||||
fi
|
||||
|
||||
if ! rustup toolchain list | grep -q "^${toolchain_channel}"; then
|
||||
cat >&2 <<EOF
|
||||
argument-comment-lint source wrapper requires the ${toolchain_channel} toolchain with rustc-dev support.
|
||||
Install it with:
|
||||
rustup toolchain install ${toolchain_channel} \\
|
||||
--component llvm-tools-preview \\
|
||||
--component rustc-dev \\
|
||||
--component rust-src
|
||||
EOF
|
||||
exit 1
|
||||
fi
|
||||
}
|
||||
|
||||
set_default_env() {
|
||||
if [[ "${DYLINT_RUSTFLAGS:-}" != *"$strict_lint"* ]]; then
|
||||
export DYLINT_RUSTFLAGS="${DYLINT_RUSTFLAGS:+${DYLINT_RUSTFLAGS} }-D $strict_lint"
|
||||
fi
|
||||
if [[ "${DYLINT_RUSTFLAGS:-}" != *"$noise_lint"* ]]; then
|
||||
export DYLINT_RUSTFLAGS="${DYLINT_RUSTFLAGS:+${DYLINT_RUSTFLAGS} }-A $noise_lint"
|
||||
fi
|
||||
|
||||
if [[ -z "${CARGO_INCREMENTAL:-}" ]]; then
|
||||
export CARGO_INCREMENTAL=0
|
||||
fi
|
||||
}
|
||||
|
||||
for arg in "$@"; do
|
||||
if [[ "$after_separator" == true ]]; then
|
||||
cargo_args+=("$arg")
|
||||
case "$arg" in
|
||||
--all-targets|--lib|--bins|--tests|--examples|--benches|--doc)
|
||||
has_cargo_target_selection=true
|
||||
;;
|
||||
--bin|--test|--example|--bench)
|
||||
has_cargo_target_selection=true
|
||||
;;
|
||||
--bin=*|--test=*|--example=*|--bench=*)
|
||||
has_cargo_target_selection=true
|
||||
;;
|
||||
esac
|
||||
continue
|
||||
fi
|
||||
|
||||
case "$arg" in
|
||||
--)
|
||||
after_separator=true
|
||||
continue
|
||||
;;
|
||||
esac
|
||||
|
||||
lint_args+=("$arg")
|
||||
|
||||
if [[ -n "$expect_value" ]]; then
|
||||
case "$expect_value" in
|
||||
manifest_path)
|
||||
has_manifest_path=true
|
||||
;;
|
||||
package_selection)
|
||||
has_package_selection=true
|
||||
;;
|
||||
library_selection)
|
||||
has_library_selection=true
|
||||
;;
|
||||
esac
|
||||
expect_value=""
|
||||
continue
|
||||
fi
|
||||
|
||||
case "$arg" in
|
||||
--manifest-path)
|
||||
expect_value="manifest_path"
|
||||
;;
|
||||
--manifest-path=*)
|
||||
has_manifest_path=true
|
||||
;;
|
||||
-p|--package)
|
||||
expect_value="package_selection"
|
||||
;;
|
||||
--package=*)
|
||||
has_package_selection=true
|
||||
;;
|
||||
--fix)
|
||||
has_fix=true
|
||||
;;
|
||||
--workspace)
|
||||
has_package_selection=true
|
||||
;;
|
||||
--no-deps)
|
||||
has_no_deps=true
|
||||
;;
|
||||
--lib|--lib-path)
|
||||
expect_value="library_selection"
|
||||
;;
|
||||
--lib=*|--lib-path=*)
|
||||
has_library_selection=true
|
||||
;;
|
||||
esac
|
||||
done
|
||||
|
||||
final_args=()
|
||||
if [[ "$has_manifest_path" == false ]]; then
|
||||
final_args+=(--manifest-path "$manifest_path")
|
||||
fi
|
||||
if [[ "$has_package_selection" == false ]]; then
|
||||
final_args+=(--workspace)
|
||||
fi
|
||||
if [[ "$has_no_deps" == false ]]; then
|
||||
final_args+=(--no-deps)
|
||||
fi
|
||||
if [[ "$has_fix" == false && "$has_cargo_target_selection" == false ]]; then
|
||||
cargo_args+=(--all-targets)
|
||||
fi
|
||||
if [[ ${#lint_args[@]} -gt 0 ]]; then
|
||||
final_args+=("${lint_args[@]}")
|
||||
fi
|
||||
if [[ ${#cargo_args[@]} -gt 0 ]]; then
|
||||
final_args+=(-- "${cargo_args[@]}")
|
||||
fi
|
||||
|
||||
ensure_local_prerequisites
|
||||
set_default_env
|
||||
|
||||
cmd=(cargo dylint --path "$lint_path")
|
||||
if [[ "$has_library_selection" == false ]]; then
|
||||
cmd+=(--all)
|
||||
fi
|
||||
cmd+=("${final_args[@]}")
|
||||
|
||||
exec "${cmd[@]}"
|
||||
88
tools/argument-comment-lint/test_wrapper_common.py
Normal file
88
tools/argument-comment-lint/test_wrapper_common.py
Normal file
@@ -0,0 +1,88 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
import unittest
|
||||
|
||||
import wrapper_common
|
||||
|
||||
|
||||
class WrapperCommonTest(unittest.TestCase):
|
||||
def test_defaults_to_workspace_and_all_targets(self) -> None:
|
||||
parsed = wrapper_common.parse_wrapper_args([])
|
||||
final_args = wrapper_common.build_final_args(parsed, Path("/repo/codex-rs/Cargo.toml"))
|
||||
|
||||
self.assertEqual(
|
||||
final_args,
|
||||
[
|
||||
"--manifest-path",
|
||||
"/repo/codex-rs/Cargo.toml",
|
||||
"--workspace",
|
||||
"--no-deps",
|
||||
"--",
|
||||
"--all-targets",
|
||||
],
|
||||
)
|
||||
|
||||
def test_forwarded_cargo_args_keep_single_separator(self) -> None:
|
||||
parsed = wrapper_common.parse_wrapper_args(["-p", "codex-core", "--", "--tests"])
|
||||
final_args = wrapper_common.build_final_args(parsed, Path("/repo/codex-rs/Cargo.toml"))
|
||||
|
||||
self.assertEqual(
|
||||
final_args,
|
||||
[
|
||||
"--manifest-path",
|
||||
"/repo/codex-rs/Cargo.toml",
|
||||
"--no-deps",
|
||||
"-p",
|
||||
"codex-core",
|
||||
"--",
|
||||
"--tests",
|
||||
],
|
||||
)
|
||||
|
||||
def test_fix_does_not_add_all_targets(self) -> None:
|
||||
parsed = wrapper_common.parse_wrapper_args(["--fix", "-p", "codex-core"])
|
||||
final_args = wrapper_common.build_final_args(parsed, Path("/repo/codex-rs/Cargo.toml"))
|
||||
|
||||
self.assertEqual(
|
||||
final_args,
|
||||
[
|
||||
"--manifest-path",
|
||||
"/repo/codex-rs/Cargo.toml",
|
||||
"--no-deps",
|
||||
"--fix",
|
||||
"-p",
|
||||
"codex-core",
|
||||
],
|
||||
)
|
||||
|
||||
def test_explicit_manifest_and_workspace_are_preserved(self) -> None:
|
||||
parsed = wrapper_common.parse_wrapper_args(
|
||||
[
|
||||
"--manifest-path",
|
||||
"/tmp/custom/Cargo.toml",
|
||||
"--workspace",
|
||||
"--no-deps",
|
||||
"--",
|
||||
"--bins",
|
||||
]
|
||||
)
|
||||
final_args = wrapper_common.build_final_args(parsed, Path("/repo/codex-rs/Cargo.toml"))
|
||||
|
||||
self.assertEqual(
|
||||
final_args,
|
||||
[
|
||||
"--manifest-path",
|
||||
"/tmp/custom/Cargo.toml",
|
||||
"--workspace",
|
||||
"--no-deps",
|
||||
"--",
|
||||
"--bins",
|
||||
],
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
271
tools/argument-comment-lint/wrapper_common.py
Normal file
271
tools/argument-comment-lint/wrapper_common.py
Normal file
@@ -0,0 +1,271 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from dataclasses import dataclass
|
||||
import os
|
||||
from pathlib import Path
|
||||
import re
|
||||
import shlex
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
from typing import MutableMapping, Sequence
|
||||
|
||||
STRICT_LINT = "uncommented-anonymous-literal-argument"
|
||||
NOISE_LINT = "unknown_lints"
|
||||
TOOLCHAIN_CHANNEL = "nightly-2025-09-18"
|
||||
|
||||
_TARGET_SELECTION_ARGS = {
|
||||
"--all-targets",
|
||||
"--lib",
|
||||
"--bins",
|
||||
"--tests",
|
||||
"--examples",
|
||||
"--benches",
|
||||
"--doc",
|
||||
}
|
||||
_TARGET_SELECTION_PREFIXES = ("--bin=", "--test=", "--example=", "--bench=")
|
||||
_TARGET_SELECTION_WITH_VALUE = {"--bin", "--test", "--example", "--bench"}
|
||||
_NIGHTLY_LIBRARY_PATTERN = re.compile(
|
||||
r"^(.+@nightly-[0-9]{4}-[0-9]{2}-[0-9]{2})-.+$"
|
||||
)
|
||||
|
||||
|
||||
@dataclass
|
||||
class ParsedWrapperArgs:
|
||||
lint_args: list[str]
|
||||
cargo_args: list[str]
|
||||
has_manifest_path: bool = False
|
||||
has_package_selection: bool = False
|
||||
has_no_deps: bool = False
|
||||
has_library_selection: bool = False
|
||||
has_cargo_target_selection: bool = False
|
||||
has_fix: bool = False
|
||||
|
||||
|
||||
def repo_root() -> Path:
|
||||
return Path(__file__).resolve().parents[2]
|
||||
|
||||
|
||||
def parse_wrapper_args(argv: Sequence[str]) -> ParsedWrapperArgs:
|
||||
parsed = ParsedWrapperArgs(lint_args=[], cargo_args=[])
|
||||
after_separator = False
|
||||
expect_value: str | None = None
|
||||
|
||||
for arg in argv:
|
||||
if after_separator:
|
||||
parsed.cargo_args.append(arg)
|
||||
if arg in _TARGET_SELECTION_ARGS or arg in _TARGET_SELECTION_WITH_VALUE:
|
||||
parsed.has_cargo_target_selection = True
|
||||
elif arg.startswith(_TARGET_SELECTION_PREFIXES):
|
||||
parsed.has_cargo_target_selection = True
|
||||
continue
|
||||
|
||||
if arg == "--":
|
||||
after_separator = True
|
||||
continue
|
||||
|
||||
parsed.lint_args.append(arg)
|
||||
|
||||
if expect_value is not None:
|
||||
if expect_value == "manifest_path":
|
||||
parsed.has_manifest_path = True
|
||||
elif expect_value == "package_selection":
|
||||
parsed.has_package_selection = True
|
||||
elif expect_value == "library_selection":
|
||||
parsed.has_library_selection = True
|
||||
expect_value = None
|
||||
continue
|
||||
|
||||
if arg == "--manifest-path":
|
||||
expect_value = "manifest_path"
|
||||
elif arg.startswith("--manifest-path="):
|
||||
parsed.has_manifest_path = True
|
||||
elif arg in {"-p", "--package"}:
|
||||
expect_value = "package_selection"
|
||||
elif arg.startswith("--package="):
|
||||
parsed.has_package_selection = True
|
||||
elif arg == "--fix":
|
||||
parsed.has_fix = True
|
||||
elif arg == "--workspace":
|
||||
parsed.has_package_selection = True
|
||||
elif arg == "--no-deps":
|
||||
parsed.has_no_deps = True
|
||||
elif arg in {"--lib", "--lib-path"}:
|
||||
expect_value = "library_selection"
|
||||
elif arg.startswith("--lib=") or arg.startswith("--lib-path="):
|
||||
parsed.has_library_selection = True
|
||||
|
||||
return parsed
|
||||
|
||||
|
||||
def build_final_args(parsed: ParsedWrapperArgs, manifest_path: Path) -> list[str]:
|
||||
final_args: list[str] = []
|
||||
cargo_args = list(parsed.cargo_args)
|
||||
|
||||
if not parsed.has_manifest_path:
|
||||
final_args.extend(["--manifest-path", str(manifest_path)])
|
||||
if not parsed.has_package_selection:
|
||||
final_args.append("--workspace")
|
||||
if not parsed.has_no_deps:
|
||||
final_args.append("--no-deps")
|
||||
if not parsed.has_fix and not parsed.has_cargo_target_selection:
|
||||
cargo_args.append("--all-targets")
|
||||
final_args.extend(parsed.lint_args)
|
||||
if cargo_args:
|
||||
final_args.extend(["--", *cargo_args])
|
||||
return final_args
|
||||
|
||||
|
||||
def append_env_flag(env: MutableMapping[str, str], key: str, flag: str) -> None:
|
||||
value = env.get(key)
|
||||
if value is None or value == "":
|
||||
env[key] = flag
|
||||
return
|
||||
if flag not in value:
|
||||
env[key] = f"{value} {flag}"
|
||||
|
||||
|
||||
def set_default_lint_env(env: MutableMapping[str, str]) -> None:
|
||||
append_env_flag(env, "DYLINT_RUSTFLAGS", f"-D {STRICT_LINT}")
|
||||
append_env_flag(env, "DYLINT_RUSTFLAGS", f"-A {NOISE_LINT}")
|
||||
if not env.get("CARGO_INCREMENTAL"):
|
||||
env["CARGO_INCREMENTAL"] = "0"
|
||||
|
||||
|
||||
def die(message: str) -> "Never":
|
||||
print(message, file=sys.stderr)
|
||||
raise SystemExit(1)
|
||||
|
||||
|
||||
def require_command(name: str, install_message: str | None = None) -> str:
|
||||
executable = shutil.which(name)
|
||||
if executable is None:
|
||||
if install_message is None:
|
||||
die(f"{name} is required but was not found on PATH.")
|
||||
die(install_message)
|
||||
return executable
|
||||
|
||||
|
||||
def run_capture(args: Sequence[str], env: MutableMapping[str, str] | None = None) -> str:
|
||||
try:
|
||||
completed = subprocess.run(
|
||||
list(args),
|
||||
capture_output=True,
|
||||
check=True,
|
||||
env=None if env is None else dict(env),
|
||||
text=True,
|
||||
)
|
||||
except subprocess.CalledProcessError as error:
|
||||
command = shlex.join(str(part) for part in error.cmd)
|
||||
stderr = error.stderr.strip()
|
||||
stdout = error.stdout.strip()
|
||||
output = stderr or stdout
|
||||
if output:
|
||||
die(f"{command} failed:\n{output}")
|
||||
die(f"{command} failed with exit code {error.returncode}")
|
||||
return completed.stdout.strip()
|
||||
|
||||
|
||||
def ensure_source_prerequisites(env: MutableMapping[str, str]) -> None:
|
||||
require_command(
|
||||
"cargo-dylint",
|
||||
"argument-comment-lint source wrapper requires cargo-dylint and dylint-link.\n"
|
||||
"Install them with:\n"
|
||||
" cargo install --locked cargo-dylint dylint-link",
|
||||
)
|
||||
require_command(
|
||||
"dylint-link",
|
||||
"argument-comment-lint source wrapper requires cargo-dylint and dylint-link.\n"
|
||||
"Install them with:\n"
|
||||
" cargo install --locked cargo-dylint dylint-link",
|
||||
)
|
||||
require_command(
|
||||
"rustup",
|
||||
"argument-comment-lint source wrapper requires rustup.\n"
|
||||
f"Install the {TOOLCHAIN_CHANNEL} toolchain with:\n"
|
||||
f" rustup toolchain install {TOOLCHAIN_CHANNEL} \\\n"
|
||||
" --component llvm-tools-preview \\\n"
|
||||
" --component rustc-dev \\\n"
|
||||
" --component rust-src",
|
||||
)
|
||||
toolchains = run_capture(["rustup", "toolchain", "list"], env=env)
|
||||
if not any(line.startswith(TOOLCHAIN_CHANNEL) for line in toolchains.splitlines()):
|
||||
die(
|
||||
"argument-comment-lint source wrapper requires the "
|
||||
f"{TOOLCHAIN_CHANNEL} toolchain with rustc-dev support.\n"
|
||||
"Install it with:\n"
|
||||
f" rustup toolchain install {TOOLCHAIN_CHANNEL} \\\n"
|
||||
" --component llvm-tools-preview \\\n"
|
||||
" --component rustc-dev \\\n"
|
||||
" --component rust-src"
|
||||
)
|
||||
|
||||
|
||||
def prefer_rustup_shims(env: MutableMapping[str, str]) -> None:
|
||||
rustup = shutil.which("rustup", path=env.get("PATH"))
|
||||
if rustup is None:
|
||||
return
|
||||
|
||||
rustup_bin_dir = str(Path(rustup).resolve().parent)
|
||||
path_entries = [
|
||||
entry
|
||||
for entry in env.get("PATH", "").split(os.pathsep)
|
||||
if entry and entry != rustup_bin_dir
|
||||
]
|
||||
env["PATH"] = os.pathsep.join([rustup_bin_dir, *path_entries])
|
||||
|
||||
if not env.get("RUSTUP_HOME"):
|
||||
rustup_home = run_capture(["rustup", "show", "home"], env=env)
|
||||
if rustup_home:
|
||||
env["RUSTUP_HOME"] = rustup_home
|
||||
|
||||
|
||||
def fetch_packaged_entrypoint(dotslash_manifest: Path, env: MutableMapping[str, str]) -> Path:
|
||||
require_command(
|
||||
"dotslash",
|
||||
"argument-comment-lint prebuilt wrapper requires dotslash.\n"
|
||||
"Install dotslash, or use:\n"
|
||||
" ./tools/argument-comment-lint/run.py ...",
|
||||
)
|
||||
entrypoint = run_capture(["dotslash", "--", "fetch", str(dotslash_manifest)], env=env)
|
||||
return Path(entrypoint).resolve()
|
||||
|
||||
|
||||
def find_packaged_cargo_dylint(package_entrypoint: Path) -> Path:
|
||||
bin_dir = package_entrypoint.parent
|
||||
cargo_dylint = bin_dir / "cargo-dylint"
|
||||
if not cargo_dylint.is_file():
|
||||
cargo_dylint = bin_dir / "cargo-dylint.exe"
|
||||
if not cargo_dylint.is_file():
|
||||
die(f"bundled cargo-dylint executable not found under {bin_dir}")
|
||||
return cargo_dylint
|
||||
|
||||
|
||||
def normalize_packaged_library(package_entrypoint: Path) -> Path:
|
||||
library_dir = package_entrypoint.parent.parent / "lib"
|
||||
libraries = sorted(path for path in library_dir.glob("*@*") if path.is_file())
|
||||
if not libraries:
|
||||
die(f"no packaged Dylint library found in {library_dir}")
|
||||
if len(libraries) != 1:
|
||||
die(f"expected exactly one packaged Dylint library in {library_dir}")
|
||||
|
||||
library_path = libraries[0]
|
||||
match = _NIGHTLY_LIBRARY_PATTERN.match(library_path.stem)
|
||||
if match is None:
|
||||
return library_path
|
||||
|
||||
temp_dir = Path(tempfile.mkdtemp(prefix="argument-comment-lint."))
|
||||
normalized_library_path = temp_dir / f"{match.group(1)}{library_path.suffix}"
|
||||
shutil.copy2(library_path, normalized_library_path)
|
||||
return normalized_library_path
|
||||
|
||||
|
||||
def exec_command(command: Sequence[str], env: MutableMapping[str, str]) -> "Never":
|
||||
try:
|
||||
completed = subprocess.run(list(command), env=dict(env), check=False)
|
||||
except FileNotFoundError:
|
||||
die(f"{command[0]} is required but was not found on PATH.")
|
||||
raise SystemExit(completed.returncode)
|
||||
Reference in New Issue
Block a user