Fix release-please failure from windows-platform-probes self-dependency cycle - #93
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: MikeGrier <220633264+MikeGrier@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The lockfile is stale and default test wiring does not enable the required oracle feature.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
| # every test build, and a matching `serde` dev-dependency was dead weight -- | ||
| # removed after confirming all 213 tests still pass without it. | ||
| # comment claimed.** The integration test that needs that gate is instead | ||
| # declared explicitly above with `required-features = ["oracle-in-renderer"]`, |
There was a problem hiding this comment.
Updated in 646526a: regenerated Cargo.lock and removed the stale windows-platform-probes self-dependency entry from the package dependencies list.
| # declared explicitly above with `required-features = ["oracle-in-renderer"]`, | ||
| # so ordinary unit-test builds keep exercising the `cfg(test)` arm while that | ||
| # one target runs with the feature on. |
There was a problem hiding this comment.
Updated in befe334: corrected the stale oracle-in-renderer wiring references in src/lib.rs and src/topology_report.rs, and updated crates/windows-platform-probes/README.md test command to run with --features oracle-in-renderer.
Co-authored-by: MikeGrier <220633264+MikeGrier@users.noreply.github.com>
Co-authored-by: MikeGrier <220633264+MikeGrier@users.noreply.github.com>
Co-authored-by: MikeGrier <220633264+MikeGrier@users.noreply.github.com>
Co-authored-by: MikeGrier <220633264+MikeGrier@users.noreply.github.com>
Co-authored-by: MikeGrier <220633264+MikeGrier@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved manifest and CI issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
.github/workflows/ci.yml:167
- This step is labeled as the default-feature test pass, but
--features windows-platform-probes/oracle-in-rendererenables a non-default feature for that package. As a result CI no longer runscargo test --workspacein the minimal configuration described at the top of this workflow; the separate probes job also runs with this feature enabled. Keep this command featureless for the default pass, and rely on the probes job (or a separate feature-on pass) for the required integration target.
run: cargo test --workspace --locked --no-fail-fast
crates/windows-platform-probes/Cargo.toml:103
- This target is now omitted by
cargo ... --all-targetscommands unlessoracle-in-rendereris passed. In particular, the existing i686 and x86-64-without-cmpxchg16bdwcas-gatingchecks invokecargo check -p windows-platform-probes --all-targetswithout that feature, so they no longer compile-check this integration target after the self-dependency removal. Add the feature to those cross-target checks or add equivalent feature-on checks to retain that coverage.
required-features = ["oracle-in-renderer"]
crates/windows-platform-probes/src/topology_report.rs:987
- The required-feature integration test does not spawn a probe:
a_real_report_agrees_with_itselfbuilds the report in-process (real_report()callsreport). This comment therefore misstates where the assertion runs and could send future changes to the wrong test boundary; describe the integration target as invoking this renderer directly (the separate stdout test is the one that spawns a binary).
// be discovered: binaries built by the feature-on test command assert, so a
// probe spawned by the required-feature integration target aborts on a
// contradiction instead of printing it; and so does an `--all-features`
- Files reviewed: 5/6 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unfeatured test paths still lack direct serde, CI coverage is incomplete, and default builds skip the documented integration tier.
Review details
Suppressed comments (3)
.github/workflows/ci.yml:233
report_oracleis still compiled for this package's unit-test target undercfg(test), butserdeis declared only as an optional normal dependency. This flag applies only to--test a_real_report_agrees_with_itself; the preceding probe test and the workspace defaultbuild/test --all-targets(as well as the cross-target checks) still select this crate without the feature, so removing the self dev-dependency leaves those test builds without the directserdedependency. Restore a direct test-onlyserdedependency or consistently exclude the probe crate from no-feature workspace checks and run its test targets with the feature enabled.
run: cargo test -p windows-platform-probes --features oracle-in-renderer --locked --test a_real_report_agrees_with_itself -- --include-ignored
crates/windows-platform-probes/Cargo.toml:103
- Declaring
required-featuresmakes this integration target disappear from ordinary default-featurecargo build/test --all-targets; Cargo compiles it only whenoracle-in-rendereris enabled. That contradicts the crate README's unchanged statement that every tier is compiled by an ordinary workspace build (README.md:62-63), so a green default build no longer implies this tier was compiled; update that documentation or make the default build feature-on.
[[test]]
name = "a_real_report_agrees_with_itself"
path = "tests/a_real_report_agrees_with_itself.rs"
required-features = ["oracle-in-renderer"]
crates/windows-platform-probes/Cargo.toml:229
- The unfeatured unit-test command still compiles the library with
cfg(test), so it includesreport_oracle; that module directly usesserde::Deserializer, butserdeis only an optional normal dependency and the remainingserde_jsondev-dependency does not enable this crate's optionalserdefeature. As a result, the ordinary unit-test configuration has no directserdedependency for the oracle path. Addserdeas a dev-dependency (while keeping the optional dependency for feature-on integration builds), or otherwise provide the dependency to the unfeatured unit-test build.
# declared explicitly above with `required-features = ["oracle-in-renderer"]`,
# so ordinary unit-test builds keep exercising the `cfg(test)` arm while that
# one target runs with the feature on.
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Review follow-ups remain for duplicate CI coverage and inaccurate renderer-test documentation.
Review details
Suppressed comments (2)
.github/workflows/ci.yml:233
cargo test --workspace --all-featuresin the build-test job already enables this feature and includes thisrequired-featuresintegration target, so this step runs the same target again without adding coverage. Either remove this duplicate invocation, or excludewindows-platform-probesfrom the workspace-wide all-features test and keep this as its sole feature-on run.
- name: cargo test (windows-platform-probes oracle-in-renderer integration target)
env:
RUST_BACKTRACE: 1
RUST_LIB_BACKTRACE: 1
run: cargo test -p windows-platform-probes --features oracle-in-renderer --locked --test a_real_report_agrees_with_itself -- --include-ignored
crates/windows-platform-probes/src/topology_report.rs:988
- This describes a child process that the required-feature target does not create:
a_real_report_agrees_with_itselfcallsreport/report_unmeasureddirectly in the test process. That distinction matters when documenting what the target exercises; describe the direct renderer call instead of saying a spawned probe aborts.
// be discovered: binaries built by the feature-on test command assert, so a
// probe spawned by the required-feature integration target aborts on a
// contradiction instead of printing it; and so does an `--all-features`
// build. The evidence survives in every case, because the assertion's
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
release-pleasewas failing while building the Cargo workspace dependency graph. The failure came fromwindows-platform-probesdepending on itself indev-dependencies, which introduced a cycle that the release plugin could not order.Manifest change
dev-dependencyfromcrates/windows-platform-probes/Cargo.toml.[[test]]required-features = ["oracle-in-renderer"]CI wiring
oracle-in-rendererexplicitly when running thewindows-platform-probestest suite.report_oracle, without leaving a package-to-itself edge in workspace metadata.Net effect
release-pleasesees an acyclic workspace graph again.