fix(odrive_native): repair the interop gate broken by the golden's espp-lib section - #727
Merged
Merged
Conversation
…pp-lib section The lib_wrapper_roundtrip section added to pc/tests/odrive_native_golden.cpp includes odrive_native.hpp (the espp::OdriveNative wrapper), but the interop harness builds that file with only c++ -std=c++20 -I components/odrive_native/include so base_component.hpp could not resolve and the gate failed at the build step (and #721 was merged with the last rerun cancelled, so main's gate is red). Guard the wrapper section with __has_include("base_component.hpp"): the pc-package build (full espp::espp) still compiles and runs it, while the minimal harness build compiles it out and prints an explicit skip line. Verified: the exact minimal CI compile now builds + ALL PASSED (wrapper skipped), the full-lib build runs the wrapper section (ALL PASSED), and the complete interop harness passes 7/7 in an ubuntu:24.04 container. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Repairs the odrive_native interop gate build by conditionally compiling the espp::OdriveNative wrapper test section only when the full espp headers are available, avoiding failures in the minimal interop harness compile.
Changes:
- Added a
__has_include("base_component.hpp")preprocessor guard to gate inclusion ofodrive_native.hpp. - Wrapped
lib_wrapper_roundtrip()(and its invocation) behind the same macro and added an explicit “skipped” print in minimal builds. - Updated file header comments to document the two build modes and why the wrapper section is optional.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…system Per PR review: ODRIVE_GOLDEN_HAS_ESPP_LIB is now ALWAYS defined to 0 or 1 (-Wundef-clean), the pc harness defines it to 1 explicitly (it links the whole espp::espp archive, so headers AND the compiled Logger are guaranteed -- closing the headers-present-but-not-linked gap), and __has_include remains only as the fallback for bare compiles with no build-system signal. Verified: minimal harness compile with -Werror=undef (wrapper skipped), explicit -D...=0 with headers present (skipped, no link error), header fallback with Logger linked (wrapper runs), and the pc harness build against the installed package (wrapper runs). ALL PASSED in every mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#721's final interop run failed at the build step and the rerun was cancelled by the merge, so main's odrive_native interop gate is currently red. Root cause: the
lib_wrapper_roundtripsection added topc/tests/odrive_native_golden.cppincludesodrive_native.hpp(theespp::OdriveNativewrapper), but the interop harness compiles that file with onlyc++ -std=c++20 -I components/odrive_native/include— no espp lib on the include/link path, sobase_component.hppcannot resolve:Fix: guard the wrapper section with
__has_include("base_component.hpp"). The pc-package build (fullespp::espp, which links the compiledLoggerthe wrapper needs) still compiles and runs it; the minimal interop-harness build compiles it out and prints an explicitlib_wrapper_roundtrip skippedline instead of silently shrinking coverage.Testing
c++ -std=c++20 -I components/odrive_native/include) now builds and printsALL PASSEDwith the wrapper section skipped.logger.cpp) runs the wrapper section:ALL PASSED.ubuntu:24.04container — the same environment as the CI gate, which also runs on this PR via itspc/tests/odrive_native_*path trigger.🤖 Generated with Claude Code