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.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/unittests/CMakeLists.txt:84

The appended privacyguard test source paths are unquoted. If ${CMAKE_SOURCE_DIR} contains spaces, these will be split into multiple arguments and break the build. Quote each ${CMAKE_SOURCE_DIR}/... path in this list(APPEND SRCS ...) block.

if(EXISTS "${CMAKE_SOURCE_DIR}/lib/modules/privacyguard/" AND BUILD_PRIVACYGUARD)
  add_definitions(-DHAVE_MAT_PRIVACYGUARD)
  list(APPEND SRCS
  ${CMAKE_SOURCE_DIR}/lib/modules/privacyguard/tests/unittests/InitializationConfigurationTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/privacyguard/tests/unittests/PrivacyConcernEventTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/privacyguard/tests/unittests/PrivacyConcernMetadataProviderTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/privacyguard/tests/unittests/PrivacyGuardTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/privacyguard/tests/unittests/SystematicSamplerTests.cpp

tests/unittests/CMakeLists.txt:96

The sanitizer test source paths added here are unquoted. If ${CMAKE_SOURCE_DIR} contains spaces, the paths can be mis-parsed as multiple arguments. Quote each ${CMAKE_SOURCE_DIR}/... entry when appending to SRCS.

if(EXISTS "${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/" AND BUILD_SANITIZER)
  list(APPEND SRCS
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SanitizerJwtTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SanitizerProviderTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SanitizerSitePathTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SanitizerStringUtilsTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SanitizerUrlTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SanitizerTrieTests.cpp
  ${CMAKE_SOURCE_DIR}/lib/modules/sanitizer/tests/unittests/SPOPasswordTests.cpp

Copilot review on PR microsoft#1416 flagged 7 unquoted ${CMAKE_SOURCE_DIR}/... paths inside list(APPEND SRCS ...) calls. CMake splits unquoted arguments on whitespace, so a checkout path containing spaces (very common on Windows: C:\Users\First Last\source\...) would fragment a single source path into multiple list elements and the test wouldn't compile. Quote every ${CMAKE_SOURCE_DIR}/.../*.cpp path inside list(APPEND SRCS ...) in both tests/unittests/CMakeLists.txt and tests/functests/CMakeLists.txt (23 paths total, including the privacyguard + sanitizer blocks Copilot didn't cite but that share the same hazard). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bmehta001 · 2026-05-28T17:35:31Z

Addressed in 1de44ae: quoted all 23 ${CMAKE_SOURCE_DIR}/... paths inside list(APPEND SRCS ...) blocks across both ests/unittests/CMakeLists.txt and ests/functests/CMakeLists.txt (including the privacyguard + sanitizer blocks not explicitly flagged but sharing the same hazard). Re-requesting Copilot review.

Copilot

Pull request overview

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

Copilot review on PR microsoft#1416 flagged that mapping VisualStudioVersion >= 18.0 to PlatformToolset v145 is a guess: VS 2025/2026/etc. has not shipped, the toolset moniker for that release may differ, and pinning to a non-existent toolset name causes MSBuild to fail early with "unknown PlatformToolset" on any machine that does pick up that VS version. Drop the v145 mapping from both before.targets and build.MIP.props. Future VS versions fall through to the v141 fallback (same behavior as before PR microsoft#1416, just one less wrong-toolset-name failure mode). Add explicit mappings for new toolsets as they actually ship. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bmehta001 · 2026-05-28T17:48:09Z

Addressed Copilot round 2: dropped the speculative v145 PlatformToolset mapping from Solutions/before.targets + Solutions/build.MIP.props (337028a). Replied on the -ffast-math thread explaining the removal was intentional (commit 704d29b, SDK isn't FP-heavy and -ffast-math brings IEEE-754 reassociation hazards). All 10 Copilot threads on this PR are now resolved.

Copilot

Pull request overview

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

The top-level CMakeLists.txt asks for C++14 (CMAKE_CXX_STANDARD 14, set in 2018) but then re-injects -std=c++11 into CMAKE_CXX_FLAGS for both Release and Debug Unix builds (set in 2019 when gcc-4.x / gcc-5.x C++14 support was still spotty). That explicit flag wins, so the Unix builds have actually been C++11 ever since, contradicting the stated standard. This is a vestige: gcc-5+, clang-3.4+, and MSVC 2015+ all support C++14 fully, and Android (lib/android_build/) already sets CMAKE_CXX_STANDARD 14 explicitly. Removing the override aligns the Unix CMake builds with the intended standard. - Drop the stale gcc-4.x/5.x caveat comment and add CMAKE_CXX_STANDARD_REQUIRED ON so the build fails loudly on toolchains that cannot do C++14, rather than silently falling back. - Drop the -std=c++11 fragment from both REL and DBG CMAKE_CXX_FLAGS lines. C and C11 are left alone (C11 remains the C floor). Not touched in this commit: - examples/**/CMakeLists.txt and wrappers/obj-c/CMakeLists.txt also pin -std=c++11. Those are standalone sample/wrapper build scripts not wired via add_subdirectory and not built by any CI workflow; their pins are intentional statements of each sample's minimum and can stay. - This commit does not bump the project to C++17. Cleanup ideas like cpp_client_telemetry_modules#302 (std::string::data() -> char*) require C++17 and are still out of scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bmehta001 · 2026-05-28T21:17:22Z

Added commit c4b3c98f that drops the vestigial -std=c++11 from CMAKE_CXX_FLAGS in both Release and Debug Unix branches of the top-level CMakeLists.txt.

Why it fits this PR's "remove stale references" theme:

L20 has set CMAKE_CXX_STANDARD 14 since 2018, but L152/L158 since 2019 re-inject -std=c++11 into CMAKE_CXX_FLAGS. The explicit flag wins, so Unix builds have actually been C++11 ever since — directly contradicting the stated standard.
Modules' Android side already does it the right way: lib/android_build/{app,maesdk}/src/main/cpp/CMakeLists.txt:7-8 set CMAKE_CXX_STANDARD 14 + STANDARD_REQUIRED ON and the Android build genuinely is C++14.
MSVC defaults to C++14 in VS 2015+, so MSVC builds are fine.
Only the Unix gcc/clang/AppleClang paths were being silently downgraded.

Toolchain risk: gcc-5+, clang-3.4+, and MSVC 2015+ all fully support C++14. The original comment ("…we may get C++11 on older gcc-4.x and gcc-5.x") is a 7-year-old caveat that no realistic build environment hits today. I also added CMAKE_CXX_STANDARD_REQUIRED ON so that if anyone does try with a too-old toolchain, they get a hard configure-time error instead of a silent fallback.

Out of scope (intentional):

examples/**/CMakeLists.txt and wrappers/obj-c/CMakeLists.txt also pin -std=c++11 but are standalone sample/wrapper scripts (not in add_subdirectory, not in any CI workflow). Their pins are intentional per-sample minimums.
This does not bump to C++17. Things like the string::data() -> char* cleanup in cpp_client_telemetry_modules#302 still require C++17 and remain out of scope.

PR description's "What changes" list updated to mention this; no other text changes needed.

Copilot

Pull request overview

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

Derive package version components from MATSDK_BUILD_VERSION so CPack packages stay aligned with the SDK version. Use PROJECT_SOURCE_DIR and PROJECT_BINARY_DIR for test CMake paths so the SDK remains subproject-friendly. Files changed: - CMakeLists.txt - tools/MakeDeb.cmake - tools/MakeRpm.cmake - tools/MakeTgz.cmake - tests/functests/CMakeLists.txt - tests/unittests/CMakeLists.txt 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 4 comments.

Validate MATSDK_BUILD_VERSION before deriving CPack version components and remove now-dead DAYNUMBER calculations from package scripts. Files changed: - CMakeLists.txt - tools/MakeDeb.cmake - tools/MakeRpm.cmake - tools/MakeTgz.cmake 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 no new comments.

# Conflicts: # lib/CMakeLists.txt

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

The previous merge from main left a conflict marker block in lib/CMakeLists.txt's trailing commented-link section. Remove the marker block and keep the cleaned-up file content. Files changed: - lib/CMakeLists.txt 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 no new comments.

Keep package script version cleanup out of the CMake cleanup PR; package version alignment can be handled separately before the next release. Files changed: - CMakeLists.txt - tools/MakeDeb.cmake - tools/MakeRpm.cmake - tools/MakeTgz.cmake 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

lalitb reviewed Mar 21, 2026

View reviewed changes

Comment thread lib/CMakeLists.txt Outdated

lalitb reviewed Mar 21, 2026

View reviewed changes

Comment thread lib/CMakeLists.txt Outdated

lalitb reviewed Mar 21, 2026

View reviewed changes

Comment thread lib/CMakeLists.txt Outdated

lalitb reviewed Mar 21, 2026

View reviewed changes

Comment thread lib/CMakeLists.txt Outdated

lalitb reviewed Mar 21, 2026

View reviewed changes

Comment thread lib/CMakeLists.txt Outdated

lalitb requested changes Mar 21, 2026

View reviewed changes

ThomsonTan reviewed Mar 21, 2026

View reviewed changes

Comment thread lib/http/HttpClient_Apple.mm Outdated

Copilot AI reviewed May 28, 2026

View reviewed changes

bmehta001 requested a review from Copilot May 28, 2026 17:35

Copilot started reviewing on behalf of bmehta001 May 28, 2026 17:35 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

Comment thread CMakeLists.txt

Comment thread Solutions/before.targets

Comment thread Solutions/build.MIP.props

bmehta001 requested a review from Copilot May 28, 2026 17:48

Copilot started reviewing on behalf of bmehta001 May 28, 2026 17:48 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

bmehta001 requested a review from Copilot May 28, 2026 21:17

Copilot started reviewing on behalf of bmehta001 May 28, 2026 21:17 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

Comment thread CMakeLists.txt

Comment thread tests/functests/CMakeLists.txt Outdated

Comment thread tests/unittests/CMakeLists.txt

bmehta001 and others added 2 commits May 29, 2026 11:42

Merge branch 'main' into bhamehta/code-cleanup

d2e2bd1

bmehta001 requested a review from Copilot May 29, 2026 20:40

Copilot started reviewing on behalf of bmehta001 May 29, 2026 20:41 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

Comment thread CMakeLists.txt Outdated

Comment thread tools/MakeDeb.cmake

Comment thread tools/MakeRpm.cmake

Comment thread tools/MakeTgz.cmake

bmehta001 requested a review from Copilot May 29, 2026 20:52

Copilot started reviewing on behalf of bmehta001 May 29, 2026 20:52 View session

Copilot AI reviewed May 29, 2026

View reviewed changes

Merge remote-tracking branch 'msft/main' into bhamehta/code-cleanup

248d53f

# Conflicts: # lib/CMakeLists.txt

Copilot AI reviewed May 29, 2026

View reviewed changes

Comment thread lib/CMakeLists.txt Outdated

Copilot AI reviewed May 29, 2026

View reviewed changes

Conversation

bmehta001 commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CMake improvements (3 files)

Visual Studio project cleanup (8 files)

Packaging

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmehta001 commented May 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmehta001 commented May 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

bmehta001 commented May 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bmehta001 commented Mar 18, 2026 •

edited

Loading