Skip to content

fix(shm): fail loudly if CARGO_CFG_TARGET_OS is unset - #2683

Merged
fuzzypixelz merged 1 commit into
eclipse-zenoh:mainfrom
ZettaScaleLabs:fix/cfg-target-os-expect
Jul 16, 2026
Merged

fix(shm): fail loudly if CARGO_CFG_TARGET_OS is unset#2683
fuzzypixelz merged 1 commit into
eclipse-zenoh:mainfrom
ZettaScaleLabs:fix/cfg-target-os-expect

Conversation

@YuanYuYuan

@YuanYuYuan YuanYuYuan commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #2680. That PR replaced commons/zenoh-shm/build.rs's cfg_aliases! macro with a direct CARGO_CFG_TARGET_OS check, using unwrap_or_default() on the env var lookup. Review feedback pointed out that CARGO_CFG_TARGET_OS is always set by Cargo when it runs a build script normally, so silently falling back to an empty string on a missing/unexpected value would misconfigure shm_external_lockfile as false with no diagnostic — the wrong failure mode for a build-time invariant that should never actually be violated under normal use.

Key Changes

  • commons/zenoh-shm/build.rs: replace .unwrap_or_default() with .expect(...) on the CARGO_CFG_TARGET_OS lookup, so a nonstandard invocation fails loudly at build time instead of silently misconfiguring BSD/Redox platform detection.

🏷️ Label-Based Checklist

Based on the labels applied to this PR, please complete these additional requirements:

Labels: bug

🐛 Bug Fix Requirements

Since this PR is labeled as a bug fix, please ensure:

  • Root cause documented - see "Summary" above: unwrap_or_default() on CARGO_CFG_TARGET_OS silently computes shm_external_lockfile = false if the var is ever missing, instead of surfacing the problem.
  • Reproduction test added - no unit test is meaningful for a build-script env-var-presence assumption; the failure mode this guards against (silent misconfiguration on a genuinely-missing CARGO_CFG_TARGET_OS) has no reliable way to reproduce under normal cargo build, since Cargo always sets it. The fix is a defensive assertion, not a fix for an observed failure.
  • Test passes with fix - verified via this PR's own CI run (cargo build/cargo test across the matrix exercise the build script on every job; a passing build confirms expect() doesn't fire under normal Cargo invocation).
  • Regression prevention - any future code path that invokes this build script without Cargo's standard environment will now panic immediately with a clear message, rather than silently producing a wrong cfg.
  • Fix is minimal - one-line change (.unwrap_or_default().expect(...)) in commons/zenoh-shm/build.rs; no other files touched.
  • Related bugs checked - grep -rn "unwrap_or_default\|unwrap_or(" commons/zenoh-shm/build.rs confirms this was the only such fallback in the file; no other silent-default pattern exists there.

Why this matters: Bugs without tests often reoccur.

Instructions:

  1. Check off items as you complete them (change - [ ] to - [x])
  2. The PR checklist CI will verify these are completed

This checklist updates automatically when labels change, but preserves your checked boxes.

unwrap_or_default() would silently compute shm_external_lockfile as
false on a missing var, misconfiguring BSD/Redox builds with no
diagnostic. CARGO_CFG_TARGET_OS is always set by Cargo when running
build scripts normally; expect() surfaces a nonstandard invocation
immediately instead of masking it.
@YuanYuYuan YuanYuYuan added the bug Something isn't working label Jul 16, 2026
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.31%. Comparing base (7583f9a) to head (af4a3eb).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2683   +/-   ##
=======================================
  Coverage   74.31%   74.31%           
=======================================
  Files         414      414           
  Lines       62301    62301           
=======================================
+ Hits        46299    46300    +1     
+ Misses      16002    16001    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@fuzzypixelz fuzzypixelz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fuzzypixelz
fuzzypixelz merged commit c730adc into eclipse-zenoh:main Jul 16, 2026
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants