Repository navigation
fix: stop enabling tracing's log feature and restore no_std builds - #2064
Benoît Cortier (CBenoit) wants to merge 3 commits into
Conversation
`ironrdp-rdpeudp` and `ironrdp-rdpemt` depended on `tracing` with its default features, which enable `std`. Their `--no-default-features` builds therefore failed on targets without `std`. Host-only feature checks could not catch this, since the host always provides `std`. Disable `tracing`'s default features and forward `tracing/std` from each crate's existing `std` feature. Also drop `tracing/log`: a library should not enable log interoperability for every consumer, and applications that need it can enable it themselves. Extend the xtask feature matrix so this is checked in CI: - `workspace/no-std-target` builds the no_std crates with default features disabled for `x86_64-unknown-none`, a target without `std` (installed by `cargo xtask check install`). - `workspace/powerset-multitransport` adds rdpeudp and rdpemt, which were not part of any powerset group. Also move an import only used by tests into the test module, fixing an unused-import warning in the rdpeudp no_std build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The combined title scope is rejected by the repository’s PR-title validator; the implementation itself appears sound.
Review effort: Balanced
Findings: None
What changed in this PR
Restores genuine no_std support for RDPEUDP/RDPEMT and adds CI coverage against a bare-metal target.
Changes:
- Disables
tracingdefaults and forwardstracing/std. - Adds no-std and multitransport feature-matrix cases.
- Localizes a test-only
vec!import.
Required metadata fix: Use one canonical scope—or no scope—instead of rdpeudp,rdpemt.
| File | Description |
|---|---|
xtask/src/features.rs |
Adds no-std and powerset checks. |
xtask/src/check.rs |
Installs the bare-metal target. |
crates/ironrdp-rdpeudp/src/pdu/v1_ack.rs |
Moves the macro import into tests. |
crates/ironrdp-rdpeudp/Cargo.toml |
Corrects tracing feature wiring. |
crates/ironrdp-rdpemt/Cargo.toml |
Corrects tracing feature wiring. |
There was a problem hiding this comment.
PR #2064 restores no_std builds for ironrdp-rdpeudp and ironrdp-rdpemt by disabling tracing's default features and forwarding tracing/std from each crate's std feature, moves a test-only alloc::vec import into the test module, and adds two new xtask feature-matrix cases (a real no_std-target check and a powerset group for the multitransport crates) plus the rustup target install. The core fix is correct: tracing's default std feature explains the x86_64-unknown-none breakage, neither crate uses #[instrument] so dropping tracing's default features (including attributes) is compile-safe, and in-workspace consumers enable tracing/log so their output is unchanged. Two valid specialist findings were confirmed: (1) dropping the tracing/log bridge is a policy choice bundled with the fix that makes the two crates the sole exceptions to the workspace-wide tracing features=["log"] pattern and silently removes log bridging for external consumers (low severity, accepted); (2) the new NoStdTarget …
`features = ["log"]` on `tracing` entered the workspace dependency table incidentally in #193 and was then copied into every crate manifest. Libraries should not enable log interoperability for all their consumers, and no application in the workspace needs it: the viewer, the FFI, the web client, the daemon, the testsuite and the benches all install a `tracing-subscriber` subscriber. Drop the feature everywhere. For the `no_std` crates (`ironrdp-dvc`, `ironrdp-rdpecam`, `ironrdp-rdpel`), also disable `tracing`'s default features, and forward `tracing/std` from `ironrdp-dvc`'s existing `std` feature, matching `ironrdp-rdpeudp` and `ironrdp-rdpemt`. `tracing` no longer depends on `log` in either lock file; `log` itself drops out of `fuzz/Cargo.lock`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PR #2064 restores no_std builds of ironrdp-rdpeudp/ironrdp-rdpemt by disabling tracing's default features and forwarding tracing/std from the existing std features, removes the incidental features = ["log"] from all 35 manifests (verified: no tracing_log/LogTracer usage in the workspace, all consumers install a tracing-subscriber), moves a test-only alloc::vec import into the test module, and adds a workspace/no-std-target feature-matrix case checking on x86_64-unknown-none with the target installed by `cargo xtask check install`. Independent verification of the head confirms the manifests, the rdpeudp test-import fix, the xtask NoStdTarget arm, and the check.rs wiring are correct; rdpecam/rdpel correctly need no std forwarding since they have no std feature. The only residual issue is a four-line package-argument loop duplicated verbatim between the CargoHack and NoStdTarget arms of run_one — valid but low-severity optional polish that the maintainer has explicitly declined, so it is…
| Invocation::NoStdTarget { packages } => { | ||
| let mut args: Vec<String> = vec![ | ||
| "check".into(), | ||
| "--locked".into(), | ||
| "--no-default-features".into(), | ||
| "--target".into(), | ||
| NO_STD_TARGET.into(), | ||
| ]; | ||
| for pkg in *packages { | ||
| args.push("-p".into()); | ||
| args.push((*pkg).into()); | ||
| } | ||
| cmd!(sh, "{CARGO}").args(&args).run()?; | ||
| } |
There was a problem hiding this comment.
[code-compressor] NoStdTarget arm duplicates CargoHack's package-argument loop — low 🟡 — The new NoStdTarget arm repeats the exact four-line pattern already present in the CargoHack arm: for pkg in *packages { args.push("-p".into()); args.push((*pkg).into()); }. A small shared helper would remove the duplication and keep the two invocations consistent if the flag ever changes. Optional compression only: the duplication is four lines, behavior is identical either way, and the maintainer has explicitly declined the helper in PR review (it would also touch the unrelated CargoHack arm, and self-contained arms match existing run_one style). No correctness impact.
There was a problem hiding this comment.
Not changed: same finding as the earlier thread, which was declined. See the reply on that thread.
🤖 Addressed by Claude Code
|
Oops. |
|
This one's on me, and I'm sorry. Recent AI model upgrades have caused me some trouble with my usual safeguards and processes, and this got through because of it. Thank you for fixing it so quickly. The no-std-target check you added is the one those PRs needed, and I've added the same x86_64-unknown-none build to my own pre-push checks. |
…udp-tracing-cleanup-cc3092 # Conflicts: # crates/ironrdp-rdpdr-native/Cargo.toml
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
What
1. Restore no_std builds of
ironrdp-rdpeudpandironrdp-rdpemttracingnow usesdefault-features = false, andtracing/stdis forwarded from each crate's existingstdfeature. No new crate features.alloc::vecimport that only the tests use now lives in the test module, which fixes an unused-import warning in the rdpeudp no_std build.2. Stop enabling
tracing'slogfeature everywherefeatures = ["log"]is removed from all 35 manifests (library crates, applications,benches,ffi).ironrdp-dvc,ironrdp-rdpecamandironrdp-rdpelareno_stdcrates, so theirtracingdependency also disables default features.ironrdp-dvcforwardstracing/stdfrom its existingstdfeature.3. Check no_std builds in CI (xtask feature matrix)
workspace/no-std-target:cargo check --no-default-features --target x86_64-unknown-noneover the crates that are expected to beno_std.cargo xtask check installnow adds that target, so the CI fan-out picks the case up with no workflow change.workspace/powerset-multitransport: rdpeudp and rdpemt were not in any powerset group.Why
tracing's defaultstdfeature has been pulled into the rdpeudp/rdpemt no-default-features builds. Both fail on a real no_std target (can't find crate for std). The existing feature checks only run on the host, wherestdis always present, so CI couldn't catch this. The new case fails if the rdpeudp manifest fix is reverted.tracing/log: it entered the workspace dependency table incidentally in feat(rdpdr):DR_CORE_SERVER_CLIENTID_CONFIRMandDR_CORE_DEVICELIST_ANNOUNCE#193 (2023) and was then copied into every crate when workspace dependencies were dropped (build: do not use workspace dependencies #695). A library shouldn't turn on log interoperability for all of its consumers. No application in the workspace needs it either: the viewer, FFI, web client, daemon (which ActiveX goes through), testsuite and benches all install atracing-subscribersubscriber. Applications that do want to forward tracing events to alogbackend can enabletracing/logthemselves.Notes for reviewers
tracingno longer depends onlogin either lock file, andlogdrops out offuzz/Cargo.lockentirely.ironrdp-mstsgustill depends onlogdirectly, which this PR doesn't touch.ironrdp-pduand the crates built on it (svc, dvc, graphics, rdpecam, rdpel, rdpeusb) still fail the no-std-target check, because pdu's dependency tree enablesstd. That's a separate issue, tracked as a FIXME next to the case.tracing's default features also drops itsattributesfeature. None of the affected crates uses#[instrument].Validation
cargo xtask check fmt,lints,tests,lockscargo xtask wasm checkcargo xtask check features --case workspace/no-std-targetcargo xtask check features --case workspace/powerset-multitransport(12/12)🤖 Generated with Claude Code