Apply CMake best practices and remove stale references by bmehta001 · Pull Request #1416 · microsoft/cpp_client_telemetry

bmehta001 · 2026-03-18T08:34:30Z

Pure build/project cleanup

CMake improvements (3 files)

Cleanup / CMake hygiene

add LANGUAGES C CXX to the top-level project() call
switch build/config logging to message(STATUS ...) instead of bare message(...)
Fix message(FATAL_ERROR, ...) comma bug (invalid CMake syntax)
fix if(EXISTS ...) quoting in lib/, tests/functests/, and tests/unittests
simplify cross-version file copies to configure_file(... COPYONLY)
remove stale Visual Studio project/header references
update VS toolset fallback logic in Solutions/before.targets and Solutions/build.MIP.props
Update MATSDK_API_VERSION from 3.4 to 3.10 to match recent releases
Remove empty compiler-id blocks (Clang/Intel/MSVC with no content); GCC -fPIC flags preserved
Remove commented-out bondlite test block and stale TODO comments
Remove duplicate include_directories entry

Visual Studio project cleanup (8 files)

Remove stale header references that no longer exist in the repo:
- targetver.h (SampleCpp, SampleCppMini)
- LogManagerV1.hpp (Shared.vcxitems)
- MockILocalStorageReader.hpp (UnitTests.vcxproj)
- SanitizerBaseTests.cpp (FuncTests.vcxproj)
Modernize VS toolset: use `` fallback instead of hardcoded v141 (Solutions/before.targets, build.MIP.props)

Packaging

keep the package install-dir overrides that Debian/RPM packaging expects
adjust packaging messages / cleanup so the CMake changes stay behavior-correct

lalitb

see comments.

- Modernize CMakeLists.txt: flatten nesting, deduplicate, use consistent quoting and variable patterns - Remove stale header references from vcxproj and vcxitems files - Simplify test CMakeLists.txt files - Fix CMake conventions in packaging scripts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Build/project cleanup focused on improving CMake hygiene and removing stale Visual Studio project references, while modernizing toolset selection for newer VS installs.

Changes:

Standardize CMake logging to message(STATUS ...), improve if(EXISTS ...) quoting, and simplify test-data copying via configure_file(... COPYONLY).
Update SDK build version prefixing (MATSDK_API_VERSION to 3.10) and remove stale/commented build logic.
Remove references to deleted headers/sources from multiple .vcxproj / .vcxitems files and adjust MSBuild toolset selection defaults.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
CMakeLists.txt	CMake best-practices cleanup (languages, STATUS messages), version bump, removes stale blocks/comments
lib/CMakeLists.txt	Consistent STATUS messages, quoted `EXISTS` checks, fix `message(FATAL_ERROR ...)` syntax
tools/ParseOsRelease.cmake	Quote `EXISTS` path; switch to `message(STATUS ...)`
tools/MakeTgz.cmake	Switch packaging output to `message(STATUS ...)`
tools/MakeDeb.cmake	Switch packaging output to `message(STATUS ...)`
tools/MakeRpm.cmake	Switch packaging output to `message(STATUS ...)`
tests/unittests/CMakeLists.txt	STATUS messages + quoting in `EXISTS` checks for optional test sources
tests/functests/CMakeLists.txt	STATUS messages, quoting in `EXISTS` checks, simplify test.json copy logic
tests/unittests/UnitTests.vcxproj	Remove stale project item reference to a non-existent test file
tests/functests/FuncTests.vcxproj	Remove stale header reference from project items
tests/functests/FuncTests.vcxproj.filters	Remove stale header reference from filters list
lib/shared/Shared.vcxitems	Remove stale header include from shared items
lib/shared/Shared.vcxitems.filters	Remove stale header include from shared items filters
examples/cpp/SampleCpp/SampleCpp.vcxproj	Remove stale `targetver.h` reference
examples/cpp/SampleCpp/SampleCpp.vcxproj.filters	Remove stale `targetver.h` reference from filters
examples/cpp/SampleCppMini/SampleCppMini.vcxproj	Remove stale `targetver.h` reference
examples/cpp/SampleCppMini/SampleCppMini.vcxproj.filters	Remove stale `targetver.h` reference from filters
Solutions/build.MIP.props	Switch toolset selection to default toolset variable
Solutions/before.targets	Update toolset fallback behavior to prefer default toolset variable

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Keep the before.targets toolset selection deterministic on newer Visual Studio hosts, but only set the MIP props fallback when a consumer has not already chosen a toolset. While addressing the MSBuild review comments, also point the optional module test conditions and source paths at the real lib/modules locations and use CPACK_PACKAGE_FILE_NAME for the RPM status message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Quote the optional ECS test.json configure_file input and output paths so CMake handles source or build directories that contain spaces. Files changed: - tests/functests/CMakeLists.txt Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

ThomsonTan · 2026-05-14T18:00:09Z


 if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
  # Using GCC with -s and -Wl linker flags
-  set(REL_FLAGS "-s -Wl,--gc-sections -Os ${WARN_FLAGS} -ffunction-sections -fdata-sections -fmerge-all-constants -ffast-math -fno-finite-math-only")


Why is -fno-finite-math-only removed? It was added in #1415 to keep std::isnan/std::isinf honest for nlohmann::json and to fix a SQLite IEEE 754 issue. This PR drops it from all three Unix REL_FLAGS branches and adds -Wno-nan-infinity-disabled to silence the warning that flags the regression. Please restore the flag and remove the suppression.

PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing -fno-finite-math-only from the three Unix REL_FLAGS branches and silencing the resulting Clang warning via -Wno-nan-infinity-disabled. That left -ffast-math implying -ffinite-math-only on Clang/AppleClang, which folds std::isnan / std::isinf to false in nlohmann::json (lib/include/mat/json.hpp) and breaks the IEEE 754 invariants that SQLite relies on for sqlite3_bind_double / sqlite3IsNaN. Rather than restore -fno-finite-math-only, address the root cause by removing -ffast-math entirely from all three Unix REL_FLAGS branches (GCC, AppleClang, generic Clang). For a telemetry library: - The hot paths are string concatenation, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes - there is essentially no floating-point arithmetic for -ffast-math to optimize. - Defining __FAST_MATH__ causes some libc++ / glibc headers to shortcut isnan / isinf even with -fno-finite-math-only set, silently corrupting NaN handling in nlohmann::json output. - On x86 Linux, -ffast-math links crtfastmath.o which sets MXCSR FTZ/DAZ bits process-wide at startup, leaking flush-to-zero behavior into client applications that link this SDK - a contract violation for any consumer doing scientific compute or audio DSP. - SQLite source explicitly recommends against -ffast-math. This subsumes the partial mitigation from microsoft#1415 (-fno-finite-math-only is no longer needed once -ffast-math itself is gone) and aligns with the intent of the long-standing draft PR microsoft#1392 by ThomsonTan, extended to the GCC branch as well (GCC's -ffast-math has the same finite-math-only and crtfastmath.o behavior on x86 Linux). Also drop -Wno-nan-infinity-disabled from the Clang WARN_FLAGS - it only existed to silence the warning that flagged this exact bug, and is unnecessary once -ffast-math is no longer pulling in -ffinite-math-only. Validation: - cmake configure + Release build of UnitTests on macOS arm64 (AppleClang 21): no warnings, no -Wnan-infinity-disabled hits. - ./out/tests/unittests/UnitTests: 486/486 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PR microsoft#1416 inadvertently regressed PR microsoft#1415 by removing -fno-finite-math-only from the three Unix REL_FLAGS branches and adding -Wno-nan-infinity-disabled to silence the Clang diagnostic. That left release builds using -ffast-math without preserving the NaN/Inf semantics needed by nlohmann::json and SQLite paths. Remove -ffast-math entirely from the GCC, AppleClang, and generic Clang release flags rather than relying on -fno-finite-math-only to partially undo it. This SDK is not floating-point compute-bound; its hot paths are string, Bond/JSON serialization, HTTP I/O, and SQLite reads/writes. Avoiding -ffast-math: - preserves std::isnan/std::isinf behavior for JSON and storage code, - avoids compiler/runtime fast-math side effects such as x86 GCC's crtfastmath.o changing MXCSR FTZ/DAZ behavior process-wide, and - aligns with SQLite's guidance to avoid fast-math. This subsumes microsoft#1415's partial mitigation and aligns with microsoft#1392's intent, extended to GCC because GCC fast-math has the same broad assumptions and runtime side effects. Also remove -Wno-nan-infinity-disabled because the warning should not be suppressed once the cause is gone. Validation: - CMake Release build of UnitTests on macOS arm64 (AppleClang 21). - UnitTests passed on macOS arm64. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bmehta001 requested a review from a team as a code owner March 18, 2026 08:34

bmehta001 force-pushed the bhamehta/code-cleanup branch 2 times, most recently from 34e4948 to 790fdd0 Compare March 18, 2026 09:03

bmehta001 changed the title ~~Use best CMake practices, rm outdated headers, and update VS settings~~ Use best CMake practices, rm/update outdated code Mar 18, 2026

bmehta001 force-pushed the bhamehta/code-cleanup branch 18 times, most recently from 9a5a166 to 2feeb94 Compare March 19, 2026 05:40

bmehta001 self-assigned this Mar 19, 2026