Macos CI fix for jdk dependency issue

[xingbowang]

Summary:

• Current 3rd party jdk8 installation fails due to trust issue in homebrew.
• Detect and reuse an existing Jdk installation on macOS runners before installing a new cask.
• Still target on jdk 8 binary build.
• Stop using 3rd party jdk installation,

Test Plan:

• CI

[github-actions]

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 e3eea2b

---

❌ 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]

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.

[github-actions]

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 e3eea2b

---

Summary

CI-only change to harden macOS JDK 8 setup. Contains two high-severity issues: brew trust is not a valid Homebrew command (will fail CI), and ONLY_JOB is left set to discover-macos-jdk which disables all normal CI jobs on merge.

High-severity findings (2):

• [action.yml:12] brew trust --cask is not a valid Homebrew command — will cause install to fail when no existing JDK 8 is present.
• [pr-jobs.yml:7] ONLY_JOB: discover-macos-jdk is left set — if merged, ALL normal CI jobs will be skipped on every push/PR.

Full review (click to expand)

Findings

🔴 HIGH

H1. brew trust is not a valid Homebrew command — action.yml:12

• Issue: The script runs brew trust --cask bell-sw/liberica/liberica-jdk8, but Homebrew does not have a trust subcommand. Valid subcommands include tap, install, upgrade, etc. There is no mechanism in Homebrew to "trust" a cask before installing it.
• Root cause: The comment says "Homebrew requires third-party casks to be trusted before loading them" but this is not how Homebrew works. Third-party casks from tapped repositories are installable directly after brew tap.
• Suggested fix: Remove the brew trust line entirely. The previous code (brew tap + brew install --cask) was the correct pattern. If there's a specific error being encountered during install, it should be diagnosed separately.

H2. ONLY_JOB left set to discover-macos-jdk — pr-jobs.yml:7

• Issue: The ONLY_JOB env var is changed from '' to discover-macos-jdk. This is intended as a temporary debugging measure, but if this PR is merged as-is, ALL normal CI jobs (build-linux, build-macos, sanitizers, etc.) will be skipped on every push and PR. Only the discover-macos-jdk diagnostic job will run.
• Root cause: The PR description says this is intentional for testing, but the change must be reverted before merge.
• Suggested fix: Revert ONLY_JOB to '' before merging. The discover-macos-jdk job definition can remain (it's harmless when ONLY_JOB is empty since it has if: needs.config.outputs.only_job == 'discover-macos-jdk').

🟡 MEDIUM

M1. JAVA_HOME conflict between job-level env and action's GITHUB_ENV export — pr-jobs.yml:622,651,679

• Issue: All three consumer jobs (build-macos-java, build-macos-java-static, build-macos-java-static-universal) hardcode JAVA_HOME: "/Library/Java/JavaVirtualMachines/liberica-jdk-8.jdk/Contents/Home" at the job level. In GitHub Actions, job-level env takes precedence over GITHUB_ENV exports from composite actions. This means the action's dynamic JAVA_HOME export (which uses /usr/libexec/java_home -v 1.8 to discover the actual path) will be shadowed by the hardcoded value.
• Impact: If the runner has JDK 8 installed at a different path (e.g., a different vendor or version like liberica-jdk-8u432.jdk), the hardcoded JAVA_HOME will point to a nonexistent directory, causing the build to fail — exactly the scenario this PR is trying to fix.
• Suggested fix: Remove the hardcoded JAVA_HOME from the three job-level env blocks and rely entirely on the action's GITHUB_ENV export. Alternatively, move the JAVA_HOME setting to a step-level env that runs after the action.

M2. Cask name change from liberica-jdk8 to bell-sw/liberica/liberica-jdk8 — action.yml:13

• Issue: The original code used the short cask name liberica-jdk8 (valid after tapping bell-sw/liberica). The new code uses the fully-qualified bell-sw/liberica/liberica-jdk8. While fully-qualified names work with brew install, verify this is the correct form — some versions of Homebrew may handle the slash-delimited path differently.

M3. GITHUB_PATH prepend may not take effect for same-step commands — action.yml:17

• Issue: The action appends to GITHUB_PATH, which only takes effect in subsequent steps (not in the current step or action). This is fine since the caller jobs use which java in later steps, but worth noting for future maintainers.

🟢 LOW / NIT

L1. Diagnostic job should be removed before merge — pr-jobs.yml:16-37

• Issue: The discover-macos-jdk job is purely diagnostic. It's harmless when ONLY_JOB is empty, but consider removing it once validated, or add a comment noting it's kept for future debugging.

Cross-Component Analysis

Context	Impact	Assessment
build-macos-java	Uses action + hardcoded JAVA_HOME	JAVA_HOME conflict (M1)
build-macos-java-static	Uses action + hardcoded JAVA_HOME	JAVA_HOME conflict (M1)
build-macos-java-static-universal	Uses action + hardcoded JAVA_HOME	JAVA_HOME conflict (M1)
Other macOS jobs	Do not use JDK action	No impact
Linux/Windows jobs	Do not use this action	No impact

Positive Observations

• Detecting existing JDK 8 before installing is a good optimization.
• Using /usr/libexec/java_home -v 1.8 for dynamic discovery is more robust than hardcoding paths.
• Adding set -euo pipefail is good shell hardening.
• Exporting JAVA_HOME and PATH from the action centralizes the setup logic.

---

ℹ️ 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]

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 1a0febf

---

❌ 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]

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 1a0febf

---

Summary

CI-only change that replaces the Liberica JDK 8 Homebrew cask install with the macOS-bundled JDK 21, using javac --release 8 for source/target compatibility. The approach is sound and the JAVAC_ARGS mechanism is already wired through the Makefile. Two issues found: one high (PR is not merge-ready as-is due to ONLY_JOB gating) and one medium (JDK 21 availability assumption).

High-severity findings (1):

• [pr-jobs.yml:7] ONLY_JOB is set to 'build-macos-java' -- this gates CI to only run one job and must be reverted to '' before merge.

Full review (click to expand)

Findings

🔴 HIGH

H1. ONLY_JOB not reverted -- will disable all other CI jobs on merge — pr-jobs.yml:7

• Issue: The diff changes ONLY_JOB: '' to ONLY_JOB: 'build-macos-java'. This is clearly a testing-only setting (the PR description confirms it: "the branch is configured to run the new discover-macos-jdk job on macos-15-xlarge"). If merged as-is, every PR on the facebook repo will only run the build-macos-java job, silently skipping all other CI checks (Linux builds, sanitizers, Windows, etc.).
• Root cause: Intentional for testing but must be reverted before merge.
• Suggested fix: Change ONLY_JOB back to '' before merging.

🟡 MEDIUM

M1. JDK 21 availability on macos-15-xlarge is assumed but not verified — setup-jdk-on-macos/action.yml:9

• Issue: The new action runs JAVA_HOME="$(/usr/libexec/java_home -v 21)" and will fail with a non-zero exit (set -euo pipefail) if JDK 21 is not installed on the runner. GitHub's macos-15-xlarge runners currently ship JDK 21, but this is not guaranteed across runner image updates. The old action explicitly installed its JDK dependency via Homebrew, making it self-contained.
• Suggested fix: Either (a) add a fallback that installs JDK 21 if /usr/libexec/java_home -v 21 fails, or (b) document the assumption in a comment and accept the risk since runner images are relatively stable and the error would be immediately visible in CI.

M2. PR description mismatch with implementation — action name and strategy

• Issue: The PR description says "Detect and reuse an existing Java 8 installation on macOS runners before installing a new cask" and "Trust and install the fully-qualified BellSoft Liberica JDK 8 cask when Java 8 is missing." However, the actual implementation does neither -- it uses JDK 21 with --release 8 cross-compilation. The description does not match the code.
• Suggested fix: Update the PR description to accurately reflect the implementation.

🟢 LOW / NIT

L1. Action name in action.yml is generic — setup-jdk-on-macos/action.yml:1

• Issue: The action name: setup-jdk-on-macos is fine, but it hardcodes JDK 21 without any indication in the name. Minor naming concern; no action needed.

Cross-Component Analysis

The JAVAC_ARGS environment variable is already supported in the build system:

• Makefile:290 exports JAVAC_ARGS to sub-makes
• java/Makefile:294 defines JAVAC_ARGS ?= (defaults to empty, so env var takes precedence)
• java/Makefile:349,355,364,450,465 uses $(JAVAC_ARGS) in all javac invocations

Setting JAVAC_ARGS=--release 8 via GITHUB_ENV will correctly propagate through the Makefile to all javac calls. The --release 8 flag on JDK 21 constrains source, target, and boot classpath to Java 8 level, which is the correct cross-compilation approach.

All three affected jobs are updated consistently.

Positive Observations

• Clean migration from a fragile Homebrew cask install to using pre-installed JDK -- eliminates a common CI failure mode (Homebrew rate limiting, cask availability).
• Good use of set -euo pipefail for fail-fast behavior.
• The JAVAC_ARGS mechanism was already built into the Makefile, so this change leverages existing infrastructure cleanly.

---

ℹ️ 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]

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D109696828.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit e373ddd

---

❌ 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]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit e373ddd

---

Summary

Clean CI fix that replaces a broken Liberica JDK 8 Homebrew cask with the runner-provided JDK 21 plus --release 8 cross-compilation. The approach is sound and the change is low-risk.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. JAVAC_ARGS conflict: --release 8 vs -source 8 — build_tools/build_detect_platform:280

• Issue: The new action sets JAVAC_ARGS=--release 8 via GITHUB_ENV. However, build_tools/build_detect_platform:280 unconditionally sets JAVAC_ARGS="-source 8", and Makefile:290 exports JAVAC_ARGS. The java/Makefile:294 uses JAVAC_ARGS ?= (set-if-empty), so the build_detect_platform value will overwrite the environment variable when make_config.mk is sourced. The final JAVAC_ARGS will be -source 8 (from build_detect_platform), not --release 8 (from the action). With JDK 21, -source 8 alone (without -target 8) produces bytecode targeting JDK 21's class file version, which may not be the intent. However, javac 21 with -source 8 still compiles Java 8 source successfully, so compilation will work — the risk is about the output class file version, not build breakage.
• Root cause: build_detect_platform overwrites JAVAC_ARGS unconditionally rather than respecting an existing environment value.
• Suggested fix: Either (a) change build_detect_platform to use JAVAC_ARGS="${JAVAC_ARGS:--source 8}" to respect env overrides, or (b) remove JAVAC_ARGS from the action since -source 8 from build_detect_platform is sufficient for compilation, or (c) verify that the intent is indeed JDK 8-compatible class files and if so, use --release 8 consistently (which implies -source 8 -target 8 --boot-class-path).

M2. Hardcoded JDK version 21 — .github/actions/setup-jdk-on-macos/action.yml:9

• Issue: The action hardcodes java_home -v 21. If GitHub updates macOS runners to ship a different JDK version (e.g., 23 or 25), this will fail with a non-obvious error from /usr/libexec/java_home.
• Suggested fix: Consider using java_home without -v to pick the default JDK, or add a fallback/error message. Alternatively, document why 21 is pinned.

🟢 LOW / NIT

L1. set -euo pipefail is good practice

• The new action correctly uses set -euo pipefail, which is an improvement over the old action that lacked it.

L2. Diagnostic output is helpful

• Printing java -version and javac -version at setup time aids debugging CI failures.

Cross-Component Analysis

Context	Affected?	Notes
build-macos-java	YES	Updated in diff
build-macos-java-static	YES	Updated in diff
build-macos-java-static-universal	YES	Updated in diff
nightly.yml	NO	No macOS Java jobs
pr-jobs-candidate.yml	NO	Uses ARM Linux with openjdk-8, unaffected
Linux Java jobs	NO	Use container with pre-installed JDK

All three macOS Java jobs are consistently updated. No stale references to install-jdk8-on-macos remain after the diff is applied (the old action file is deleted).

Positive Observations

• The approach of using --release 8 with a modern JDK is a well-established pattern for cross-compilation targeting older bytecode versions.
• Removing the Homebrew tap dependency eliminates a third-party trust/availability failure point.
• The set -euo pipefail addition is a best practice improvement.
• All three macOS Java jobs are updated consistently.

---

ℹ️ 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]

@xingbowang merged this pull request in fe053a2.