Skip to content

Standardize packaged telemetry dependencies - #1537

Closed
bmehta001 wants to merge 18 commits into
microsoft:mainfrom
bmehta001:bhamehta/standardize-telemetry-dependencies
Closed

bmehta001 wants to merge 18 commits into
microsoft:mainfrom
bmehta001:bhamehta/standardize-telemetry-dependencies

Conversation

@bmehta001

Copy link
Copy Markdown
Contributor

Summary

  • centralize pthread-safe mbedTLS configuration in the SDK-owned fetched curl path
  • document one dependency policy for Apple, Linux, Windows, and Android consumers
  • document host-owned SQLite lifecycle requirements when multiple embedded SDK copies share system SQLite

Validation

  • VS 2026 Release build with minimal SQLite and vendored zlib
  • WSL Release build with minimal SQLite, vendored zlib, and SDK-fetched curl/mbedTLS
  • downstream compatibility patches applied successfully against v3.10.240.1
  • git diff --check

Centralize the self-contained Linux curl/mbedTLS threading setup so consumers no longer carry private build scripts. Document the shared platform policy and system-SQLite lifecycle required when multiple embedded SDK copies coexist.

Files changed:
- cmake/MatsdkFetchCurl.cmake
- docs/embedding-with-cmake.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001 and others added 2 commits September 20, 2026 11:30
Installed system-provider packages must support CMake versions whose FindSQLite3 module exposes SQLite::SQLite3 instead of SQLite3::SQLite3. Normalize the legacy target to the SDK's canonical target so Linux system mode remains optional and portable.

Files changed:
- cmake/MatsdkDependencyTargets.cmake
- cmake/MSTelemetryConfig.cmake.in

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Require CMake 4.3 for builds and installed-package consumers so FindSQLite3 provides SQLite3::SQLite3 without a legacy compatibility alias. Pin CI to CMake 4.4.2 and use the newest sdkmanager release, CMake 4.1.2, for Android source builds where system SQLite discovery is not used.

Files changed: root/package dependency configuration, CI workflows and setup action, Android build entry points, standalone examples/tests/wrappers, and build documentation.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Standardizes CMake requirements, Android tooling, dependency policies, and fetched mbedTLS threading configuration across SDK consumers and CI.

Changes:

  • Raises CMake requirements and pins CMake 4.1.2 for Android.
  • Documents packaged dependency policies and SQLite lifecycle ownership.
  • Centralizes CMake setup in CI and enables pthread-safe fetched mbedTLS.
File Description
wrappers/​swift/​CMakeLists.txt Updated as part of this pull request.
wrappers/​obj-c/​CMakeLists.txt Updated as part of this pull request.
tools/​build-android-aar.sh Updated as part of this pull request.
tests/​vcpkg/​CMakeLists.txt Updated as part of this pull request.
tests/​embedding/​CMakeLists.txt Updated as part of this pull request.
README.md Updated as part of this pull request.
lib/​android_build/​maesdk/​src/​main/​cpp/​CMakeLists.txt Updated as part of this pull request.
lib/​android_build/​maesdk/​build.gradle Updated as part of this pull request.
lib/​android_build/​app/​src/​main/​cpp/​CMakeLists.txt Updated as part of this pull request.
lib/​android_build/​app/​build.gradle Updated as part of this pull request.
examples/​objc/​cocoa-app/​CMakeLists.txt Updated as part of this pull request.
examples/​cpp/​SampleCppMini/​CMakeLists.txt Updated as part of this pull request.
examples/​cpp/​SampleCpp/​CMakeLists.txt Updated as part of this pull request.
examples/​cpp/​MacProxy/​CMakeLists.txt Updated as part of this pull request.
examples/​cpp/​EventSender/​CMakeLists.txt Updated as part of this pull request.
examples/​c/​SampleC-Guest/​CMakeLists.txt Updated as part of this pull request.
docs/​embedding-with-cmake.md Updated as part of this pull request.
docs/​cpp-start-android.md Updated as part of this pull request.
docs/​building-with-vcpkg.md Updated as part of this pull request.
CMakeLists.txt Updated as part of this pull request.
cmake/​MSTelemetryConfig.cmake.in Updated as part of this pull request.
cmake/​MatsdkRequirePresetSupport.cmake Updated as part of this pull request.
cmake/​MatsdkOptions.cmake Updated as part of this pull request.
cmake/​MatsdkFetchCurl.cmake Updated as part of this pull request.
build-android.cmd Updated as part of this pull request.
.github/​workflows/​vcpkg-release-bump.yml Updated as part of this pull request.
.github/​workflows/​test-win-latest.yml Updated as part of this pull request.
.github/​workflows/​test-vcpkg.yml Updated as part of this pull request.
.github/​workflows/​test-embedding.yml Updated as part of this pull request.
.github/​workflows/​spm-release.yml Updated as part of this pull request.
.github/​workflows/​codeql-analysis.yml Updated as part of this pull request.
.github/​workflows/​build-ubuntu-2204.yml Updated as part of this pull request.
.github/​workflows/​build-posix-latest.yml Updated as part of this pull request.
.github/​workflows/​build-ios-mac.yml Updated as part of this pull request.
.github/​workflows/​build-android.yml Updated as part of this pull request.
.github/​actions/​setup-cmake/​action.yml Updated as part of this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread CMakeLists.txt
Persist the pip-installed CMake binary directory through GITHUB_PATH so subsequent workflow steps use 4.4.2 instead of the runner default. Avoid the removed Android SDK 'tools' package while retaining platform-tools and the explicitly installed NDK and CMake packages.

Files changed: .github/actions/setup-cmake/action.yml, .github/workflows/build-android.yml, and .github/workflows/codeql-analysis.yml.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread CMakeLists.txt Outdated
Make the supported build.sh setup path install verified CMake 4.4.2 binaries on x86_64 and arm64 when the host provides an older release. Preserve caller-provided modern CMake paths and document the CMake 4.3 floor.

Files changed: build.sh, tools/setup-buildtools.sh, and docs/linux-setup-build.md.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Moderate issues remain around stale tool markers, installer status validation, and unsupported Linux architecture handling.

Review effort: Lite
Findings: 1 High severity

Open (1)

Version the build-tools marker so existing checkouts rerun setup for the CMake 4.4.2 requirement. Propagate installer failures without writing a success marker, and give unsupported architectures an actionable CMake 4.3+ requirement.

Files changed: build.sh, tools/build-common.sh, and tools/setup-buildtools.sh.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved CMake compatibility and build-tool marker issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread tests/embedding/CMakeLists.txt Outdated
bmehta001 and others added 2 commits September 21, 2026 11:27
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Allow the Android embedding test and downstream Android source consumers to enter the root project with sdkmanager's CMake 4.1.2. The root project continues to reject versions below 4.3 on non-Android platforms.

Files changed: tests/embedding/CMakeLists.txt.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
bmehta001 requested a lite review from Copilot September 21, 2026 17:12
…endencies' into bhamehta/standardize-telemetry-dependencies

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Write CMake version marker only after successful setup

build.sh:167

This unconditionally records cmake-4.4.2 even when the NOROOT branch deliberately skipped installation. If that run leaves an older CMake on PATH, a later run without noroot sees the matching marker, skips setup, and fails the new 4.3 requirement instead of installing CMake. Only write the version marker after a successful setup (and leave it stale/empty when setup was skipped).

Comment thread build-android.cmd Outdated
bmehta001 and others added 2 commits September 22, 2026 03:13
Resolve the Windows workflow conflict while preserving CMake setup and the WinHTTP/WinInet matrix. Set the desktop API floor to Windows 8.1 and remove the pre-8.1 WinHTTP proxy fallback so CI enforces the supported contract.

Files changed: merged upstream main; .github/workflows/test-win-latest.yml; README.md; lib/CMakeLists.txt; lib/http/HttpClient_WinHttp.cpp; Solutions Windows project files; tests/headers/check_public_headers.cmd.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Version and validate the Android CMake/NDK marker so stale setup state cannot hide missing tools. Provision CMake for the Linux no-exceptions job that failed under the runner's 3.31 release.

Raise desktop builds and header gates to the Windows 10 API floor. Remove the Windows 7 runtime probe, hand-defined network-cost GUID, and obsolete warning suppressions in favor of the SDK IID.

Files changed: Android and Linux setup paths, Windows workflows/docs/project definitions, WinHTTP and network detection sources, and the public-header gate.

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

Copilot-Session: fa477318-3257-45cd-8711-d5214b5cb119
@bmehta001
bmehta001 requested a lite review from Copilot September 22, 2026 16:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Six unresolved moderate issues remain around CMake metadata, build markers, legacy setup paths, and supported Linux architectures.

Review effort: Lite
Findings: None

Resolved since last review (1)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Private mbedTLS threading definitions create an ABI layout mismatch with the fetched curl consumer.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread cmake/MatsdkFetchCurl.cmake Outdated
Expose the public mbedTLS threading macros to curl so both dependencies compile public context types with identical layouts and avoid an entropy-context overflow.

Files changed: cmake/MatsdkFetchCurl.cmake.

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

Copilot-Session: fa477318-3257-45cd-8711-d5214b5cb119

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The release workflow compatibility and stale iOS build-tools marker issues must be resolved.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity iOS wrapper treats stale unversioned buildtools marker as valid

cmake/​MatsdkRequirePresetSupport.cmake:1

Raising this check leaves build-ios.sh unable to migrate an existing checkout: that wrapper still treats any .buildtools file as valid (build-ios.sh:89-94), while older runs created an unversioned/empty marker. With CMake 3.x and that marker present, the wrapper skips Homebrew setup and now fails here on every run. Version or validate the iOS marker just as build.sh and the Android setup now do.

@bmehta001 bmehta001 closed this Sep 22, 2026
@bmehta001 bmehta001 reopened this Sep 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate build/linkage and directory-cleanup issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread lib/pal/desktop/NetworkDetector.cpp
Comment thread tools/setup-buildtools-android.cmd
bmehta001 and others added 3 commits September 23, 2026 16:31
Added commands to install Android SDK platforms and sources.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add uuid to the Windows target dependencies so SDK-declared COM GUID symbols resolve for CMake consumers instead of relying on toolchain defaults.

Files changed:
- lib/CMakeLists.txt: propagate the Windows UUID import library through mat.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c72c9f67-f709-4c28-a8fa-809e0aefc14d
…endencies' into bhamehta/standardize-telemetry-dependencies

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Build-tool failures can continue the build or leave the caller’s working directory changed.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Propagate installer failure to stop the build

tools/​build-common.sh:40

Returning a nonzero status here does not stop build.sh: its only caller at build.sh:155 invokes this function as an unchecked command, and that script does not enable set -e. An installer failure therefore prints the message and avoids the marker, but the build continues anyway. Update the caller to exit when this function fails (while preserving return 1 in this sourced helper).

Disable curl's build-host CA auto-detection and remove generated CA path macros so redistributable Linux binaries rely on the target host's runtime CA selection.

Files changed:
- cmake/MatsdkFetchCurl.cmake: sanitize fetched curl CA defaults and enforce that no build-time path remains.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c72c9f67-f709-4c28-a8fa-809e0aefc14d
@bmehta001 bmehta001 closed this Sep 23, 2026
bmehta001 added a commit to bmehta001/cpp_client_telemetry that referenced this pull request Sep 23, 2026
Integrate PR microsoft#1537 into PR microsoft#1536 so the dependency and platform updates ship with the leak-analysis work. Resolve the network detector overlap in favor of the leak-safe WinRT lifecycle, narrow the SEH warning suppressions, and consistently enforce the Windows 10 API floor without legacy Windows fallbacks.

Files changed: Windows workflows/projects/docs, CMake dependency setup, Android build setup, WinHTTP transport, and WinRT network detection.

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

Copilot-Session: d2b55402-0dec-4ad7-bf0f-d30d92c96171
@bmehta001

Copy link
Copy Markdown
Contributor Author

Integrated into #1536 in merge commit 330c78c. The conflict was resolved in favor of #1536's leak-safe WinRT network detector, and the combined branch now consistently targets Windows 10 or newer without legacy Windows 7/8/8.1 fallbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants