Fix missing propagation of defines during build time#4245
Open
atobiszei wants to merge 4 commits into
Open
Conversation
…E_DISABLE Replace per-target copts/local_defines with transitive defines propagated via ovms_cc_library macro. All dependents automatically get correct preprocessor state without per-target configuration. - Add COMMON_DEFINES (select-based) to ovms_cc_library defines field - Remove COPTS_PYTHON, COPTS_MEDIAPIPE from src/BUILD - Clean up LOCAL_DEFINES_TESTS to only contain SPDLOG - Move rerank_api_handler and embeddings_handler_tests into mediapipe select - Remove additional_local_defines from src/llm/BUILD, src/mediapipe_internal/BUILD, src/graph_export/BUILD
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes preprocessor macro propagation in the Bazel build. PYTHON_DISABLE and MEDIAPIPE_DISABLE were previously injected via per-target additional_copts (COPTS_PYTHON, COPTS_MEDIAPIPE), which are non-transitive — dependents that included headers using those #if guards (e.g. servable.hpp, modelmanager.hpp, logging.hpp, http_graph_executor_impl.hpp) could see different macro values than the library that exported them. The fix moves the macros into the Bazel defines attribute on ovms_cc_library, making them transitive to all dependents.
Changes:
- Add
COMMON_DEFINES(built from newLOCAL_DEFINES_PYTHON/LOCAL_DEFINES_MEDIAPIPEselects) and haveovms_cc_librarydefaultdefines = COMMON_DEFINES; addadditional_local_definesplumbing. - Remove
COPTS_PYTHON/COPTS_MEDIAPIPEand alladditional_copts = COPTS_PYTHON + COPTS_MEDIAPIPEusages fromsrc/BUILD,src/llm/BUILD,src/mediapipe_internal/BUILD, andsrc/graph_export/BUILD. - In
src/BUILD, introduceLOCAL_DEFINES_TESTSand apply it consistently to all nativecc_library/cc_testtest helpers; also movererank_api_handlerandembeddings_handler_testsinto thenot_disable_mediapipebranch ofovms_testsrcs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| common_settings.bzl | Replaces non-transitive COPTS_PYTHON/COPTS_MEDIAPIPE with LOCAL_DEFINES_PYTHON/LOCAL_DEFINES_MEDIAPIPE, exposes them via COMMON_DEFINES, and updates ovms_cc_library to propagate them through defines. |
| src/BUILD | Drops COPTS_PYTHON/COPTS_MEDIAPIPE usages, defines LOCAL_DEFINES_TESTS, and applies it to all test libraries; reorganizes mediapipe-only test deps in ovms_test. |
| src/llm/BUILD | Removes COPTS_PYTHON import and all additional_copts = COPTS_PYTHON lines now that the macro is transitive. |
| src/mediapipe_internal/BUILD | Removes COPTS_PYTHON/COPTS_MEDIAPIPE import and per-target additions. |
| src/graph_export/BUILD | Removes COPTS_MEDIAPIPE import and additional copts. |
atobiszei
force-pushed
the
atobisze_build_fixes
branch
from
73e5214 to
633489b
Compare
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.
PYTHON_DISABLE and MEDIAPIPE_DISABLE preprocessor macros were not reliably propagated to all translation units that needed them. The previous approach used copts (-DPYTHON_DISABLE=1) or local_defines on individual targets in BUILD files. Both mechanisms are non-transitive in Bazel — they only apply to the target they're defined on, not to its dependents, so if header used them, and other bazel target included this header but by itself did not rely on those ifdefs it was missing them.
JIRA:CVS-187417