Rearchitect vcpkg port for official submission#1414
Conversation
8c1d2b5 to
0999317
Compare
lalitb
left a comment
There was a problem hiding this comment.
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:
- Let #1415 land first (after addressing its open feedback)
- 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.
3c281e7 to
549645f
Compare
|
@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 |
7a5f685 to
0d9bb76
Compare
6d9b5c4 to
c857e4b
Compare
e6fd7e3 to
9e2b8fe
Compare
- 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>
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 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>
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>
Avoid carrying a confusing note about warning flags that are not used, and keep CMakeLists.txt focused on active compiler settings. Files changed: - CMakeLists.txt 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>
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>
The previous submodule pin (2022.02.02) cannot find Visual Studio 2026 (VS 18) and rejects local vcpkg builds with 'Unable to find a valid Visual Studio instance'. Update to the 2026.04.27 release so the bundled vcpkg supports VS 2026 toolchains. Validated by running tests/vcpkg/test-vcpkg-windows.ps1 with VS 2026, x64-windows-static triplet: vcpkg installed mstelemetry from the pinned GitHub SHA together with sqlite3, zlib, and nlohmann-json, the consumer linked, and all 7 integration assertions passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c9f028f to
0b0be1a
Compare
Addresses #1412, #1390, and #781
Also addresses ##1339
Changes:
vcpkg
CMake (had to modify to fix some behavior in legacy builds and for vcpkg)
(-lsqlite3 -lz).
Build flags
Obj-C/Swift wrapper resilience
Updated samples
Test infrastructure (tests/vcpkg/)
- 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.
Pipelines
Other changes
After merging
For zlib vcpkg lacking SIMD patches:
for the triplet .cmake file
Then, in vcpkg.json for the downstream consuming project, a user can use:
Another option for future consideration would be to use zlib-ng directly with ZLIB_COMPAT OFF (giving us a different namespace and preventing any package conflicts, duplicate symbols, and any issues that can come from that). This would also require updating the codebase to use the updated header files, which means that to prevent breaking the standard CMake build, we would have to also updated the vendored version to use zlib-ng (which would make updating the version easier but would require extensive testing).