Add MATSDK_HAVE_CS4 CMake option with public propagation to consumers#1476
Closed
bmehta001 wants to merge 2 commits into
Closed
Add MATSDK_HAVE_CS4 CMake option with public propagation to consumers#1476bmehta001 wants to merge 2 commits into
bmehta001 wants to merge 2 commits into
Conversation
HAVE_CS4 (and HAVE_CS4_FULL) toggle optional CsProtocol fields whose presence changes the public struct layout, so the define must be consistent across the SDK and any consumer that includes the CS-protocol / IDecorator headers. Previously CS4 could only be enabled via a raw -DCMAKE_CXX_FLAGS=-DHAVE_CS4=1 or a custom config header, which defines it for the SDK build only -- a consumer that omits it gets a mismatched layout. Add first-class, default-off options MATSDK_HAVE_CS4 / MATSDK_HAVE_CS4_FULL that apply HAVE_CS4 as a PUBLIC compile definition on the exported MSTelemetry::mat target, so consumers inherit it automatically. Default off => existing builds are unchanged. Validated on x64-windows: -DMATSDK_HAVE_CS4=ON configures, the library builds with HAVE_CS4, and the installed MSTelemetryTargets.cmake carries INTERFACE_COMPILE_DEFINITIONS "HAVE_CS4". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes Common Schema 4.0 (CS4) support a first-class CMake configuration by introducing MATSDK_HAVE_CS4 / MATSDK_HAVE_CS4_FULL options and propagating the corresponding HAVE_CS4* preprocessor defines to consumers via the exported MSTelemetry::mat target—preventing SDK/consumer ABI mismatches due to CS4-dependent public struct layout changes.
Changes:
- Add default-off CMake options
MATSDK_HAVE_CS4andMATSDK_HAVE_CS4_FULL. - Propagate
HAVE_CS4/HAVE_CS4_FULLas PUBLIC compile definitions on themattarget when enabled. - Update
building-custom-SKU.mdto document enabling CS4 via the new CMake options.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
CMakeLists.txt |
Adds new CMake options to control CS4/CS4_FULL builds. |
lib/CMakeLists.txt |
Applies CS4-related compile definitions publicly on the exported mat target for consumer propagation. |
docs/building-custom-SKU.md |
Documents how to enable CS4/CS4_FULL with the new CMake options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CMakeLists.txt (option independence): MATSDK_HAVE_CS4_FULL now forces MATSDK_HAVE_CS4 ON (cache FORCE) so the cache cannot be left FULL=ON/CS4=OFF; lib/CMakeLists.txt simplified to check MATSDK_HAVE_CS4. Verified by configuring with only -DMATSDK_HAVE_CS4_FULL=ON: it enables CS4 and defines both HAVE_CS4 and HAVE_CS4_FULL. docs/building-custom-SKU.md: reframe HAVE_CS4 / HAVE_CS4_FULL as preprocessor defines valid for any build system (CMake option is the CMake-preferred way), so MSBuild/other users are not misled into thinking the defines are invalid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Makes Common Schema 4.0 (CS4) a first-class, propagating build option instead of a raw compiler flag.
Problem
HAVE_CS4/HAVE_CS4_FULLtoggle optionalCsProtocolfields whose presence changes the public struct layout (29#ifdef HAVE_CS4blocks inCsProtocol_types.hpp). Today CS4 can only be enabled via a raw-DCMAKE_CXX_FLAGS=-DHAVE_CS4=1or a custom config header — both define it for the SDK build only. A consumer that includes the CS-protocol /IDecoratorheaders without the same define gets a mismatched layout (silent ODR/ABI hazard), and there's no first-class CMake toggle.Change
MATSDK_HAVE_CS4andMATSDK_HAVE_CS4_FULL(the latter implies the former).HAVE_CS4is applied as aPUBLICcompile definition on the exportedMSTelemetry::mattarget, so consumers inherit it automatically viaINTERFACE_COMPILE_DEFINITIONS— keeping the SDK and its consumers in sync.building-custom-SKU.md).Default off ⇒ existing builds are unchanged.
Validation (x64-windows)
cmake -DMATSDK_HAVE_CS4=ON …configures cleanly.-DHAVE_CS4(CS4 code path compiles).MSTelemetryTargets.cmakecarriesINTERFACE_COMPILE_DEFINITIONS "HAVE_CS4"— confirming propagation to consumers.Follow-ups (not in this PR)
-PHAVE_CS4=1Gradle property mapping to this option (instead of the genericCXXFLAGS).cs4feature on thecpp-client-telemetryvcpkg port.Note for schema/collector owners: with this change, consumers linking a CS4-built
MSTelemetry::matnow inheritHAVE_CS4automatically.