-
Notifications
You must be signed in to change notification settings - Fork 44
Fix cargo test on Windows
#696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cargo test on Windows
#696
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes cargo test on Windows by avoiding desktop-incompatible dependencies from being pulled into workspace test builds.
Changes:
- Remove
defmtfrompartition-managerdefault features to prevent unintended desktop linking requirements. - Gate
espi-servicemodules/exports behind#[cfg(not(test))]so workspace tests can compile on Windows. - Gate
debug-servicemodules/exports behind#[cfg(not(test))]for the same reason, and adjust a few log/trace call sites inespi-service.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| partition-manager/partition-manager/Cargo.toml | Drops defmt from default features to avoid pulling it into desktop test builds. |
| espi-service/src/lib.rs | Disables the crate’s modules/exports during tests to keep Windows test builds linkable. |
| espi-service/src/espi_service.rs | Tweaks logging guards and error logging in the eSPI service implementation. |
| debug-service/src/lib.rs | Disables the crate’s modules/exports during tests to keep Windows test builds linkable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cargo testcurrently doesn't compile on Windows. It looks like this is because a couple crates have hard dependencies on libraries that don't work on desktop - in particular:These don't build on desktop, so when you try to run
cargo testagainst mocks, the build fails.We get away with it on Linux because it looks like the linker over there is more aggressive at pruning unused symbols than the MSVC linker, but the MSVC linker errors immediately when it can't find a referenced symbol, even if that symbol is not reachable from any entry points.
This change mitigates this problem by making partition-manager not default-depend on defmt and disabling build of debug-service and espi-service in test contexts. This does unfortunately mean that we can't write tests in those modules, but those modules already didn't have tests, so we're not conceding any existing test collateral by doing this.
In future, we can look into breaking the dependency espi-service has on embassy-imxrt by introducing traits. It's less clear to me how we would do this with the debug-service - perhaps a stub implementation of some of the defmt macros.
Fixes #691