The GitHub Actions job "Fory CI" on fory.git/main has succeeded. Run started by GitHub user chaokunyang (triggered by chaokunyang).
Head commit for run: 0a96e1c29a94eb8a006fc89a8351f212bcb6380d / Truffle <[email protected]> fix(c++): propagate /Zc:preprocessor on MSVC for FORY_STRUCT consumers (#3694) ## Why? #3693 reports that `FORY_STRUCT` fails to compile under MSVC unless `/Zc:preprocessor` is set. The macro forwards `__VA_ARGS__` through nested macros, which the legacy MSVC preprocessor expands as a single token. `.bazelrc:59-60` already sets the flag for Bazel Windows builds; CMake consumers had to remember to add it manually. #3078 papered over the gap in the docs sample, but every downstream CMake project that depends on `fory::serialization` / `fory::row_format` / `fory::fory` still has to opt in by hand. ## What does this PR do? - Adds `target_compile_options(fory_meta PUBLIC $<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>)` in `cpp/fory/meta/CMakeLists.txt`. - Placed on `fory_meta` because that target owns `FORY_STRUCT` in `cpp/fory/meta/field_info.h`; PUBLIC propagates through the existing link chain (`fory_meta` -> `fory_serialization` / `fory_row_format` / `fory_encoder` -> the user-facing INTERFACE wrappers) without duplicating on three targets. - Uses a generator expression on `CXX_COMPILER_ID:MSVC` (not the legacy `if(MSVC)` variable) so the flag is omitted for `clang-cl` (compiler id `Clang`), which warns on `/Zc:preprocessor` and is already conforming. The genex also keeps the install-export hermetic: a non-MSVC consumer importing `fory::meta` through `find_package(fory)` reads an empty entry rather than a stuck flag. - The flag mirrors `.bazelrc:59-60` for Bazel parity. The docs sample at `docs/guide/cpp/index.md:55-57` becomes redundant for v0.18.0+ users but is left in place: the sample still pins `GIT_TAG v0.17.0`, so removing it would silently break readers following the docs on the prior version. ## Related issues Closes #3693 ## AI Contribution Checklist - [x] Substantial AI assistance was used in this PR: `yes` - [x] If `yes`, I included a completed [AI Contribution Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs) in this PR description and the required `AI Usage Disclosure`. - [x] If `yes`, my PR description includes the required `ai_review` summary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes. ## Does this PR introduce any user-facing change? - [ ] Does this PR introduce any public API change? - [ ] Does this PR introduce any binary protocol compatibility change? CMake users on MSVC will automatically receive `/Zc:preprocessor` when linking any fory target on v0.18.0+. No source-level API or binary protocol change. ## Benchmark N/A. Build-config change. --- ### AI Usage Disclosure - **substantial_ai_assistance**: yes - **scope**: code drafting (single-file CMake change), tradeoff exploration, PR text drafting - **affected_files_or_subsystems**: `cpp/fory/meta/CMakeLists.txt` (C++ build system) - **ai_review**: Two-reviewer loop completed per AI_POLICY.md \xa75. Reviewer A used `.claude/skills/fory-code-review/SKILL.md`; Reviewer B did not. First pass surfaced five actionable items (clang-cl misclassification, install-export hermeticity, scope justification, docs redundancy, MSVC-path verification). Revision moved from `if(MSVC) { target_compile_options(... PUBLIC /Zc:preprocessor) }` to the genex form `$<$<CXX_COMPILER_ID:MSVC>:/Zc:preprocessor>`; rerun on rev2 returned **no further actionable findings** from either reviewer. Reviewer A explicitly: \"NO FURTHER FINDINGS. Diff is clean.\" Reviewer B: \"NO BLOCKING FINDINGS. The change is correct.\" - **ai_review_artifacts**: Both review sessions ran in dedicated subagents on the rev2 diff. Reviewer A traced each prior finding through the revision (RESOLVED 1-5) plus left a residual non-blocking note recommending an MSVC CI smoke job. Reviewer B confirmed `$<CXX_COMPILER_ID:MSVC>` is the modern idiom over the legacy `MSVC` variable, PUBLIC propagation through the link chain is correct, install-export carries the genex verbatim and re-evaluates per consumer. Both transcripts available on request. - **human_verification**: Linux/GCC build of `fory_meta` succeeds; `examples/cpp/hello_world` (which uses `FORY_STRUCT` via `fory::serialization`) builds and runs end-to-end (`./hello_world` serializes/deserializes all 8 sample types). `grep -c '/Zc:preprocessor' compile_commands.json` returns 0 on Linux, confirming the genex evaluates to empty for non-MSVC. MSVC path is not exercised locally on the Linux VM; the change mirrors `.bazelrc:59-60` (already in production for Bazel Windows) and PR #3078 (merged docs precedent). A Windows CMake smoke job would be the ideal proof and is a reasonable follow-up. - **performance_verification**: N/A. - **provenance_license_confirmation**: Apache-2.0-compatible. No third-party code introduced. The line in question is original and analogous to existing in-tree `.bazelrc` settings. NIT from Reviewer B (not addressed; low priority): adding `MSVC_VERSION GREATER_EQUAL 1925` fast-fail to surface the floor explicitly rather than silently warning on older MSVC. `FORY_STRUCT`'s inline comment already documents requiring VS 2022 17.11+ for in-class usage, so older MSVC users will hit unrelated errors regardless; leaving the floor implicit matches the existing `.bazelrc` precedent. Report URL: https://github.com/apache/fory/actions/runs/26158663836 With regards, GitHub Actions via GitBox --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
