rust: take lints from cargo unit graph#239
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
AI review found issues in this pull request. Verdict: patch is incorrect The patch breaks existing eval assertions around the shared Rust lint policy and adds an incomplete manifest lint renderer for
Some inline comments could not be placed on the diff:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec7295061e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Andrew Gazelka (andrewgazelka)
force-pushed
the
codex/cargo-unit-manifest-lints
branch
from
ec72950 to
4b14005
Compare
Andrew Gazelka (andrewgazelka)
force-pushed
the
codex/cargo-unit-manifest-lints
branch
from
4b14005 to
fa14265
Compare
Andrew Gazelka (andrewgazelka)
changed the title
Andrew Gazelka (andrewgazelka)
deleted the
codex/cargo-unit-manifest-lints
branch
There was a problem hiding this comment.
AI review found issues in this pull request.
Verdict: patch is incorrect
Confidence: 0.90
The patch leaves the test suite internally inconsistent after clearing the shared clippy lint defaults, so required repo checks are expected to fail.
- P1
lib/rust.nix:40Default lint policy now contradicts the remaining repo-package test
| deniedLints = [ ]; | ||
| allowedLints = [ ]; |
There was a problem hiding this comment.
Changing the shared default clippy.deniedLints to [] applies to every rust.resolvePolicy caller, including the repo package workspace exposed through repoPackages.*.passthru.policy. The PR updates the cargoUnitWorkspace assertion to expect an empty list, but the existing tests/default.nix repo-package assertion still expects warnings and the clippy groups, so the repo eval/lint check will fail. Either update that remaining assertion to the new Cargo.toml-owned contract or keep an explicit strict policy for repo packages.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa14265d9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| #[serde(default)] | ||
| pub lint_rustflags: Vec<String>, | ||
| #[serde(default)] | ||
| pub check_cfg_args: Vec<String>, |
There was a problem hiding this comment.
When the real Cargo unit graph is used, these fields are absent: Cargo's unit_graph::SerializedUnit still serializes only pkg_id, target, profile, platform, mode, features, is_std, and dependencies, and a local cargo +1.95.0 build --unit-graph -Z unstable-options with [lints] produced no lint_rustflags or check_cfg_args. Because the new fields default to empty here, every generated unit silently drops the workspace manifest lints, so nix-cargo-unit builds no longer deny warnings after the Nix-side defaults were removed.
Useful? React with 👍 / 👎.
| for rustflag in &unit.profile.rustflags { | ||
| push_arg(script, rustflag); | ||
| } | ||
| for rustflag in &unit.lint_rustflags { | ||
| push_arg(script, rustflag); |
There was a problem hiding this comment.
For packages that set profile rustflags to override a manifest lint level, this renders the flags in the opposite order from Cargo: Cargo appends manifest lint flags before profile rustflags, while this puts profile.rustflags first and then lint_rustflags. Since rustc uses the later lint level for the same lint, generated unit builds can fail or pass differently from cargo build whenever profile rustflags adjust a lint configured in [lints].
Useful? React with 👍 / 👎.
| for arg in &unit.check_cfg_args { | ||
| push_arg(script, arg); | ||
| } |
There was a problem hiding this comment.
For crates that enable unexpected_cfgs and have a build script emit a dynamic cfg, Cargo forwards both cargo::rustc-cfg=has_foo and its matching cargo::rustc-check-cfg=cfg(has_foo) to the final rustc command. This only appends the static unit check_cfg_args, while the build-script reader later consumes rustc-cfg but not rustc-check-cfg, so generated builds reject valid build-script cfgs as unexpected even though cargo build accepts them.
Useful? React with 👍 / 👎.
| cargo = "deny" | ||
| multiple_crate_versions = "allow" |
There was a problem hiding this comment.
With manifest lint tables, Cargo ignores table order and Clippy rejects a lint group and an overridden member at the same priority. This combination makes cargo clippy fail with clippy::lint_groups_priority before checking the crate because clippy::cargo and clippy::multiple_crate_versions both default to priority 0; the old CLI flags did not trigger that manifest-priority lint. Set the group to a lower priority, for example cargo = { level = "deny", priority = -1 }, so the allow can override it.
Useful? React with 👍 / 👎.
The Nix-side clippy policy stopped seeding default deniedLints once the workspace started inheriting [workspace.lints] in Cargo.toml (#239), so the eval-time check should mirror that contract instead of asserting the old strict-lint list.
1 participant
Summary
[workspace.lints]and opt every workspace crate into inherited lints-Dand-Aflagsnix-cargo-unitto consumelint_rustflagsandcheck_cfg_argsfrom Cargo's unit graph instead of parsing manifest lint tables locallyValidation
nix run .#lint