Skip to content

Fix CI: detect Windows MSVC toolchain#14869

Closed
xingbowang wants to merge 1 commit into
facebook:mainfrom
xingbowang:fix_ci_win
Closed

Fix CI: detect Windows MSVC toolchain#14869
xingbowang wants to merge 1 commit into
facebook:mainfrom
xingbowang:fix_ci_win

Conversation

@xingbowang

@xingbowang xingbowang commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Update Windows CI to stop hardcoding Visual Studio 2022. The Windows build action now detects the installed Visual Studio C++ toolchain with vswhere, derives the matching CMake generator, and passes the corresponding version range to microsoft/setup-msbuild@v2.

Also update the Windows build steps to use the preinstalled JDK instead of installing Liberica JDK8 through Chocolatey, and build Snappy/RocksDB through cmake --build, instead of assuming fixed .sln names. Rename Windows CI jobs from VS2022-specific names to MSVC-neutral names, and move the nightly AVX2 Windows job to windows-8-core so PR and nightly CI use the same runner/toolchain path.

Testing

  • CI
@meta-cla meta-cla Bot added the CLA Signed label Jun 19, 2026
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.
@xingbowang xingbowang marked this pull request as draft June 19, 2026 13:17
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit e90c4d0

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.
ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

  • Codex may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /codex-review [context] — Request a code review
  • /codex-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit e90c4d0

Summary

CI-only PR that adds VS2022 Build Tools auto-installation to the Windows composite action and a diagnostic job to audit the windows-8-core runner. The composite action changes are well-structured (detect, install, verify). However, the PR has a critical issue that must be addressed before merge.

High-severity findings (1):

  • [pr-jobs.yml:7] ONLY_JOB is set to 'detect-windows-8-core-software', which disables ALL other CI jobs. If merged as-is, CI is broken for all subsequent PRs.
Full review (click to expand)

Findings

🔴 HIGH

H1. ONLY_JOB left set to diagnostic job name — pr-jobs.yml:7
  • Issue: The ONLY_JOB env var is changed from '' to 'detect-windows-8-core-software'. This causes every other CI job's if: condition to evaluate to false, effectively disabling the entire CI pipeline (format checks, Linux builds, sanitizer builds, macOS builds, existing Windows builds, Java builds, etc.).
  • Root cause: This appears to be a temporary debugging aid that was not reverted before the PR was opened. The ONLY_JOB mechanism is designed for selective testing during development; it must be '' in the committed version.
  • Suggested fix: Revert line 7 to ONLY_JOB: '' before merging. The diagnostic job can still be triggered by temporarily setting ONLY_JOB in a separate development branch.

🟡 MEDIUM

M1. Diagnostic job appears temporary but has no removal plan — pr-jobs.yml:46-327
  • Issue: The detect-windows-8-core-software job is ~280 lines of diagnostic PowerShell that dumps comprehensive system information. It serves a one-time investigative purpose (understanding what's on the windows-8-core runner) but would persist in the workflow file indefinitely.
  • Suggested fix: Either (a) remove the diagnostic job before merging, keeping only the composite action changes, or (b) add a comment indicating it's temporary and should be removed after investigation.
M2. Chocolatey package not version-pinned — action.yml:36
  • Issue: choco install visualstudio2022-workload-vctools does not pin a specific package version. Future Chocolatey package updates could change behavior or break CI without any code change.
  • Suggested fix: Consider pinning with --version=X.Y.Z or at minimum add a comment acknowledging the unpinned version is intentional.
M3. --execution-timeout=0 means infinite timeout — action.yml:36
  • Issue: The --execution-timeout=0 flag disables Chocolatey's install timeout. If the VS installer hangs, the entire CI job will hang until the GitHub Actions job-level timeout (which varies per job, e.g., 20 minutes for the diagnostic job, but undefined for the composite action consumers).
  • Suggested fix: Set a reasonable timeout (e.g., --execution-timeout=1800 for 30 minutes) instead of infinite.
M4. Information disclosure in diagnostic job logs — pr-jobs.yml:92-96
  • Issue: The diagnostic job dumps environment variables matching '^(GITHUB_|RUNNER_|...)'. The GITHUB_ prefix matches GITHUB_TOKEN and ACTIONS_RUNTIME_TOKEN, which could appear in CI logs. While GitHub Actions typically masks secrets in logs, relying on the masking mechanism as the only defense is fragile.
  • Suggested fix: Exclude known secret variable names explicitly, e.g., filter out GITHUB_TOKEN, ACTIONS_RUNTIME_TOKEN, ACTIONS_ID_TOKEN_REQUEST_TOKEN.

🟢 LOW / NIT

L1. return vs exit 0 in composite action — action.yml:30
  • Issue: The script uses return to exit early when VS2022 is already found. In a PowerShell script executed by GitHub Actions, return exits the script scope correctly. However, exit 0 is more conventional and unambiguous for CI scripts.
  • Suggested fix: Consider using exit 0 instead of return for clarity.
L2. Nightly Windows job runs on windows-2022, not windows-8-core — nightly.yml:118
  • Issue: The nightly build-windows-vs2022-avx2 job runs on windows-2022 (GitHub-hosted, has VS2022 pre-installed) while PR jobs run on windows-8-core (custom runner, may lack VS2022). The composite action change handles both cases correctly (detect before install), but this asymmetry is worth noting — if the nightly runner also changes to windows-8-core, the install step will be needed there too.
  • Suggested fix: No action needed, just awareness.
L3. vswhere fallback — action.yml:22-24
  • Issue: If vswhere.exe doesn't exist on the runner, the function returns "" and proceeds to install. This is correct behavior, but vswhere is included with the VS Installer; its absence strongly suggests VS is not installed at all. The logic is sound.
  • Suggested fix: None needed.
L4. setup-msbuild pinned to tag, not SHA — action.yml:43
  • Issue: microsoft/setup-msbuild@v1.3.1 is pinned to a Git tag, not a commit SHA. Tags can be force-pushed (though this is uncommon for well-maintained actions). SHA pinning is more secure. Note that other actions in the file (e.g., hendrikmuhs/ccache-action@v1.2) are also tag-pinned, so this is consistent with existing practice.
  • Suggested fix: Consider SHA pinning consistent with the project's action pinning policy.

Cross-Component Analysis

Component Impacted? Analysis
build-windows-vs2022 (PR) YES Will use updated composite action; currently skipped due to ONLY_JOB
build-windows-vs2022-avx2 (nightly) YES Uses same composite action; runs on windows-2022 which already has VS2022, so the detect step will skip installation
All other PR jobs BLOCKED All disabled by ONLY_JOB='detect-windows-8-core-software'
detect-windows-8-core-software NEW Only job that runs; diagnostic only, no repo checkout, no build

Positive Observations

  • The detect-before-install pattern in the composite action is well-structured: check with vswhere, install if missing, verify after install, fail explicitly if verification fails.
  • The vs-version: "[17.0,18.0)" pin for setup-msbuild correctly constrains it to VS2022 (major version 17), preventing accidental pickup of VS2019 or a future VS2025.
  • The diagnostic job is thorough and would provide valuable information about the runner environment.
ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit deab4f7

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.
ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

  • Codex may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /codex-review [context] — Request a code review
  • /codex-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit deab4f7

Summary

CI-only PR that auto-detects Visual Studio and Java on Windows runners, replaces msbuild with cmake --build, and upgrades setup-msbuild. The VS detection and build modernization are sound, but the PR contains two changes that appear to be testing artifacts which would break CI if merged as-is.

High-severity findings (2):

  • [pr-jobs.yml:7] ONLY_JOB is set to 'build-windows-vs2022', which disables ALL other PR CI jobs (Linux, macOS, sanitizers, format checks, Java, crashtests).
  • [pr-jobs.yml:536-544] Two of three Windows test matrix shards ("other" with 13 test suites, and "java") are removed, silently dropping Windows test coverage for those suites from PR CI.
Full review (click to expand)

Findings

🔴 HIGH

H1. ONLY_JOB left as 'build-windows-vs2022' -- pr-jobs.yml:7
  • Issue: The ONLY_JOB env var is changed from '' to 'build-windows-vs2022'. Every job in pr-jobs.yml has a condition like needs.config.outputs.only_job == '<job-name>' || (needs.config.outputs.only_job == '' && github.repository_owner == 'facebook'). With ONLY_JOB set to a specific job name, only that one job runs. All Linux builds, macOS builds, sanitizer runs (ASAN/UBSAN/TSAN), crashtests, format checks, Java builds, and other Windows shards are skipped on every push and PR.
  • Root cause: This is almost certainly a testing artifact used to iterate on the Windows changes without waiting for the full CI suite.
  • Suggested fix: Revert to ONLY_JOB: '' before merging.
H2. Windows test matrix reduced from 3 shards to 1 -- pr-jobs.yml:536-544

  • Issue: The build-windows-vs2022 matrix previously had three shards:

    1. db_test -- runs db_test suite
    2. other -- runs arena_test, db_basic_test, db_etc2_test, db_merge_operand_test, bloom_test, c_test, coding_test, crc32c_test, dynamic_bloom_test, env_basic_test, env_test, hash_test, random_test
    3. java -- runs Java/JNI tests

    The diff removes shards 2 and 3, leaving only db_test. This eliminates 13 test suites and all Java tests from Windows PR CI. The nightly job (build-windows-vs2022-avx2) uses the action defaults and would still run all tests, but PR coverage is significantly reduced.

  • Root cause: Unclear if intentional (to reduce CI cost/time) or a testing artifact. If intentional, the rationale should be documented.

  • Suggested fix: Either restore the removed shards, or if intentional, add a comment explaining why and confirm nightly provides sufficient compensating coverage.

🟡 MEDIUM

M1. Java detection may derive wrong JAVA_HOME from shim executables -- action.yml:78-86
  • Issue: The new Java detection logic finds javac on PATH and derives JAVA_HOME by calling Split-Path twice (going up two directory levels from the javac path). On GitHub-hosted runners, javac might be a Chocolatey shim at C:\ProgramData\chocolatey\bin\javac.exe, which would yield JAVA_HOME=C:\ProgramData -- incorrect.
  • Suggested fix: After deriving JAVA_HOME, validate that $env:JAVA_HOME\bin\javac.exe actually exists. Consider also checking runner environment variables like JAVA_HOME_8_X64.
M2. JAVA_HOME not propagated to GITHUB_ENV -- action.yml:78-86
  • Issue: The detected JAVA_HOME is set via $env:JAVA_HOME = ... which is process-scoped. Unlike CMAKE_GENERATOR (written to $env:GITHUB_ENV), JAVA_HOME would be lost if the action is refactored to split steps.
  • Suggested fix: Also write JAVA_HOME to $env:GITHUB_ENV for robustness.
M3. productLineVersion may return marketing year (2022) not internal version (17) -- action.yml:30-33
  • Issue: $vsMajor = [int]$vsInstallation.catalog.productLineVersion may yield 2022 instead of 17. The fallback to installationVersion.Split(".")[0] only triggers if $vsMajor -eq 0, which won't happen for 2022. The cmake regex then searches for "Visual Studio 2022 \d{4}" which won't match cmake's actual output "Visual Studio 17 2022".
  • Suggested fix: Use installationVersion.Split(".")[0] as the primary source (always gives the internal version like "17"), with productLineVersion as fallback. Severity may be HIGH depending on what productLineVersion actually returns on the runner image -- needs verification.

🟢 LOW / NIT

L1. Hardcoded --parallel 32 -- action.yml:105,117
  • Issue: Both builds hardcode 32 parallel jobs on an 8-core runner. Intentional per existing comments about ccache hits, but could cause resource pressure.
L2. Missing error checks after java -version / javac -version -- action.yml:87-88
  • Issue: No if(!$?) { Exit $LASTEXITCODE } after the version commands. If JAVA_HOME is wrong, the error message will be unclear.

Cross-Component Analysis

Context Affected? Notes
PR CI (all jobs) YES - H1 ONLY_JOB breaks all non-Windows jobs
PR CI (Windows tests) YES - H2 Only db_test shard runs
Nightly CI (Windows) YES Runner changed, auto-detection. Action defaults cover all tests
Fork CI NO Already skipped
Build correctness MAYBE - M3 Depends on productLineVersion value

Positive Observations

  • Auto-detecting VS installation is a sound approach for portability across runner images.
  • Replacing msbuild with cmake --build is good modernization.
  • The PATH fix from $env:JAVA_HOME + ";" to "$env:JAVA_HOME\bin;" corrects a real bug.
  • $ErrorActionPreference = "Stop" ensures early failure detection.
  • -products * in vswhere correctly handles both full VS and Build Tools.
ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit a045e42

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.
ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

  • Codex may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /codex-review [context] — Request a code review
  • /codex-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit a045e42

Summary

CI-only PR that modernizes Windows build infrastructure: auto-detects VS version, replaces msbuild with cmake --build, and replaces Chocolatey Java install with auto-detection. The approach is sound overall, but the PR contains a critical testing artifact and a significant test coverage regression.

High-severity findings (2):

  • [pr-jobs.yml:7] ONLY_JOB is set to 'build-windows-msvc', which disables ALL other PR CI jobs (Linux, macOS, sanitizers, format checks, etc.). This is a testing artifact that must be reverted before merge.
  • [pr-jobs.yml:536-537] Two test matrix shards removed ("other" and "java"), eliminating PR-level Windows test coverage for 13 C++ test suites and all Java tests. Only db_test remains.
Full review (click to expand)

Findings

🔴 HIGH

H1. ONLY_JOB not reset — all non-Windows PR jobs disabled — pr-jobs.yml:7
  • Issue: ONLY_JOB is changed from '' to 'build-windows-msvc'. Every other job in pr-jobs.yml has an if: condition that checks needs.config.outputs.only_job == '' for normal operation. When ONLY_JOB is non-empty, only the job matching that name runs. This means all Linux builds, sanitizer runs, format checks, macOS builds, Java builds, crash tests, and every other PR job are skipped.
  • Root cause: This is clearly a testing artifact used to iterate on the Windows job in the PR itself. It must be reverted to '' before merge.
  • Suggested fix: 
    ONLY_JOB: ''
H2. Test matrix reduced from 3 shards to 1 — significant coverage loss — pr-jobs.yml:536-537

  • Issue: The PR removes two matrix entries from build-windows-msvc:

    • other shard: arena_test, db_basic_test, db_etc2_test, db_merge_operand_test, bloom_test, c_test, coding_test, crc32c_test, dynamic_bloom_test, env_basic_test, env_test, hash_test, random_test
    • java shard: Java tests via ctest

    Only the db_test shard remains. This means PR authors get no Windows feedback for 13 other C++ test suites and all Java tests until the nightly run (which uses the action's defaults and runs everything).

  • Root cause: Possibly intentional to reduce CI costs or as a temporary simplification during development.

  • Suggested fix: If cost reduction is the goal, consider consolidating to 2 shards (C++ tests + Java) rather than dropping coverage entirely. Alternatively, restore the full matrix or add a comment explaining the rationale.

🟡 MEDIUM

M1. Java auto-detection may be fragile on runner image updates — action.yml:76-84

  • Issue: The PR removes explicit Java installation (choco install liberica8jdk) and instead relies on Java being pre-installed on the runner. The detection logic:

    1. Checks if $env:JAVA_HOME is set and has bin\javac.exe
    2. Falls back to Get-Command javac on PATH
    3. Derives JAVA_HOME from javac location: Split-Path (Split-Path $javac.Source -Parent) -Parent

    This works today because GitHub-hosted Windows runners include JDK, but if the runner image changes (e.g., removes Java or changes the install location), the CI silently breaks. The previous explicit install was more deterministic.

  • Suggested fix: Add a comment explaining why explicit install was removed (runner already provides Java). Consider adding a more informative error message that suggests installing Java or updating the runner image.

M2. $ErrorActionPreference = "Stop" not set in "Custom steps" — action.yml:73-87
  • Issue: The "Detect Visual Studio generator" step sets $ErrorActionPreference = "Stop", but the "Custom steps" step does not. The new java.exe -version and javac.exe -version calls after the detection block don't have error checks. If these fail, execution continues silently.
  • Suggested fix: Add if(!$?) { Exit $LASTEXITCODE } after the java.exe -version and javac.exe -version calls, consistent with the existing pattern in the script.
M3. Nightly job runner changed without clear justification — nightly.yml:120
  • Issue: The nightly Windows AVX2 job runner is changed from windows-2022 to windows-8-core. The windows-2022 label is a standard GitHub-hosted runner with a well-known software list. windows-8-core appears to be a custom/larger runner label (already used in pr-jobs.yml). If windows-8-core runs a different Windows version or has different pre-installed software, this could cause test failures or behavioral differences.
  • Suggested fix: Ensure windows-8-core provides equivalent or better software coverage.

🟢 LOW / NIT

L1. cmake --help parsing is fragile — action.yml:34-38
  • Issue: The generator name is extracted from cmake --help output using a regex. The --help output format is not a stable API.
  • Suggested fix: Accept this fragility (the throw on no match provides a clear error) or map VS major versions to generator names directly.
L2. productLineVersion fallback logic — action.yml:27-30
  • Issue: $vsMajor -eq 0 as the trigger for fallback relies on PowerShell casting $null to 0. Works, but a more explicit check would be clearer.
L3. Job rename may require GitHub branch protection update — pr-jobs.yml:528, nightly.yml:116
  • Issue: If branch protection rules require the old build-windows-vs2022 status check, the rename will block merges.
  • Suggested fix: Verify and update branch protection settings.
L4. Hardcoded --parallel 32 — action.yml:105, 117
  • Issue: Not a regression (matches prior msbuild behavior) and intentional per existing comment.

Cross-Component Analysis

Aspect Status Notes
ONLY_JOB interaction BROKEN All non-Windows PR jobs disabled
Job name references Needs check Branch protection rules may reference old names
Runner compatibility OK windows-8-core already used in pr-jobs.yml
CMAKE_GENERATOR propagation OK GITHUB_ENV is available to subsequent steps
setup-msbuild v2 compatibility OK v2 supports vs-version with version range format
Java availability on runners Fragile Depends on runner image; no fallback install
cmake --build compatibility OK Drop-in replacement for msbuild with CMake projects

Positive Observations

  • Auto-detecting VS version is a forward-looking improvement that will ease migration to future VS versions.
  • Using cmake --build instead of msbuild directly is more portable and aligns with modern CMake best practices.
  • The vswhere detection step has good error handling with throw on each failure case and $ErrorActionPreference = "Stop".
  • Removing the Chocolatey install step speeds up CI.
  • The path fix ($env:JAVA_HOME\bin instead of $env:JAVA_HOME) is a genuine bug fix.
ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 85424e5

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.
ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

  • Codex may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /codex-review [context] — Request a code review
  • /codex-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 85424e5

Summary

Solid CI improvement that decouples Windows builds from a specific VS version and modernizes the build commands. The changes are well-structured and the dynamic detection approach is the right direction. A few issues worth addressing before merge.

High-severity findings (1):

  • [action.yml: JDK detection] JDK auto-detection assumes a JDK is pre-installed on the runner but provides no fallback installation. If the windows-8-core runner image changes or lacks a JDK, all Windows CI jobs (including Java tests) will fail with no recovery path.
Full review (click to expand)

Findings

🔴 HIGH

H1. No JDK installation fallback — action.yml:73-84
  • Issue: The old code explicitly installed liberica8jdk via Chocolatey, guaranteeing JDK availability. The new code only detects a pre-installed JDK — if neither $env:JAVA_HOME is set nor javac is on PATH, the step fails with Exit 1. There is no fallback to install a JDK.
  • Root cause: The PR assumes the windows-8-core runner image ships with a JDK. GitHub-hosted Windows runners do currently include JDK installations, but the specific version and availability are not guaranteed across runner image updates.
  • Suggested fix: Add a fallback that installs a JDK (e.g., via Chocolatey or the actions/setup-java action) when detection fails. Alternatively, add an explicit actions/setup-java step before the detection logic to guarantee availability. At minimum, document the runner JDK requirement.

🟡 MEDIUM

M1. cmake --help output parsing is fragile — action.yml:31-37
  • Issue: The regex ^\*?\s*Visual Studio $vsMajor\s+\d{4}\s*= parses the output of cmake --help to find the generator name. This format is an implementation detail of CMake's help output, not a stable API. Different CMake versions may format this differently.
  • Suggested fix: Consider constructing the generator name directly (e.g., "Visual Studio $vsMajor $year") using vswhere catalog data and verifying with a cmake test invocation, rather than parsing help output.
M2. $vsMajor / productLineVersion confusion — action.yml:24-27
  • Issue: catalog.productLineVersion returns the year (e.g., "2022"), not the VS major version (17). If this path is taken, $vsMajor = 2022, and the regex Visual Studio 2022\s+\d{4} would NOT match Visual Studio 17 2022 in cmake's help output. The installationVersion fallback (giving 17) is correct, but the primary path appears broken.
  • Suggested fix: Verify what productLineVersion actually returns in practice. If it returns the year, this code path needs fixing — either extract the major version differently or adjust the regex.
M3. Job rename may break branch protection rules — pr-jobs.yml:528-529
  • Issue: Renaming build-windows-vs2022 to build-windows-msvc changes the status check name. If branch protection rules require the old name, PRs cannot merge until rules are updated.
  • Suggested fix: Coordinate with branch protection rule updates.
M4. Missing -A x64 in cmake generate steps — action.yml:54,63
  • Issue: The cmake generate steps don't specify -A x64. With VS generators, cmake may default to x86 on some configurations. The -- /p:Platform=x64 passed to cmake --build may conflict if cmake generated for a different architecture.
  • Suggested fix: Add -A x64 to cmake generate invocations.

🟢 LOW / NIT

L1. Inconsistent error handling style — action.yml:78-79
  • Issue: Three different error patterns: throw (VS detection), Write-Error + Exit 1 (JDK detection), if(!$?) { Exit $LASTEXITCODE } (build steps).
  • Suggested fix: Standardize on one pattern.
L2. setup-msbuild version pinning — action.yml
  • Issue: Upgraded from @v1.3.1 (patch-pinned) to @v2 (floating major). Less deterministic.
  • Suggested fix: Consider @v2.0.0 for consistency with checkout@v4.1.0.
L3. Nightly runner change — nightly.yml:120
  • Issue: Runner changed from windows-2022 to windows-8-core. Verify these have equivalent toolchain installations.

Cross-Component Analysis

Concern Assessment
ccache compatibility with cmake --build Safe
Java test matrix At risk — auto-detected JDK version may differ from old liberica8jdk (JDK 8)
CMAKE_GENERATOR propagation via GITHUB_ENV Safe — takes effect in subsequent steps
only_job filter mechanism Works with new names; old name build-windows-vs2022 no longer matches

Positive Observations

  • Dynamic VS detection is the right architectural direction for future-proofing.
  • cmake --build instead of direct msbuild is more portable and follows CMake best practices.
  • The PATH fix ($env:JAVA_HOME\bin; instead of $env:JAVA_HOME;) is a genuine bug fix.
  • Good diagnostic output added (java/javac version, VS detection logging).
ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase
@xingbowang xingbowang marked this pull request as ready for review June 19, 2026 22:22
@meta-codesync

meta-codesync Bot commented Jun 19, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D109176291.
@xingbowang xingbowang changed the title CI: install VS2022 Build Tools for Windows jobs Jun 19, 2026
@meta-codesync

meta-codesync Bot commented Jun 19, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D109176291.
@xingbowang xingbowang mentioned this pull request Jun 20, 2026
Open
@xingbowang
CI: detect Windows MSVC toolchain
9c297b0 
Detect the installed Visual Studio C++ toolchain on Windows runners and derive the matching CMake generator/setup-msbuild version instead of hardcoding VS2022.

Keep PR and nightly Windows jobs on windows-8-core with version-neutral MSVC job names. Use the preinstalled JDK and build generated CMake projects through cmake --build so project file names do not depend on the selected VS generator.
@meta-codesync

meta-codesync Bot commented Jun 21, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D109176291.
@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 9c297b0

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.
ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

  • Codex may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /codex-review [context] — Request a code review
  • /codex-query <question> — Ask about the PR or codebase
@github-actions

github-actions Bot commented Jun 21, 2026

Copy link
Copy Markdown

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 9c297b0

Summary

CI-only change that makes Windows builds toolchain-agnostic. The approach is sound: dynamic detection via vswhere is the canonical way to find VS installations. One potential correctness issue with catalog.productLineVersion parsing, and a few minor robustness concerns.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. catalog.productLineVersion returns year string (e.g. "2022"), not major version number — action.yml:31
  • Issue: vswhere returns catalog.productLineVersion as the marketing year (e.g., "2022"), not the internal major version (e.g., 17). When [int]$vsInstallation.catalog.productLineVersion is called and the field is "2022", $vsMajor becomes 2022, not 17. The if ($vsMajor -eq 0) fallback would NOT trigger, and the regex would search for "Visual Studio 2022" in cmake's help output. CMake's help format is Visual Studio 17 2022, so the regex "^\*?\s*Visual Studio $vsMajor\s+\d{4}\s*=" with $vsMajor=2022 would NOT match — falling through to the throw.
  • Root cause: The fallback logic assumes productLineVersion is either the internal major version or 0/absent. In reality, it's the year string.
  • Suggested fix: Parse installationVersion unconditionally: 
    $vsMajor = [int]($vsInstallation.installationVersion.Split(".")[0])
    Or if productLineVersion is preferred, match the year position in the regex: 
    $_ -match "^\*?\s*Visual Studio \d+\s+$vsMajor\s*="
M2. windows-8-core is not a standard GitHub-hosted runner label — pr-jobs.yml:531, nightly.yml:118
  • Issue: Standard labels are windows-latest, windows-2022, windows-2025. windows-8-core appears to be a custom or GitHub Teams/Enterprise "larger runner" label. Needs to be pre-configured in repository runner settings.
  • Suggested fix: Verify the label is correctly configured. If it's a GitHub larger runner, confirm the exact label format.
M3. JDK availability not guaranteed on new runner — action.yml:79-89
  • Issue: Old code explicitly installed Liberica JDK 8. New code assumes a JDK is preinstalled. The fallback Get-Command javac may find an unexpected version (e.g., JDK 21 instead of 8), which could cause RocksJava compatibility issues.
  • Suggested fix: Consider actions/setup-java@v4 for deterministic version selection, or validate the JDK version after detection.

🟢 LOW / NIT

L1. Missing error check after java -version / javac -version — action.yml:88-89

These diagnostic commands run without if(!$?) { Exit $LASTEXITCODE }. If JAVA_HOME is invalid, they'd fail but the script continues. The $ErrorActionPreference = "Stop" is set in a different step and doesn't propagate here.

L2. Job rename may break branch protection rules — pr-jobs.yml, nightly.yml

Renaming build-windows-vs2022 to build-windows-msvc will break any required status checks in GitHub settings referencing the old name. No dangling references exist in code, but repo settings need updating.

L3. $env:JAVA_HOME set in process scope only — action.yml:86

Only persists within the current step's PowerShell process. Fine today since all usage is in the same run: block, but fragile for future refactoring. Consider also writing to $env:GITHUB_ENV.

Positive Observations

  • vswhere-based detection is the right approach for future-proofing.
  • cmake --build eliminates hardcoded .sln name dependency.
  • Good error handling with $ErrorActionPreference = "Stop" and explicit throw.
  • The $env:Path = "$env:JAVA_HOME\bin;" fix correctly appends \bin (old code added JAVA_HOME directly without \bin).
ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase
@meta-codesync meta-codesync Bot closed this in 5b324a2 Jun 22, 2026
@meta-codesync meta-codesync Bot added the Merged label Jun 22, 2026
@meta-codesync

meta-codesync Bot commented Jun 22, 2026

Copy link
Copy Markdown

@xingbowang merged this pull request in 5b324a2.
@zaidoon1 zaidoon1 mentioned this pull request Jun 24, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Merged

1 participant

@xingbowang