Update folly hash for 11.6 release#14912
Conversation
✅ clang-tidy: No findings on changed linesCompleted in 0.0s. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D110524765. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D110524765. |
Codex Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.🟡 Codex Code ReviewAuto-triggered after CI reached the early-review threshold — reviewing commit b6ca6d8 ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
Claude Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.✅ Claude Code ReviewAuto-triggered after CI reached the early-review threshold — reviewing commit b6ca6d8 SummaryThis PR updates the pinned folly commit hash in No high-severity findings. Full review (click to expand)Findings🔴 HIGHNone. 🟡 MEDIUMNone. 🟢 LOW / NITL1. New folly commit not independently verified —
|
| Context | Affected? | Notes |
|---|---|---|
| Non-folly builds | No | Hash is unused unless make checkout_folly is invoked |
| USE_FOLLY=1 builds | Yes | Will fetch and build the new folly version |
| USE_FOLLY_LITE=1 builds | Indirectly | checkout_folly fetches boost/fmt sources used by LITE mode |
| All other build modes | No | No impact |
Positive Observations
- The change is minimal and well-scoped — exactly one variable updated.
- The PR description clearly states the purpose (11.6 release workflow) and gives reviewers the option to close if folly bump should be skipped.
- Pinning to a specific commit hash (rather than a branch) is good practice for reproducible builds.
ℹ️ 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
Codex Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.🟡 Codex Code ReviewAuto-triggered after CI reached the early-review threshold — reviewing commit fec822a ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
Claude Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.✅ Claude Code ReviewAuto-triggered after CI reached the early-review threshold — reviewing commit fec822a SummaryStraightforward build-system PR that updates the folly dependency for the 11.6 release: bumps the folly commit hash, removes the now-unnecessary double-conversion dependency, fixes gflags library version references (2.2 to 2.3), and improves library path discovery for libevent. Overall well-structured with one medium-severity consistency concern. High-severity findings (0): Full review (click to expand)Findings🔴 HIGHNone. 🟡 MEDIUMM1. folly.mk still hardcodes gflags
|
| Context | Code executes? | Assumptions hold? | Action needed? |
|---|---|---|---|
| Make + USE_FOLLY=1 | YES | YES (gflags 2.3 matches new getdeps) | None |
| CMake + USE_FOLLY=1 + Debug | YES | YES (glob discovers version) | CI coverage gap (L1) |
| CMake + USE_FOLLY=1 + Release | YES | N/A (debug block skipped) | None |
| USE_FOLLY_LITE | NO | N/A (different code path) | None |
| AIX platform | YES | YES (double-conversion removed consistently from both -I/-isystem paths) | None |
| BUCK (internal) | NO | N/A (not affected by folly.mk) | None |
Positive Observations
- The CMake glob approach for gflags is a clear improvement over hardcoding -- it prevents this same breakage in future version bumps.
- The LIBEVENT_LIB_PATH addition follows the established pattern for GLOG_LIB_PATH and FMT_LIB_PATH.
- The double-conversion removal is clean and complete within folly.mk (all 6 reference sites updated).
- The FATAL_ERROR in CMake if the glob finds nothing provides a clear failure message.
- The comment update ("some libraries" instead of "glog and fmt") correctly reflects the addition of libevent to the lib/lib64 pattern.
ℹ️ 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
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D110524765. |
🟡 Codex Code ReviewAuto-triggered after CI passed — reviewing commit fec822a ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit fec822a SummaryBuild system update for RocksDB 11.6 release: bumps folly commit hash, removes double-conversion dependency, bumps gflags from 2.2 to 2.3, fixes libevent and gflags library path resolution. The CMakeLists.txt change is well-designed (glob-based), but folly.mk re-hardcodes a version number instead of adopting a similar approach. High-severity findings (0): Full review (click to expand)Findings🟡 MEDIUMM1. folly.mk hardcodes gflags soname
|
| Component | Impact | Assessment |
|---|---|---|
| Make build (folly.mk) | Folly hash bump, dependency removal, version bumps | Correct, works for current gflags 2.3 |
| CMake build (CMakeLists.txt) | Glob-based gflags resolution for debug builds | Robust, future-proof |
| CI caching (.github/actions/cache-folly) | Cache key uses md5 of folly.mk | Cache will correctly invalidate |
| BUCK build | Not affected (no folly.mk dependency) | N/A |
| double-conversion | Removed from folly.mk only; no other references exist | Clean removal |
Positive Observations
- The CMakeLists.txt glob approach with a
FATAL_ERRORfallback is well-designed and robust. - The
LIBEVENT_LIB_PATHvariable follows the existing pattern used forGLOG_LIB_PATHandFMT_LIB_PATH, maintaining consistency. - The double-conversion removal is clean -- no stale references remain elsewhere in the codebase.
- CI cache invalidation is handled automatically via the md5 hash of
folly.mk.
ℹ️ 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
Summary
folly.mkFOLLY_COMMIT_HASH to 2a68075b77d9This is folly's
mainat release-cut time. Review and merge to take the bump, or close to skip folly for this release.Part of 11.6 release workflow.