Enable --deny-warnings for cargo shear#21616
Conversation
397d504 to
36473a7
Compare
| with: | ||
| tool: cargo-shear | ||
| version: 1.11.2 | ||
| tool: cargo-shear@1.11.2 |
There was a problem hiding this comment.
This was an oversight from a prior PR (i.e., using the wrong format here).
There was a problem hiding this comment.
These previously weren't included / run at all; I added them to codex-rs/core/tests/suite/mod.rs, then fixed the test, though we could also consider dropping it entirely.
994855a to
6579105
Compare
| @@ -7,9 +7,10 @@ | |||
| //! accepts stdin/terminate frames, and emits a final exit frame. The legacy restricted‑token | |||
| //! path spawns the child directly and does not use this runner. | |||
|
|
|||
| #![cfg(target_os = "windows")] | |||
There was a problem hiding this comment.
No longer necessary because this is now gated at:
#[cfg(target_os = "windows")]
mod win;| approval_texts.is_empty(), | ||
| "did not expect permissions updates before a new user turn: {approval_texts:?}" | ||
| !rollout_path.exists(), | ||
| "did not expect a rollout before a new user turn" |
There was a problem hiding this comment.
These tests weren't being run or compiled at all; this attempts to fix them.
bolinfest
left a comment
There was a problem hiding this comment.
Thanks for all this cleanup!
| #[doc(hidden)] | ||
| pub use read_acl_mutex::ReadAclMutexGuard; | ||
| #[cfg(target_os = "windows")] | ||
| #[doc(hidden)] |
There was a problem hiding this comment.
Let's try to avoid more #[doc(hidden)] sneaking in!
There was a problem hiding this comment.
I recall Codex wanting to try to avoid adding these to the public API to maintain the status quo. I didn't dig into why though.
There was a problem hiding this comment.
(I think it's easiest to just duplicate the one shared utility function for now across the two binaries. The other alternative is a crate which feels too heavy here.)
There was a problem hiding this comment.
FYI, https://github.com/openai/codex/tree/main/codex-rs/utils is where we often put "tiny utility crates," as necessary.
| #[path = "conpty/mod.rs"] | ||
| mod conpty; | ||
|
|
||
| #[cfg(target_os = "windows")] |
There was a problem hiding this comment.
I haven't looked at this in detail, but ideally we would just never try to build this crate on Windows at all and cfg(target_os = "windows") would be assumed throughout and we wouldn't need these repeated annotations at all?
There was a problem hiding this comment.
In uv-windows, we use a top-level bang
then we gate the dependencies on it
I'm not sure if there's a way for a crate to declare it is platform-specific beyond ensuring its empty on other platforms.
There was a problem hiding this comment.
I'll look into as a separate PR. That would be behavior-changing since we'd then omit even the non-Windows-dependent tests (etc.) on other platforms.
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Avoid tripping clippy::duplicated_attributes when Bazel clippy builds the Windows-only sandbox modules with their include sites already cfg-gated. Co-authored-by: Codex <noreply@openai.com>
9f05258 to
b347e68
Compare
Summary
In #21584, we disabled doctests for crates that lack any doctests. We can enforce that property via
cargo shear --deny-warnings: crates that lack doctests will be flagged if doctests are enabled, and crates with doctests will be flagged if doctests are disabled.A few additional notes:
--deny-warnings,cargo shearalso flagged a number of modules that were not reachable at all. Some of those have been removed.windows_modules!(sincecargo shearandrustfmtcouldn't see through it) in favor of simple#[cfg(target_os = "windows")]macros. As a consequence, many of these files exhibit churn in this PR, since they weren't being formatted byrustfmtat all on main.#[path = "cwd_junction.rs"]in favor of a more standard module structure. The bin sidecar structure is still retained, but, e.g.,windows-sandbox-rs/src/bin/command_runner.rswas moved towindows-sandbox-rs/src/bin/command_runner/main.rs, and so on.