Skip to content

Rearchitect vcpkg port for official submission#1

Open
bmehta001 wants to merge 18 commits intomainfrom
bhamehta/vcpkg-port-rearchitect
Open

Rearchitect vcpkg port for official submission#1
bmehta001 wants to merge 18 commits intomainfrom
bhamehta/vcpkg-port-rearchitect

Conversation

@bmehta001
Copy link
Copy Markdown
Owner

@bmehta001 bmehta001 commented Mar 4, 2026

  • Rearchitects the vcpkg port from a shell-script-based build with different behavior for different platforms, into a pure CMake workflow for official submission to the vcpkg registry that should works on all supported platforms
  • Have created vcpkg tests and a sample project where this library was imported into ONNXRuntime and successfully tested these and all existing unit/functional tests where applicable, on Windows, WSL, Android Simulator on WSL with NDK, macOS, and iOS Simulator on macOS

Changes:

vcpkg

  • For portfile.cmake, use vcpkg_from_github() + vcpkg_cmake_configure() + vcpkg_cmake_install() + vcpkg_cmake_config_fixup()
  • Added vcpkg.json manifest with dependency declarations (sqlite3, zlib, nlohmann-json, curl), platform constraints (!uwp), and optional feature flags (azmon, privacyguard, cds, signals, sanitizer, liveeventinspector).
  • Added usage file (vcpkg usage instructions shown after install)
  • Removed: CONTROL, TODO.md, get_repo_name.sh, response_file_linux.txt, response_file_mac.txt, v142-build.patch, since they are now redundant or outdated

CMake (had to modify to fix some behavior in legacy builds and for vcpkg)

  • Use MATSDK_USE_VCPKG_DEPS option to detect vcpkg toolchain and thus
    • Resolve dependencies with find_package() ; when OFF, use existing vendored/system-installed workflow
    • Skip manual compiler/platform flag configuration (architecture, paths, etc.) for vcpkg builds
    • Added CMake package config (cmake/MSTelemetryConfig.cmake.in) to generate find_package(MSTelemetry CONFIG) support with transitive dependency re-finding (find_dependency() for sqlite3, zlib, nlohmann-json, curl, pthreads as needed).
    • Added install/export targets — install(TARGETS mat EXPORT MSTelemetryTargets ...) with GNUInstallDirs, write_basic_package_version_file(), and configure_package_config_file() to specify paths in standardized way
    • Deduplicated and simplified dependency linking — replaced the tangled shared/static forking with a clear three-way branch: vcpkg mode (imported targets), Android legacy (bundled source compilation), and Linux/macOS legacy
      (-lsqlite3 -lz).
    • Added Android HTTP client selection (HttpClient_Android.cpp is now compiled on Android instead of HttpClient_Curl.cpp)
    • Link Apple frameworks like CoreFoundation, Foundation, CFNetwork, Network, SystemConfiguration, UIKit (iOS) / IOKit (macOS) via target_link_libraries() with -framework syntax.
    • Used message(STATUS ...) consistently throughout (or FATAL_ERROR) to use official CMake keywords
    • Quote EXISTS path arguments to prevent CMP0012 warnings.

Build flags

  • Added -fno-finite-math-only (helps fix issues for ORT builds consuming this library where -ffast-math is enabled), since sqlite3 uses the INFINITY macro, which is undefined under -ffinite-math-only
  • Move -Wno-reorder to apply only to CMAKE_CXX_FLAGS (since it's a C++-only flag)
  • Split GCC vs Clang warning flags (GCC doesn't support -Wno-unknown-warning-option)
  • Removed ZLIB_WINAPI from vcpkg builds since vcpkg's zlib does not use WINAPI/STDCALL.
  • Updated MATSDK_API_VERSION from 3.4 to 3.10 (which I assume is current SDK version)

Obj-C/Swift wrapper resilience

  • Added compile-time feature detection to ODWLogger.mm to conditionally compile Privacy Guard and Sanitizer integration. When modules aren't available (e.g., submodule not cloned), calls log a warning instead of failing to compile.
  • Updated Package.swift to add Privacy Guard and Sanitizer compilation conditions.
  • Updated Swift wrapper to use #if canImport() for optional module imports.

Updated samples

  • Added shared MSTelemetrySample.cmake for ease of updating all sample CMakeLists.txt files now include this shared module that handles both vcpkg (find_package) and legacy (vendored) dependency resolution.
  • Removed hardcoded install paths from all sample CMakeLists.txt files, since it was causing local builds to fail

Test infrastructure (tests/vcpkg/)

  • Added standalone vcpkg consumer test (main.cpp, CMakeLists.txt, vcpkg.json) using core APIs (LogManager, EventProperties, PII annotations, event priority) and verifies find_package(MSTelemetry CONFIG) works correctly.
  • Added platform-specific test scripts:
    - test-vcpkg-windows.ps1 — auto-detects host architecture (x64/ARM64), finds VS via vswhere, builds with x64-windows-static or arm64-windows-static, runs 8 integration tests.
    - test-vcpkg-linux.sh — detects host triplet, builds and runs tests.
    - test-vcpkg-macos.sh — builds with host-native triplet, runs tests.
    - test-vcpkg-ios.sh — builds for both arm64-ios (device) and simulator (arm64-ios-simulator), verifies Mach-O binary architecture.
    - test-vcpkg-android.sh — cross-compiles for configurable ABI (arm64-v8a, armeabi-v7a, x86_64, x86), verifies ELF binary production.
  • Added README.md with usage instructions for all platforms.

Other changes

  • Added .gitattributes to marks tools/ports/, tests/vcpkg/, and docs/building-with-vcpkg.md with export-ignore so they are excluded from GitHub tarballs and so vcpkg files don't affect SHA needed for vcpkg, since vcpkg doesn't need the port files themselves when downloading the GitHub source tarball
  • Updated .gitignore to ignores vcpkg test build artifacts.
  • Fixed Logger.cpp destructor — removed LOG_TRACE call from ~Logger() to prevent static-destruction-order crash on iOS simulator (where debugLogMutex may be destroyed before the Logger instance). If this is an issue, I can revert it, as I am not sure if it would be an actual issue in a real iOS deployment scenario.
  • Removed stale header references from .vcxproj and .vcxproj.filters files (referenced files that no longer exist). If I was in doubt, I kept it in.
  • Updated docs (docs/building-with-vcpkg.md) to cover all platforms, overlay port usage, feature flags, and troubleshooting.

After merging

  • Need a separate commit to update the SHA and point REF at a release tag (I assume will be 3.10.42.1)
  • Then will officially submit port to vcpkg registry
    • May be possible to set it so that future releases will automatically result in updated vcpkg ports

@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch 6 times, most recently from 8c1d2b5 to 0999317 Compare March 19, 2026 04:34
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch 6 times, most recently from 7a5f685 to 0d9bb76 Compare April 1, 2026 16:55
hanselip and others added 6 commits April 8, 2026 17:40
…microsoft#1417)

* Fix JNI stale local ref in OfflineStorage_Room record iteration loops

GetAndReserveRecords, GetRecords, and ReleaseRecords each use a
pushLocalFrame/popLocalFrame scope per record. The jclass obtained from
GetObjectClass on the first iteration was stored as a plain local reference;
after popLocalFrame returns, that reference is freed by ART. On subsequent
iterations the C++ pointer is non-null, so the if(!record_class) guard is
skipped, but the jclass* now points into a freed local frame. ART's JNI
reference validity checker detects this and calls JniAbort, producing SIGABRT.

Fix: promote the jclass to a global reference via NewGlobalRef immediately
after GetObjectClass so it survives across popLocalFrame calls, and delete
it with DeleteGlobalRef after the loop.

Affected methods:
- GetAndReserveRecords: record_class
- ReleaseRecords: bt_class
- GetRecords: record_class

The bug was introduced in commit 873b562 (Jan 2021) when per-record
pushLocalFrame/popLocalFrame scoping was added. See GitHub issue microsoft#1227.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(OfflineStorage_Room): use RAII guard for JNI global refs

The initial fix (previous commit) correctly promoted jclass local refs to
global refs to survive pushLocalFrame/popLocalFrame, but placed
DeleteGlobalRef only after the loop in normal flow. Three paths leaked:

1. ThrowLogic throws std::logic_error — NOT caught by catch(runtime_error)
2. ThrowRuntime throws std::runtime_error — catch block never called Delete
3. consumer() throwing in GetAndReserveRecords also bypassed the delete

Fix: replace post-loop DeleteGlobalRef with a RAII guard struct whose
destructor calls DeleteGlobalRef unconditionally. Applied to all three
methods: GetAndReserveRecords, ReleaseRecords, and GetRecords.

Also:
- Initialize all jfieldID variables to nullptr (prevents use of
  indeterminate values if ThrowLogic fires mid field-ID setup)
- Check NewGlobalRef return value; throw std::runtime_error on null
  (caught by existing catch block)

Regression test improvements:
- GetRecords section: add order-independent set-based blob[0] check
  to verify field IDs cached on iteration 0 are valid on iterations 1+
- ReleaseRecords section: cycle GetAndReserveRecords + ReleaseRecords
  (incrementRetryCount=true) GetMaximumRetryCount()+1 times so the
  Room impl actually drops records and returns a non-empty byTenant
  array, exercising the bt_class loop. Without this, bt_class path
  was never entered.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor: consolidate GlobalRefGuard; fix build & test portability

OfflineStorage_Room.cpp:
- Move GlobalRefGuard struct into the anonymous namespace at the top of
  the file, replacing three identical inline local struct definitions.
  Eliminates duplication and makes a single authoritative definition.
- Change guard member from jclass& ref to jclass* ref_ptr for cleaner
  pointer semantics (more idiomatic RAII ownership pattern).

Tests:
- GetAndReserveRecords blob check: switch from index-based to set-based
  comparison since Memory storage returns records in LIFO order, not
  insertion order.
- GetRecords check: guard with if(!Memory) because Memory::GetRecords
  delegates to GetAndReserveRecords internally and returns nothing when
  records are already reserved.

Build fixes (macOS Apple Silicon):
- lib/CMakeLists.txt: use find_library to set IMPORTED_LOCATION on the
  sqlite3 IMPORTED GLOBAL target; without this cmake fails to generate
  on Apple Silicon where homebrew lives under /opt/homebrew.
- tests/unittests/CMakeLists.txt: add /opt/homebrew/opt/sqlite/lib path
  to the sqlite3 library lookup chain.
- lib/modules: update submodule to fix -Werror,-Wreorder-ctor build
  failure in Sanitizer.cpp (initializer list order mismatch).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Hansel Ip <hanselip@microsoft.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…oad (microsoft#1422)

* disable built-in SDK stats events by default to reduce OneCollector load

* Fix BasicFuncTests
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch from 0d9bb76 to df8b674 Compare April 28, 2026 19:02
* Modernize CI workflows and fix iOS simulator selection

- Update Actions to latest versions (checkout@v4, setup-java@v5, etc.)
- Add concurrency groups scoped to PR events only (push builds on main
  always run to completion)
- Add 30-minute timeout to iOS CI to prevent deadlock hangs
- Fix iOS simulator UUID resolution for unambiguous destination
- Fix Android SDK path detection in CodeQL workflow
- Document python3 requirement for iOS tests

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

* Fix compiler flags and scope -Wno-reorder

- Split GCC/Clang warning flags (Clang-only flags were breaking GCC)
- Add -fno-finite-math-only to restore IEEE 754 compliance needed by
  sqlite3 when -ffast-math is enabled
- Remove global -Wno-reorder, scope to Sanitizer.cpp only (submodule
  declares members in wrong order)

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

* Improve iOS test script and documentation

- Resolve simulator UUID for unambiguous xcodebuild destination
- Add python3 dependency documentation
- Update iOS getting-started docs

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

* Fix flaky functional tests for iOS simulator CI

- Use unique phase2 DB paths instead of sleep to avoid SQLite vnode
  collisions from deferred sqlite3_close_v2 cleanup
- Clean SQLite journal files (-wal, -shm, -journal) in CleanStorage
- Fix cross-test config contamination (reset storage settings)
- Use sleep(10) instead of yield() in waitForEvents to reduce CPU
  contention on single-core simulator runners
- Increase timeouts for iOS simulator environment
- Fix multi-batch upload flakiness with SetTransmitProfile(RealTime)
- Use 127.0.0.1 instead of localhost for local test server

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

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch 7 times, most recently from 6f874e4 to 5405100 Compare April 29, 2026 21:37
bmehta001 and others added 2 commits April 29, 2026 16:39
* Remove private in6 header includes

Remove the dead netinet6/in6.h imports from ODWReachability and its Objective-C unit test so Apple builds no longer fail on the private-header error from main.

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

* Remove dead reachability imports

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

* Restore explicit reachability socket headers

Add back the canonical sys/socket.h and netinet/in.h includes in ODWReachability.m and ODWReachabilityTests.mm while keeping the other dead imports removed. This avoids relying on transitive Apple headers in the reachability PR review.

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

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- 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>
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch from ffc37ed to ed30a8a Compare April 29, 2026 21:42
bmehta001 and others added 8 commits April 30, 2026 06:57
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 force-pushed the bhamehta/vcpkg-port-rearchitect branch from 6d9b5c4 to c857e4b Compare May 2, 2026 01:58
Use a real ZLIB package in the non-vcpkg Linux/macOS path instead of a bare imported z target with no location. This restores the regular debug test path on Ubuntu/macOS while keeping the vendored-vs-vcpkg dependency split intact.

Files changed:
- lib/CMakeLists.txt
- tests/functests/CMakeLists.txt
- tests/unittests/CMakeLists.txt

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

5 participants