Skip to content

Rearchitect vcpkg port for official submission#1414

Open
bmehta001 wants to merge 43 commits into
microsoft:mainfrom
bmehta001:bhamehta/vcpkg-port-rearchitect
Open

Rearchitect vcpkg port for official submission#1414
bmehta001 wants to merge 43 commits into
microsoft:mainfrom
bmehta001:bhamehta/vcpkg-port-rearchitect

Conversation

@bmehta001
Copy link
Copy Markdown
Contributor

@bmehta001 bmehta001 commented Mar 16, 2026 •
edited
Loading

Addresses #1412, #1390, and #781.

Summary

  • Re-architects the in-tree mstelemetry vcpkg port from a shell-script/MSBuild hybrid into a CMake-based port shape suitable for official vcpkg submission.
  • Adds dual CMake dependency modes: legacy vendored/system-installed dependencies by default, and vcpkg-provided dependencies when building under the vcpkg toolchain.
  • Adds CMake install/export support so consumers can use find_package(MSTelemetry CONFIG REQUIRED) and link MSTelemetry::mat.
  • Adds standalone vcpkg consumer tests for Windows, Linux, macOS, iOS cross-compile, and Android cross-compile.
  • Updates samples/wrappers so CMake consumers can use a shared helper instead of hard-coded install paths.

Stacking note

This branch is rebased on #1416 (bhamehta/code-cleanup). Review #1414 as the vcpkg port layer on top of the CMake cleanup branch.

vcpkg port changes

  • Replaces the old CONTROL-based port with vcpkg.json and a portfile.cmake using vcpkg_from_github, vcpkg_cmake_configure, vcpkg_cmake_install, and vcpkg_cmake_config_fixup.
  • Declares base dependencies: sqlite3, zlib, nlohmann-json, and curl[ssl] only where curl is used.
  • Adds usage output for CMake consumers.
  • Removes obsolete port support files: CONTROL, TODO.md, get_repo_name.sh, response files, and the old MSBuild patch.

CMake changes

  • Adds MATSDK_USE_VCPKG_DEPS to switch dependency resolution to find_package targets under vcpkg.
  • Skips manual compiler/platform flag configuration in vcpkg builds so the vcpkg toolchain controls target flags.
  • Adds package config/export generation in cmake/MSTelemetryConfig.cmake.in and lib/CMakeLists.txt.
  • Keeps legacy builds working with bundled Android sqlite/zlib and system-installed Linux/macOS dependencies.
  • Keeps Apple HTTP/framework linking target-based and compatible with vcpkg Apple/iOS builds.

Validation

  • git diff --check for the rebased branch, excluding the generated Xcode project file that uses CRLF and is already noisy under diff --check.
  • tests/vcpkg/test-vcpkg-windows.ps1 -VcpkgRoot <repo>\tools\vcpkg passed locally on Windows with VS 2026 tooling.
  • GitHub Actions vcpkg matrix is running on the rebased branch.

@bmehta001 bmehta001 requested a review from a team as a code owner March 16, 2026 09:28
@bmehta001 bmehta001 self-assigned this Mar 16, 2026
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch 6 times, most recently from 8c1d2b5 to 0999317 Compare March 19, 2026 04:34
lalitb
lalitb requested changes Mar 21, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

This PR bundles the vcpkg port rewrite with changes that are already under review in #1415 and partially in #1416. That makes this very hard to review.
Please keep these separate. The recommended path:

  1. Let #1415 land first (after addressing its open feedback)
  2. Either close this PR and open a new one rebased on top of #1415 containing only the vcpkg-unique changes or scope this PR down to just those changes and let the CI fail for now until #1415 merges. Either way, the vcpkg work should not carry duplicate bug fixes that are being reviewed and corrected elsewhere.

@lalitb lalitb mentioned this pull request Mar 21, 2026
Open
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch from 3c281e7 to 549645f Compare March 28, 2026 01:02
@bmehta001
Copy link
Copy Markdown
Contributor Author

bmehta001 commented Mar 28, 2026 •
edited
Loading

@lalitb Thanks for the reviews so far. I am not sure if you were on the email thread where I described how I split up the vcpkg PR into 3 to make it easier to review and merge. Unfortunately, I cannot make the branches completely separate, if I want to be able to ensure that the cumulative changes still pass all tests.

So, I separated the pipeline + test fixes into #1415, then created a new branch from that PR with additional CMake changes + cleanup in #1416 (fixed some syntax issues, removed outdated files, used standard CMake style that the vcpkg PR then follows, and necessary compilation fixes). The original vcpkg PR (#1414) contains all these changes plus the changes for a vcpkg port.

Thus, the order that they should be reviewed and merged in are #1415 -> #1416 -> #1414. I will cherry-pick/rebase/merge from main for the later PRs as needed as we go through PR revisions and each previous PR is merged in.

It would be great if we can focus on just #1415 for now. If this is too confusing, I can close the #1416 / #1414 PRs for now and re-open them once #1415 is merged.

Happy to discuss more over email/Teams/a call, if that is easier

@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch 5 times, most recently from 7a5f685 to 0d9bb76 Compare April 1, 2026 16:55
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch 10 times, most recently from 6d9b5c4 to c857e4b Compare May 2, 2026 01:58
@bmehta001 bmehta001 linked an issue May 2, 2026 that may be closed by this pull request
cpp_client_telemetry fails to build with CMake 4.0 #1339
Open
@bmehta001 bmehta001 linked an issue May 14, 2026 that may be closed by this pull request
Adding cpp_client_telemetry to vcpkg #1412
Open
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch from e6fd7e3 to 9e2b8fe Compare May 22, 2026 08:02
@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 06:42
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread tests/vcpkg/test-vcpkg-ios.sh
@bmehta001
Point overlay port at current PR source
1295893 
Local review found the overlay port was still downloading an older source snapshot, so vcpkg validation could pass without exercising the current PR code. Update the REF/SHA512 to the current PR source archive and validate the Windows overlay-port consumer against that archive.

Validation:
- tests/vcpkg/test-vcpkg-windows.ps1 -VcpkgRoot tools/vcpkg
- Material self-review found no issues

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 07:06
@bmehta001
Address Copilot iOS simulator host-arch comment
1827a39 
Comment 3339226785: --simulator always builds arm64-ios-simulator, which cannot run on Intel macOS. Add an early Apple Silicon capability check that also allows Rosetta shells on Apple Silicon via hw.optional.arm64.
  Verified at tests/vcpkg/test-vcpkg-ios.sh.

Validation:
- bash -n tests/vcpkg/test-vcpkg-ios.sh
- Material self-review found no issues

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread CMakeLists.txt
Comment thread lib/CMakeLists.txt
@bmehta001
Address Copilot Obj-C init config and Apple HTTP comments
4f97c68 
Comment 3339346904: APPLE_HTTP was defined in two locations. Remove the duplicate later block and keep the definition near the BUILD_APPLE_HTTP option.
  Verified at CMakeLists.txt.

Comment 3339346945: ODWLogger.h exposes PrivacyGuard/Sanitizer init-config types even when module-backed wrappers are disabled. Always compile the init-config/support classes with BUILD_OBJC_WRAPPER, and keep only the module-backed wrapper implementations conditional.
  Verified at lib/CMakeLists.txt.

Validation:
- CMake configure on Windows legacy path
- Material self-review found no issues

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 07:23
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread tests/vcpkg/test-vcpkg-ios.sh
@bmehta001
Refresh overlay port source after review fixes
3040174 
Update the overlay port REF/SHA512 so vcpkg validation downloads the current PR source snapshot that includes the latest review fixes.

Validation:
- tests/vcpkg/test-vcpkg-windows.ps1 -VcpkgRoot tools/vcpkg

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 07:33
@bmehta001
Address Copilot simulator creation hint comment
ce1e416 
Comment 3339435283: the simctl create hint omitted the required runtime identifier. Replace the invalid one-liner with guidance to use Xcode Devices and Simulators or list device/runtime identifiers before creating a simulator.
  Verified at tests/vcpkg/test-vcpkg-ios.sh.

Validation:
- bash -n tests/vcpkg/test-vcpkg-ios.sh
- Material self-review found no issues

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 07:46
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread wrappers/swift/Package.swift Outdated
Comment thread wrappers/swift/Package.swift Outdated
Comment thread zlib/CMakeLists.txt
bmehta001 and others added 2 commits June 2, 2026 14:46
@bmehta001
Address Copilot Swift settings comments
88ad746 
Comments 3339555065/3339555113: Package.swift defined MATSDK_DATAVIEWER_AVAILABLE and MATSDK_SANITIZER_AVAILABLE, but no Swift sources read those compilation conditions. Remove the unused defines and keep source exclusion as the availability mechanism. Keep MATSDK_PRIVACYGUARD_AVAILABLE because Logger.swift uses it.
  Verified at wrappers/swift/Package.swift and wrappers/swift/Sources/OneDSSwift/Logger.swift.

Comment 3339555134: no code change. add_compile_definitions requires newer CMake than the 3.5 compatibility target, so zlib must keep add_definitions here.
  Verified against CMake compatibility requirements.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Refresh overlay port source after Swift cleanup
fd9bb22 
Update the overlay port REF/SHA512 so the downloaded source snapshot includes the latest Swift package cleanup. This keeps overlay-port validation aligned with the source-bearing PR commits.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 19:50
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread tools/ports/mstelemetry/vcpkg.json Outdated
Comment thread lib/CMakeLists.txt Outdated
bmehta001 and others added 2 commits June 2, 2026 15:02
@bmehta001
Address Copilot port support and sqlite comment issues
dc53d66 
Comment 3344040029: the vcpkg port supported any non-UWP target even though the repo only supports Windows, Linux, macOS, iOS, and Android. Restrict the supports expression to those platforms and still exclude UWP.
  Verified at tools/ports/mstelemetry/vcpkg.json.

Comment 3344040085: sqlite3_bundled comment implied this SDK enables -ffast-math. Reword it to describe guarding against toolchain/environment finite-math-only flags.
  Verified at lib/CMakeLists.txt.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Refresh overlay port source after support restriction
6566897 
Update the overlay port REF/SHA512 so vcpkg validation downloads the source snapshot that includes the latest supports-expression and comment fixes.

Validation:
- tests/vcpkg/test-vcpkg-windows.ps1 -VcpkgRoot tools/vcpkg

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 requested a review from Copilot June 2, 2026 20:09
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

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

Reviewers

Copilot code review Copilot Copilot left review comments

@lalitb lalitb lalitb requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@bmehta001 bmehta001

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adding cpp_client_telemetry to vcpkg cpp_client_telemetry fails to build with CMake 4.0

4 participants

@bmehta001 @lalitb