Skip to content

build: Update FBOS to v2026.10.26.00 - #19909

Open
tinadu1018 wants to merge 12 commits into
facebookincubator:mainfrom
tinadu1018:users/tiadu/update-fb-oss-20261026
Open

tinadu1018 wants to merge 12 commits into
facebookincubator:mainfrom
tinadu1018:users/tiadu/update-fb-oss-20261026

Conversation

@tinadu1018

@tinadu1018 tinadu1018 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Update the shared Facebook open-source dependency group from
v2026.09.07.00 to v2026.10.26.00, keeping Folly, Fizz, Wangle, mvfst,
and FBThrift aligned. The Folly release includes the upstream fixed-string
literal correction from meta/folly#2726 needed for Windows builds.

Refresh the bundled Folly checksum and CMake patches for the new release,
preserving both bundled and system-installed glog support.

@netlify

netlify Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 7f83cf0
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/6aea9bd2f4db37502ed4e057

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 28, 2026
@tinadu1018
tinadu1018 marked this pull request as ready for review October 29, 2026 01:01
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Selective Build Plan

Linux release with adapters is running a full build (PR has a standing approval). See the CI workflows README for what this means.


Selective build plan

@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

CI Failure Analysis

Auto-generated by the CI Failure Analysis workflow. This comment is updated in place each time CI fails on a new commit, so it always reflects the latest run — re-pushing or re-running CI will refresh the analysis below. Last updated 2026-11-04 04:35:37 UTC from workflow run 41762595093.

🔴 Linux release with adapters — BUILD Failure View logs

Build errors:

The CMake configuration step failed because the newly installed fbthrift (v2026.10.26.00) depends on Xxhash::xxhash as a link-interface target, but no find_package(xxhash) provides that target in the Velox build.

CMake Error at /usr/local/lib64/cmake/fbthrift/FBThriftTargets.cmake:178 (set_target_properties):
  The link interface of target "FBThrift::thriftmetadata" contains:

    Xxhash::xxhash

  but the target was not found.  Possible reasons include:

    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

  Call Stack (most recent call first):
    /usr/local/lib64/cmake/fbthrift/FBThriftConfig.cmake:82 (include)
    CMakeLists.txt:796 (find_package)

CMake Error at /usr/local/lib64/cmake/fbthrift/FBThriftTargets.cmake:244 (set_target_properties):
  The link interface of target "FBThrift::thriftcpp2" contains:

    Xxhash::xxhash

  but the target was not found.

CMake Generate step failed.  Build files cannot be regenerated correctly.

🔴 Linux ASAN/UBSAN with system dependencies — BUILD Failure View logs

Build errors:

The ASAN/UBSAN build uses Clang, and the updated folly (v2026.10.26.00) now adds -fcoroutines to the compile flags. This flag is GCC-only; Clang does not recognize it and treats it as an error under -Werror:

clang++: error: argument unused during compilation: '-fcoroutines' [-Werror,-Wunused-command-line-argument]

This caused ninja: build stopped: subcommand failed after compiling 40/2711 targets.

Multiple source files failed with the same error (e.g., Exceptions.cpp, BloomFilter.cpp, VeloxException.cpp).


Correlation with PR changes:

Both failures are directly caused by this PR. The PR updates FB_OS_VERSION from v2026.09.07.00 to v2026.10.26.00, which upgrades folly, fizz, wangle, mvfst, and fbthrift to the new version:

  1. Xxhash::xxhash missing target — The new fbthrift v2026.10.26.00 now declares a dependency on Xxhash::xxhash in its CMake exported targets. Velox's CMakeLists.txt does not call find_package(xxhash) (or provide an Xxhash::xxhash alias) before find_package(fbthrift), so the target is not defined when fbthrift tries to reference it. The CMakeLists.txt:796 call to find_package(fbthrift) triggers the error.

  2. -fcoroutines with Clang — The new folly v2026.10.26.00 apparently adds -fcoroutines to its compile flags. This is a GCC-specific flag (GCC uses -fcoroutines while Clang enables coroutines by default with -std=c++20). The ASAN/UBSAN job uses Clang, which rejects the unused flag under -Werror.

Known issues:

  • No open issues directly track these specific failures.
  • The most recent 7 completed runs on the main branch all passed — these are not pre-existing/flaky failures.

Recommended fix:

  1. For the Xxhash::xxhash error: Add a find_package(xxhash) call (or create an Xxhash::xxhash alias target from the existing xxhash sources in velox/external/xxhash) before the find_package(fbthrift) call at CMakeLists.txt:796. Alternatively, update the dependency install scripts to install xxhash in a way that provides the Xxhash::xxhash CMake target.

  2. For the -fcoroutines error: Add -Wno-unused-command-line-argument to the Clang warning flags in the ASAN/UBSAN build configuration, or patch folly to conditionally use -fcoroutines only for GCC (not Clang). Check if there is a folly CMake option to control coroutine flags.

tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 29, 2026
## Summary
Keep the coordinated FBOS v2026.10.26.00 upgrade and repair its Linux
integration: export GCC-only coroutine flags conditionally for the consuming
compiler, resolve a compiled xxhash target despite cuDF's source-only CPM
redirect, and use one shared system gflags dependency across Folly, glog,
and the Ubuntu shared Velox build.

Ship the FBThrift patch with each affected Docker build context and make
installer patching idempotent while rejecting incompatible dependency source.
Preserve the metadata xxhash link needed by older container installations.

## Prompting Intent
Investigate and fix the failures in GitHub CI run 40787171849 for PR facebookincubator#19909
without downgrading the FBOS release or weakening existing tests and checks.
Review the correction, resolve and validate its findings, then commit and
push the focused change to the existing PR. The engineer stopped further
review after three clean rounds and explicitly requested publication.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Failing CI run: https://github.com/facebookincubator/velox/actions/runs/40787171849
- ASAN job: https://github.com/facebookincubator/velox/actions/runs/40787171849/job/123163347906
- Adapters job: https://github.com/facebookincubator/velox/actions/runs/40787171849/job/123163656672
- Ubuntu job: https://github.com/facebookincubator/velox/actions/runs/40787171849/job/123163347458
- FBThrift release build: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/CMakeLists.txt
- FBThrift package: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/thrift/cmake/FBThriftConfig.cmake.in
- cuDF xxhash recipe: https://github.com/rapidsai/cudf/blob/c9086e0afbf0cb11194a12bb2233b80139eabd09/cpp/cmake/thirdparty/get_xxhash.cmake
- RAPIDS CPM integration: https://github.com/rapidsai/rapids-cmake/blob/037d4c5489cc13f5b378b54197238a6e2cdbff17/rapids-cmake/cpm/find.cmake
- CPM release: https://github.com/cpm-cmake/CPM.cmake/releases/tag/v0.42.0
- CMake package redirects: https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure
- Earlier metadata dependency issue: meta/fbthrift#651

## Rationale
Fix the producer's exported compiler condition instead of suppressing Clang
warnings or editing installed package exports. Directly include the existing
xxhash discovery module behind a target guard because CMake package redirects
take precedence even over MODULE lookups; this also works when a parent project
created the redirect. Keep CPM configuration and cuDF's source-only target intact,
respect existing imported targets, and fail explicitly if the compiled library
or headers are unavailable.

Build Folly shared for both Ubuntu compiler variants and resolve gflags from
SYSTEM so Folly, glog, and Velox do not register separate static and shared
copies of the same flags. Keep the dependency release and test selection intact.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 29, 2026
## Summary
Remove the broader CI and build-integration changes from this dependency
upgrade. Restore the original Linux workflow settings, gflags provider,
compiler-specific Folly linkage, xxhash discovery, FBThrift installer,
Dockerfiles, and FBThrift CMake helper.

Keep only the coordinated dependency version metadata, the bundled Folly
version and checksum, and refreshes of its two existing CMake patches.

## Prompting Intent
The engineer requested a version update with only a few small compatibility
changes required by the new Folly release. Existing build settings must not
change, and local-environment failures must not expand the repository change.
Revert the extra changes rather than replace them with other workarounds.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Original upgrade: facebookincubator@6db1162
- Broader CI correction being removed: facebookincubator@75af12c
- Folly release: https://github.com/facebook/folly/releases/tag/v2026.10.26.00
- mvfst compatibility targets: https://github.com/facebook/mvfst/blob/v2026.10.26.00/cmake/MvfstCompatAliases.cmake

## Rationale
Keep the dependency bump reviewable and preserve the existing build policy.
The retained Folly patches are adaptations of existing patches to the new
release's CMake context and dependency variables, not new build settings.
The selected mvfst release still exports the old compatibility target names,
so changing FBThriftCppLibrary's explicit link entries is unnecessary.

This restores every file outside the five-file version/Folly scope to the
pre-upgrade baseline. It narrows the change rather than claiming that the
separately observed CI failures are resolved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 29, 2026
## Summary
Restore the original gflags/glog package lookup calls and diagnostic while
retaining only the variable mappings needed by the new Folly dependency code.
Remove the added CONFIG/REQUIRED restrictions and provider-selection branch.

Preserve the original commented-out replacement text and formatting in the
no-export patch. Change only the hunk coordinates and source context needed
to match the selected Folly release.

## Prompting Intent
The engineer requested minimal changes, preservation of the original code,
and removal of unnecessary formatting differences. Keep the PR limited to
version/checksum updates and small compatibility changes required by Folly,
without changing existing build settings.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Prior scope reduction: facebookincubator@a04678b
- Original patches: https://github.com/facebookincubator/velox/tree/5df6f9b010ab15959eeb83fdecba3437774dd321/CMake/resolve_dependency_modules/folly
- Pinned dependency discovery: https://github.com/facebook/folly/blob/v2026.10.26.00/CMake/folly-deps.cmake
- Pinned install/export rules: https://github.com/facebook/folly/blob/v2026.10.26.00/CMakeLists.txt

## Rationale
Use Velox's existing package discovery and imported targets instead of
introducing a second provider-selection policy. Preserve the system Findglog
module's include information and the existing diagnostic.

The original patch hunks no longer apply to the selected source release,
but their commented replacement text does not need reformatting. Keeping
that text unchanged preserves behavior and removes review-only churn.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 29, 2026
## Summary
Make the no-export patch comment out the selected upstream statements by
prefixing their existing lines with # only. Preserve every line break,
indentation level, and blank line in the selected Folly source.

Do not combine the install arguments or split the auto_install_files call.
Keep the same install, header-install, and export statements disabled.

## Prompting Intent
The engineer pointed out that the patch was joining and splitting upstream
lines while commenting them out. Preserve the original upstream lines and
make only the minimum comment-marker changes, without reformatting code or
changing existing build behavior.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Previous patch layout: facebookincubator@aef1ebe
- Selected upstream source: https://github.com/facebook/folly/blob/v2026.10.26.00/CMakeLists.txt

## Rationale
Use the selected upstream source as the formatting reference. Preserving
the older patch's replacement layout had unnecessarily reflowed the newer
source. A line-for-line comment-only transformation preserves that source
layout and the existing no-export behavior without changing other files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 30, 2026
## Summary
Fix the two dependency-compatibility regressions introduced by the
v2026.10.26.00 FBOS upgrade: a GCC-only coroutine option leaking into
Clang consumers and FBThrift's missing Xxhash::xxhash library target
when cuDF registers a source-only CPM package redirect.

## Prompting Intent
The engineer asked to address the PR-introduced failures identified by
the PR's CI Failure Analysis report. Keep the version-upgrade scope
minimal and retain the existing formatting, compiler warning policy,
dependency providers, shared/static settings, and CI build profiles.

## Linked Sources
- PR and upgrade context: facebookincubator#19909
- CI Failure Analysis: facebookincubator#19909 (comment)
- ASAN/UBSAN failure: https://github.com/facebookincubator/velox/actions/runs/40941103139/job/123718883755
- Adapters failure: https://github.com/facebookincubator/velox/actions/runs/40941103139/job/123719208953
- FBThrift target configuration: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/CMakeLists.txt#L61-L76
- cuDF xxhash recipe: https://github.com/rapidsai/cudf/blob/c9086e0afbf0cb11194a12bb2233b80139eabd09/cpp/cmake/thirdparty/get_xxhash.cmake

## Rationale
Make the exported coroutine option conditional on the consumer's
compiler as well as its language instead of suppressing Clang warnings.
Apply the single-line correction through the dependency installer and
copy the patch alongside that installer in existing container stages.

Use the existing xxhash finder directly so the source-only CPM redirect
cannot intercept FBThrift's compiled-library lookup. Export an imported
target with the existing include path and release/debug library choices,
preserving an already-provided target. The pinned FBThrift release now
declares its xxhash target edges, replacing the old raw metadata-link
workaround without changing build settings or cuDF's own xxhash target.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 30, 2026
## Summary
Add GTest::gmock to the shared aggregate-test dependencies so the
VectorIndex test target in CI's merged checkout receives GoogleMock's
headers and library.

## Prompting Intent
The engineer asked to address the macOS missing gmock/gmock.h failure,
commit and push the known fix now, and verify it in the new CI run.
Keep the change minimal and leave CUDA settings and build profiles
unchanged.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- macOS failure: https://github.com/facebookincubator/velox/actions/runs/40995890308/job/123868890791
- Upstream vector-index tests: facebookincubator#19886
- Failing merged target: https://github.com/facebookincubator/velox/blob/633c6f0eff87ccb66d4895a2f74dd377ba564b4f/velox/functions/prestosql/aggregates/tests/CMakeLists.txt
- GoogleMock matcher usage: https://github.com/facebookincubator/velox/blob/633c6f0eff87ccb66d4895a2f74dd377ba564b4f/velox/functions/prestosql/aggregates/tests/VectorIndexBuildTest.cpp

## Rationale
The new VectorIndex test inherits VELOX_AGGREGATES_TEST_DEPS but
requires GoogleMock matchers in addition to GoogleTest. Declare the
dependency in that existing shared list instead of adding global
include paths, changing package providers, or copying upstream-only
test targets into this branch. This one-line change also reaches the
target when GitHub merges the branch with main for CI, without a
local merge or history rewrite.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 30, 2026
## Summary
Keep macOS dependency builds and Velox on Homebrew OpenSSL 3, which
is compatible with the bundled gRPC version. Reject OpenSSL 4 early
on macOS instead of reaching incompatible X509 and ASN1 APIs.

Derive Spark aggregate bias candidates from the fuzzer's own eligible
function list. When only excluded aggregates changed, preserve the
existing regular-fuzzer selection instead of starting an empty
biased run.

## Prompting Intent
The engineer reported the Spark biased aggregate failure selecting
collect_set with seed 6600 and the subsequent macos-15-debug build
failure. Address the actual causes with production logic, preserve
build profiles and existing exclusions, and publish scoped fixes
for verification in new CI. Do not suppress failures, disable tests,
or add speculative CUDA changes.

## Linked Sources
- PR: facebookincubator#19909
- Spark workflow: https://github.com/facebookincubator/velox/actions/runs/41084854188
- Spark failure: https://github.com/facebookincubator/velox/actions/runs/41084854188/job/124175927004
- Real signature artifact: https://github.com/facebookincubator/velox/actions/runs/41084854188/artifacts/12991948431
- macOS failure: https://github.com/facebookincubator/velox/actions/runs/41084854386/job/124163673084
- macOS setup in the failing checkout: https://github.com/facebookincubator/velox/blob/e5add781d8aaacf443740580d6cdb2b0fbb43c2d/scripts/setup-macos.sh
- Homebrew OpenSSL 3 formula: https://formulae.brew.sh/formula/openssl@3
- Homebrew prefix behavior: https://docs.brew.sh/Manpage
- OpenSSL discovery and version ranges: https://cmake.org/cmake/help/latest/module/FindOpenSSL.html

## Rationale
Homebrew's unversioned openssl formula now selects 4.1.0, whose APIs
are incompatible with gRPC 1.48.1. Select the maintained 3.x formula
consistently, honor explicit installation roots, and leave non-Apple
OpenSSL discovery unchanged rather than patching third-party TLS
code or changing dependency providers.

The actual signature delta adds collect_set_merge_extract, which
normalizes to collect_set even though Spark fuzzing excludes that
aggregate. Export the eligible names through a list-only fuzzer mode
that uses the same registration and only/skip filtering as execution.
Consume that list before selecting the biased job, without duplicating
the C++ exclusion policy in Python or relaxing the runner's error.

Keep compatibility checks over all signatures, reject empty or
inconsistent eligibility exports, and retain regular-fuzzer coverage.
Load PyVelox only for signature export commands so the pure comparison
tests run against real DeepDiff without a mocked extension. Add focused
tests for excluded companions, mixed changes, incompatibilities,
invalid eligibility exports, and unchanged Presto behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 30, 2026
## Summary
Remove the Spark eligibility-listing mode, signature-bias filtering, added
signature tests, and scheduled-workflow edits. Restore these paths exactly
to the original PR baseline. Retain the FBOS version/checksum updates and
the compiler, xxhash, OpenSSL, and GoogleMock build-compatibility fixes.

## Prompting Intent
Keep this PR focused on the dependency version upgrade. The engineer
explicitly prohibited pipeline and test-coverage changes and requested
removal of unnecessary changes, rather than relocating those changes into
another script.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Change being reduced: 68778c5
- Original PR baseline: facebookincubator@5df6f9b
- Spark failure: https://github.com/facebookincubator/velox/actions/runs/41084854188/job/124175927004
- macOS dependency failure: https://github.com/facebookincubator/velox/actions/runs/41084854386/job/124163673084

## Rationale
Moving eligibility discovery into the existing Python CLI would still
change bias selection and which fuzzer jobs execute, even without a YAML
diff. Remove that policy change entirely instead. Preserve the original
workflow, signature comparison behavior, only/skip filtering, and test
cases; do not substitute a different run to avoid the reported failure.

The reported Spark empty-selection failure remains unresolved by this
cleanup. The previously requested dependency-resolution and link fixes
remain intact because they repair compilation without changing test
coverage. Publish this as a normal follow-up without rewriting history.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
meta-codesync Bot pushed a commit that referenced this pull request Oct 30, 2026
Homebrew's unversioned `openssl` formula has pointed at OpenSSL 4 since 2026-09-27 (Homebrew/homebrew-core 9a12b94a88), and that formula is no longer keg-only, so OpenSSL 4 headers and libraries are linked under `$(brew --prefix)`. The macos-15 runner image 20261028.0489.1 ships it, and since it rolled out every `macOS Build` run fails while installing dependencies, because Folly at `FB_OS_VERSION` (v2026.09.07.00) reads `ASN1_STRING` fields directly and assigns `X509_get_subject_name()` to a non-const pointer, both of which OpenSSL 4 removed:

    folly/io/async/ssl/OpenSSLUtils.cpp:126:62: error: member access into incomplete type 'ASN1_OCTET_STRING' (aka 'asn1_string_st')

Moving `FB_OS_VERSION` forward is not enough on its own: Folly supports OpenSSL 4 from v2026.10.12.00 (wangle from v2026.09.21.00), but the gRPC 1.48.1 that Velox bundles for its tests fails the same way (`src/core/tsi/ssl_transport_security.cc`; fixed upstream only in gRPC 1.85.0), as the FBOS bump in #19909 shows (run 41084854386).

- `scripts/setup-macos.sh` installs `openssl@3` and exports `OPENSSL_ROOT_DIR` for it, so every dependency it builds uses OpenSSL 3. A caller that already set `OPENSSL_ROOT_DIR` keeps its choice.
- `cmake_install` also passes `OPENSSL_ROOT_DIR` as a cache variable. fbthrift ships its own `FindOpenSSL.cmake` that ignores the environment and otherwise runs `brew --prefix openssl`, which now resolves to OpenSSL 4.
- The macOS and pyvelox workflows carry `OPENSSL_ROOT_DIR` into their later steps, which run in a new shell.
- `CMakeLists.txt` defaults a macOS configure to Homebrew's OpenSSL 3 when no OpenSSL root was chosen (and warns when `openssl@3` is missing), so a plain `make` in a fresh shell keeps matching the dependencies built by the script.
- On macOS the OpenSSL include directory is added first: `Folly::folly_deps` exports `$(brew --prefix)/include` ahead of it, where OpenSSL 4's headers now live, so Velox sources that include `<openssl/*.h>` would otherwise compile against OpenSSL 4 headers while linking OpenSSL 3.

This can be reverted once the pinned Folly and gRPC releases both build against OpenSSL 4.

Related: #19936 and #19257 pin `openssl@3` the same way. This PR additionally covers fbthrift, whose own `FindOpenSSL.cmake` ignores `OPENSSL_ROOT_DIR` from the environment (the macOS run of #19936, 41096009930, configures fbthrift against `/opt/homebrew/opt/openssl@4` while folly, fizz, wangle and mvfst use `openssl@3`), and the include order on Velox's own compile lines.

Note: with dependencies building again, the macOS Build step still fails on `main` with `VectorIndexBuildTest.cpp:17:10: fatal error: 'gmock/gmock.h' file not found` (from #19886); #19942 fixes that, so the macOS job here can only turn green once it has landed and `main` is merged into this branch.

Differential Revision: D128692789

Pull Request resolved: #19945
tinadu1018 added a commit to tinadu1018/velox that referenced this pull request Oct 30, 2026
## Summary
Merge upstream main at 8f6c946 into the
FBOS upgrade branch. Align the signature-comparison baseline with upstream
changes already present in CI's merged checkout.

Keep upstream's OpenSSL 3 selection and targeted VectorIndex GoogleMock
linkage. Remove our superseded OpenSSL selection block, duplicate setup
export, and broad aggregates-test GoogleMock dependency. The remaining
PR diff contains only the dependency upgrade and its FBThrift/xxhash
compatibility fixes.

## Prompting Intent
Fix the reported Fuzzer Jobs failures reasonably without pipeline,
test-selection, coverage, or unnecessary component changes. The engineer
explicitly approved a normal merge of upstream main, without history
rewriting or force-pushing, and requested inspection of macOS 15 Debug.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Failed Fuzzer Jobs: https://github.com/facebookincubator/velox/actions/runs/41132095439
- Baseline and contender provenance: https://github.com/facebookincubator/velox/actions/runs/41132095439/job/124325288109
- Upstream timezone signature change: facebookincubator#19632
- Upstream collect_set companion: facebookincubator#19894
- Upstream OpenSSL fix: facebookincubator#19945
- Upstream targeted GoogleMock fix: facebookincubator#19942
- Current macOS Debug job: https://github.com/facebookincubator/velox/actions/runs/41132095473/job/124331293031
- Presto mismatch: https://github.com/facebookincubator/velox/actions/runs/41132095439/job/124333376983

## Rationale
CI compared signatures saved for merge base 5df6f9b with a contender
containing upstream commits c853cce and b8e1609. Those upstream commits
intentionally removed at_timezone_convert and added collect_set_merge_extract.
Do not restore removed functions, filter signatures, modify skip lists, or
replace the failing jobs. Updating the branch baseline avoids attributing
inherited upstream changes to this dependency upgrade.

Upstream now supplies the macOS build fixes, so preserve its implementation
instead of keeping duplicate or broader local workarounds. Retain the
version/checksum updates and the proven compiler-export and xxhash-target
compatibility fixes. No independent fix for the saved Presto value/null
mismatch is claimed by this merge.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
@kgpai

kgpai commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

cc: @majetideepak @czentgr

@kgpai kgpai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase and fix the merge conflict.

tinadu1018 and others added 8 commits November 2, 2026 14:38
Upgrade the shared Facebook open-source dependency group from
v2026.09.07.00 to v2026.10.26.00: Folly, Fizz, Wangle, mvfst, and FBThrift.
Update the bundled Folly version and checksum, refresh its CMake patches,
and synchronize the vendored FBThrift helper with the selected release.

Prepare a separate Folly dependency-upgrade PR from the latest official
Velox main at branch creation. Update the full FB_OS_VERSION group
together, use earlier upstream upgrade PRs as migration references, and
leave the separate Windows integration changes untouched.

- Folly release:
  https://github.com/facebook/folly/releases/tag/v2026.10.26.00
- Upstream fixed-string literal correction:
  meta/folly#2726
- Coordinated Fizz, Wangle, and mvfst tags:
  https://github.com/facebookincubator/fizz/tree/v2026.10.26.00
  https://github.com/facebook/wangle/tree/v2026.10.26.00
  https://github.com/facebook/mvfst/tree/v2026.10.26.00
- Matching FBThrift release and helper:
  https://github.com/facebook/fbthrift/tree/v2026.10.26.00
  build/fbcode_builder/CMake/FBThriftCppLibrary.cmake
- Previous coordinated Velox upgrades:
  facebookincubator#18968
  facebookincubator#18194
- Shared-version and helper synchronization contract:
  scripts/setup-versions.sh
- Pre-commit review finding: preserve the SYSTEM glog contract declared
  in CMake/Findglog.cmake and the root dependency-source selection.

Keep the Facebook libraries on one release instead of updating Folly
alone. Consume the released upstream fixed-string correction rather
than introducing a Windows-specific Folly source patch.

Adapt the bundled patches to the new CMake layout while retaining
target-based dependency propagation. Use required CONFIG lookup for
bundled glog and required MODULE lookup for SYSTEM glog so library-only
installations remain supported and dependency failures remain explicit.

Match the FBThrift helper to the pinned tag while preserving Velox's
compiler environment wrapper. Keep existing container-transition
workarounds until their documented removal conditions are satisfied.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Keep the coordinated FBOS v2026.10.26.00 upgrade and repair its Linux
integration: export GCC-only coroutine flags conditionally for the consuming
compiler, resolve a compiled xxhash target despite cuDF's source-only CPM
redirect, and use one shared system gflags dependency across Folly, glog,
and the Ubuntu shared Velox build.

Ship the FBThrift patch with each affected Docker build context and make
installer patching idempotent while rejecting incompatible dependency source.
Preserve the metadata xxhash link needed by older container installations.

## Prompting Intent
Investigate and fix the failures in GitHub CI run 40787171849 for PR facebookincubator#19909
without downgrading the FBOS release or weakening existing tests and checks.
Review the correction, resolve and validate its findings, then commit and
push the focused change to the existing PR. The engineer stopped further
review after three clean rounds and explicitly requested publication.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Failing CI run: https://github.com/facebookincubator/velox/actions/runs/40787171849
- ASAN job: https://github.com/facebookincubator/velox/actions/runs/40787171849/job/123163347906
- Adapters job: https://github.com/facebookincubator/velox/actions/runs/40787171849/job/123163656672
- Ubuntu job: https://github.com/facebookincubator/velox/actions/runs/40787171849/job/123163347458
- FBThrift release build: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/CMakeLists.txt
- FBThrift package: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/thrift/cmake/FBThriftConfig.cmake.in
- cuDF xxhash recipe: https://github.com/rapidsai/cudf/blob/c9086e0afbf0cb11194a12bb2233b80139eabd09/cpp/cmake/thirdparty/get_xxhash.cmake
- RAPIDS CPM integration: https://github.com/rapidsai/rapids-cmake/blob/037d4c5489cc13f5b378b54197238a6e2cdbff17/rapids-cmake/cpm/find.cmake
- CPM release: https://github.com/cpm-cmake/CPM.cmake/releases/tag/v0.42.0
- CMake package redirects: https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure
- Earlier metadata dependency issue: meta/fbthrift#651

## Rationale
Fix the producer's exported compiler condition instead of suppressing Clang
warnings or editing installed package exports. Directly include the existing
xxhash discovery module behind a target guard because CMake package redirects
take precedence even over MODULE lookups; this also works when a parent project
created the redirect. Keep CPM configuration and cuDF's source-only target intact,
respect existing imported targets, and fail explicitly if the compiled library
or headers are unavailable.

Build Folly shared for both Ubuntu compiler variants and resolve gflags from
SYSTEM so Folly, glog, and Velox do not register separate static and shared
copies of the same flags. Keep the dependency release and test selection intact.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Remove the broader CI and build-integration changes from this dependency
upgrade. Restore the original Linux workflow settings, gflags provider,
compiler-specific Folly linkage, xxhash discovery, FBThrift installer,
Dockerfiles, and FBThrift CMake helper.

Keep only the coordinated dependency version metadata, the bundled Folly
version and checksum, and refreshes of its two existing CMake patches.

## Prompting Intent
The engineer requested a version update with only a few small compatibility
changes required by the new Folly release. Existing build settings must not
change, and local-environment failures must not expand the repository change.
Revert the extra changes rather than replace them with other workarounds.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Original upgrade: facebookincubator@6db1162
- Broader CI correction being removed: facebookincubator@75af12c
- Folly release: https://github.com/facebook/folly/releases/tag/v2026.10.26.00
- mvfst compatibility targets: https://github.com/facebook/mvfst/blob/v2026.10.26.00/cmake/MvfstCompatAliases.cmake

## Rationale
Keep the dependency bump reviewable and preserve the existing build policy.
The retained Folly patches are adaptations of existing patches to the new
release's CMake context and dependency variables, not new build settings.
The selected mvfst release still exports the old compatibility target names,
so changing FBThriftCppLibrary's explicit link entries is unnecessary.

This restores every file outside the five-file version/Folly scope to the
pre-upgrade baseline. It narrows the change rather than claiming that the
separately observed CI failures are resolved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Restore the original gflags/glog package lookup calls and diagnostic while
retaining only the variable mappings needed by the new Folly dependency code.
Remove the added CONFIG/REQUIRED restrictions and provider-selection branch.

Preserve the original commented-out replacement text and formatting in the
no-export patch. Change only the hunk coordinates and source context needed
to match the selected Folly release.

## Prompting Intent
The engineer requested minimal changes, preservation of the original code,
and removal of unnecessary formatting differences. Keep the PR limited to
version/checksum updates and small compatibility changes required by Folly,
without changing existing build settings.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Prior scope reduction: facebookincubator@a04678b
- Original patches: https://github.com/facebookincubator/velox/tree/5df6f9b010ab15959eeb83fdecba3437774dd321/CMake/resolve_dependency_modules/folly
- Pinned dependency discovery: https://github.com/facebook/folly/blob/v2026.10.26.00/CMake/folly-deps.cmake
- Pinned install/export rules: https://github.com/facebook/folly/blob/v2026.10.26.00/CMakeLists.txt

## Rationale
Use Velox's existing package discovery and imported targets instead of
introducing a second provider-selection policy. Preserve the system Findglog
module's include information and the existing diagnostic.

The original patch hunks no longer apply to the selected source release,
but their commented replacement text does not need reformatting. Keeping
that text unchanged preserves behavior and removes review-only churn.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Make the no-export patch comment out the selected upstream statements by
prefixing their existing lines with # only. Preserve every line break,
indentation level, and blank line in the selected Folly source.

Do not combine the install arguments or split the auto_install_files call.
Keep the same install, header-install, and export statements disabled.

## Prompting Intent
The engineer pointed out that the patch was joining and splitting upstream
lines while commenting them out. Preserve the original upstream lines and
make only the minimum comment-marker changes, without reformatting code or
changing existing build behavior.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Previous patch layout: facebookincubator@aef1ebe
- Selected upstream source: https://github.com/facebook/folly/blob/v2026.10.26.00/CMakeLists.txt

## Rationale
Use the selected upstream source as the formatting reference. Preserving
the older patch's replacement layout had unnecessarily reflowed the newer
source. A line-for-line comment-only transformation preserves that source
layout and the existing no-export behavior without changing other files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Fix the two dependency-compatibility regressions introduced by the
v2026.10.26.00 FBOS upgrade: a GCC-only coroutine option leaking into
Clang consumers and FBThrift's missing Xxhash::xxhash library target
when cuDF registers a source-only CPM package redirect.

## Prompting Intent
The engineer asked to address the PR-introduced failures identified by
the PR's CI Failure Analysis report. Keep the version-upgrade scope
minimal and retain the existing formatting, compiler warning policy,
dependency providers, shared/static settings, and CI build profiles.

## Linked Sources
- PR and upgrade context: facebookincubator#19909
- CI Failure Analysis: facebookincubator#19909 (comment)
- ASAN/UBSAN failure: https://github.com/facebookincubator/velox/actions/runs/40941103139/job/123718883755
- Adapters failure: https://github.com/facebookincubator/velox/actions/runs/40941103139/job/123719208953
- FBThrift target configuration: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/CMakeLists.txt#L61-L76
- cuDF xxhash recipe: https://github.com/rapidsai/cudf/blob/c9086e0afbf0cb11194a12bb2233b80139eabd09/cpp/cmake/thirdparty/get_xxhash.cmake

## Rationale
Make the exported coroutine option conditional on the consumer's
compiler as well as its language instead of suppressing Clang warnings.
Apply the single-line correction through the dependency installer and
copy the patch alongside that installer in existing container stages.

Use the existing xxhash finder directly so the source-only CPM redirect
cannot intercept FBThrift's compiled-library lookup. Export an imported
target with the existing include path and release/debug library choices,
preserving an already-provided target. The pinned FBThrift release now
declares its xxhash target edges, replacing the old raw metadata-link
workaround without changing build settings or cuDF's own xxhash target.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Add GTest::gmock to the shared aggregate-test dependencies so the
VectorIndex test target in CI's merged checkout receives GoogleMock's
headers and library.

## Prompting Intent
The engineer asked to address the macOS missing gmock/gmock.h failure,
commit and push the known fix now, and verify it in the new CI run.
Keep the change minimal and leave CUDA settings and build profiles
unchanged.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- macOS failure: https://github.com/facebookincubator/velox/actions/runs/40995890308/job/123868890791
- Upstream vector-index tests: facebookincubator#19886
- Failing merged target: https://github.com/facebookincubator/velox/blob/633c6f0eff87ccb66d4895a2f74dd377ba564b4f/velox/functions/prestosql/aggregates/tests/CMakeLists.txt
- GoogleMock matcher usage: https://github.com/facebookincubator/velox/blob/633c6f0eff87ccb66d4895a2f74dd377ba564b4f/velox/functions/prestosql/aggregates/tests/VectorIndexBuildTest.cpp

## Rationale
The new VectorIndex test inherits VELOX_AGGREGATES_TEST_DEPS but
requires GoogleMock matchers in addition to GoogleTest. Declare the
dependency in that existing shared list instead of adding global
include paths, changing package providers, or copying upstream-only
test targets into this branch. This one-line change also reaches the
target when GitHub merges the branch with main for CI, without a
local merge or history rewrite.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Keep macOS dependency builds and Velox on Homebrew OpenSSL 3, which
is compatible with the bundled gRPC version. Reject OpenSSL 4 early
on macOS instead of reaching incompatible X509 and ASN1 APIs.

Derive Spark aggregate bias candidates from the fuzzer's own eligible
function list. When only excluded aggregates changed, preserve the
existing regular-fuzzer selection instead of starting an empty
biased run.

## Prompting Intent
The engineer reported the Spark biased aggregate failure selecting
collect_set with seed 6600 and the subsequent macos-15-debug build
failure. Address the actual causes with production logic, preserve
build profiles and existing exclusions, and publish scoped fixes
for verification in new CI. Do not suppress failures, disable tests,
or add speculative CUDA changes.

## Linked Sources
- PR: facebookincubator#19909
- Spark workflow: https://github.com/facebookincubator/velox/actions/runs/41084854188
- Spark failure: https://github.com/facebookincubator/velox/actions/runs/41084854188/job/124175927004
- Real signature artifact: https://github.com/facebookincubator/velox/actions/runs/41084854188/artifacts/12991948431
- macOS failure: https://github.com/facebookincubator/velox/actions/runs/41084854386/job/124163673084
- macOS setup in the failing checkout: https://github.com/facebookincubator/velox/blob/e5add781d8aaacf443740580d6cdb2b0fbb43c2d/scripts/setup-macos.sh
- Homebrew OpenSSL 3 formula: https://formulae.brew.sh/formula/openssl@3
- Homebrew prefix behavior: https://docs.brew.sh/Manpage
- OpenSSL discovery and version ranges: https://cmake.org/cmake/help/latest/module/FindOpenSSL.html

## Rationale
Homebrew's unversioned openssl formula now selects 4.1.0, whose APIs
are incompatible with gRPC 1.48.1. Select the maintained 3.x formula
consistently, honor explicit installation roots, and leave non-Apple
OpenSSL discovery unchanged rather than patching third-party TLS
code or changing dependency providers.

The actual signature delta adds collect_set_merge_extract, which
normalizes to collect_set even though Spark fuzzing excludes that
aggregate. Export the eligible names through a list-only fuzzer mode
that uses the same registration and only/skip filtering as execution.
Consume that list before selecting the biased job, without duplicating
the C++ exclusion policy in Python or relaxing the runner's error.

Keep compatibility checks over all signatures, reject empty or
inconsistent eligibility exports, and retain regular-fuzzer coverage.
Load PyVelox only for signature export commands so the pure comparison
tests run against real DeepDiff without a mocked extension. Add focused
tests for excluded companions, mixed changes, incompatibilities,
invalid eligibility exports, and unchanged Presto behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
tinadu1018 and others added 2 commits November 2, 2026 14:38
## Summary
Remove the Spark eligibility-listing mode, signature-bias filtering, added
signature tests, and scheduled-workflow edits. Restore these paths exactly
to the original PR baseline. Retain the FBOS version/checksum updates and
the compiler, xxhash, OpenSSL, and GoogleMock build-compatibility fixes.

## Prompting Intent
Keep this PR focused on the dependency version upgrade. The engineer
explicitly prohibited pipeline and test-coverage changes and requested
removal of unnecessary changes, rather than relocating those changes into
another script.

## Linked Sources
- Upgrade PR: facebookincubator#19909
- Change being reduced: 68778c5
- Original PR baseline: facebookincubator@5df6f9b
- Spark failure: https://github.com/facebookincubator/velox/actions/runs/41084854188/job/124175927004
- macOS dependency failure: https://github.com/facebookincubator/velox/actions/runs/41084854386/job/124163673084

## Rationale
Moving eligibility discovery into the existing Python CLI would still
change bias selection and which fuzzer jobs execute, even without a YAML
diff. Remove that policy change entirely instead. Preserve the original
workflow, signature comparison behavior, only/skip filtering, and test
cases; do not substitute a different run to avoid the reported failure.

The reported Spark empty-selection failure remains unresolved by this
cleanup. The previously requested dependency-resolution and link fixes
remain intact because they repair compilation without changing test
coverage. Publish this as a normal follow-up without rewriting history.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Preserve the previous merge's scope cleanup after rebasing PR 19909 onto
main at cd11e9e. The README conflict was
resolved during replay by retaining upstream FAISS 1.12.0 alongside the
five FBOS v2026.10.26.00 entries.

Restore the removal of duplicate OpenSSL selection and broad aggregate-test
GoogleMock linkage that was previously recorded only in merge 9d03fae.

## Prompting Intent
The engineer requested immediate rebasing and conflict resolution following
kgpai's approval. Preserve the dependency-upgrade scope, upstream changes,
and existing workflows and test coverage; do not add unrelated fixes.

## Linked Sources
- PR: facebookincubator#19909
- Reviewer request: facebookincubator#19909 (review)
- Rebase target: facebookincubator@cd11e9e
- Previous merge cleanup: 9d03fae

## Rationale
A linear rebase replays the ordinary commits but omits the prior merge
commit and its manual cleanup. Preserve those merge-only edits explicitly
rather than reintroducing obsolete local macOS workarounds. The final tree
must match the independently computed combination of the original PR and
current main, with only the README conflict resolved.

Keep the existing dependency versions, checksum, Folly patch refreshes,
FBThrift compiler-export repair, and xxhash compatibility changes. Do not
squash the existing commits or include the optional Docker cleanup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
@tinadu1018
tinadu1018 force-pushed the users/tiadu/update-fb-oss-20261026 branch from 9d03fae to 87691c3 Compare November 2, 2026 22:45
@tinadu1018

Copy link
Copy Markdown
Contributor Author

Please rebase and fix the merge conflict.

thanks, rebased and resolved the conflicts.

+#)

-auto_install_files(folly ${FOLLY_DIR} ${hfiles})
+#auto_install_files(folly ${FOLLY_DIR} ${hfiles})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we remove this instead of commenting?

- FILE folly-targets.cmake
- COMPONENT dev
-)
+#install(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove instead of comment.

Comment thread CMake/FindXxhash.cmake Outdated

if(Xxhash_FOUND)
message(STATUS "Found xxhash: ${Xxhash_LIBRARY}")
if(NOT TARGET Xxhash::xxhash)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change related to this PR?

Comment thread scripts/setup-fbthrift.patch Outdated
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
target_compile_options(${target} ${visibility}
- $<$<COMPILE_LANGUAGE:CXX>:-fcoroutines>)
+ $<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fcoroutines>)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change? Should this go to fbthrift?

Comment thread CMakeLists.txt Outdated
PROPERTY INTERFACE_LINK_LIBRARIES "${VELOX_XXHASH_LIB}"
)
# cuDF's source-only xxhash CPM redirect cannot provide FBThrift's library target.
if(NOT TARGET Xxhash::xxhash)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need FBThrift always. Is this a cuDF bug?

tinadu1018 and others added 2 commits November 3, 2026 15:40
## Summary
Remove Folly's disabled installation and export commands in the bundled
patch instead of retaining commented-out copies. Limit the compiled xxhash
lookup workaround to cuDF builds, leaving ordinary FBThrift dependency
discovery to its package configuration.

## Prompting Intent
Address majetideepak's latest review of the FBOS version upgrade and try
removing both the FBThrift compiler patch and the xxhash workaround. Keep
the upgrade narrowly scoped without changing pipelines, compiler policy,
runtime behavior, or test selection.

## Linked Sources
- Review: facebookincubator#19909 (review)
- Folly installation cleanup: facebookincubator#19909 (comment)
- Folly export cleanup: facebookincubator#19909 (comment)
- Xxhash target question: facebookincubator#19909 (comment)
- FBThrift patch question: facebookincubator#19909 (comment)
- FBThrift/cuDF dependency question: facebookincubator#19909 (comment)
- Pinned FBThrift producer: https://github.com/facebook/fbthrift/blob/v2026.10.26.00/CMakeLists.txt
- Pinned cuDF xxhash recipe: https://github.com/rapidsai/cudf/blob/c9086e0afbf0cb11194a12bb2233b80139eabd09/cpp/cmake/thirdparty/get_xxhash.cmake

## Rationale
Fresh real-dependency removal experiments reproduced both compatibility
failures. Unpatched GCC-built FBThrift exports -fcoroutines to Clang
consumers, failing the existing -Werror build. The compiler-specific export
fix belongs upstream in FBThrift but remains necessary for the pinned
release; retaining it avoids suppressing warnings or changing compilers.

Without cuDF, FBThrift resolves its compiled xxhash target itself. With
cuDF's source-only CPM package redirect, generation fails because
FBThrift::thriftmetadata references the missing Xxhash::xxhash target.
Keep the real imported-target repair only for that cuDF configuration.
FBThrift discovery remains outside the xxhash-target condition; the
pre-existing VELOX_NEEDS_FBTHRIFT gate is unchanged.

Both Folly patches apply to the checksum-verified release, and removing
the commented blocks preserves the active CMake commands.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
## Summary
Remove the FBThrift compiler patch and its installer and Docker plumbing.
Restore FindXxhash.cmake and the root FBThrift discovery block to the
rebased main baseline, including its pre-existing metadata linkage.
Preserve the FBOS version upgrade and the reviewed Folly patch cleanup.

## Prompting Intent
The engineer explicitly requested reverting the additions questioned by
the reviewer, even though focused local Linux removal experiments had
reproduced compatibility failures. Publish a separate removal-only
revision so actual CI can provide evidence. Do not post comments or tag
reviewers, and do not change unrelated pipelines or the Hadoop installer.

## Linked Sources
- PR: facebookincubator#19909
- Review: facebookincubator#19909 (review)
- Xxhash question: facebookincubator#19909 (comment)
- FBThrift patch question: facebookincubator#19909 (comment)
- FBThrift/cuDF question: facebookincubator#19909 (comment)
- Preserved Folly cleanup: facebookincubator#19909 (comment)
- Preserved Folly export cleanup: facebookincubator#19909 (comment)
- Prior CI run: https://github.com/facebookincubator/velox/actions/runs/41741101472

## Rationale
Follow the engineer's confirmed review workflow rather than retaining
questioned compatibility additions on the basis of local reproductions.
The six-path reversal returns the PR to five version/patch files without
changing the dependency version, build profiles, or test selection.

The prior CI run failed extracting the Hadoop download, before compiling
Velox; it is not evidence that these removals caused a compiler failure.
This commit deliberately leaves that separate dependency-download issue
unchanged and makes no claim that the removal revision passes CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
@tinadu1018

Copy link
Copy Markdown
Contributor Author

@majetideepak I reverted the FBThrift patch and additional xxhash/CMake changes in 7f83cf09b, preserving the FBOS version upgrade and Folly patch updates.

Linux CI now reproduces two failures:

  1. ASAN/UBSAN build: Clang fails with argument unused during compilation: '-fcoroutines' [-Werror,-Wunused-command-line-argument].

  2. Release build with adapters: CMake generation fails because FBThrift::thriftmetadata and FBThrift::thriftcpp2 reference the missing target Xxhash::xxhash.

@majetideepak

Copy link
Copy Markdown
Collaborator

Clang fails with argument unused during compilation: '-fcoroutines'

Looks like we wait until next Monday and pick your fix meta/fbthrift#720

CMake generation fails because FBThrift::thriftmetadata and FBThrift::thriftcpp2 reference the missing target Xxhash::xxhash.

Seems to be linked with cudf. I am looking into this.

@czentgr

czentgr commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Yes, let's wait for the fix. Also we need to run a test with prestodb when this comes in because presto also uses some FBOS dependencies and we want to make sure we don't need additional changes here.

@majetideepak

Copy link
Copy Markdown
Collaborator

Here is the better fix for xxhash. I will try to fix this in cuDF.

@@ -571,6 +571,22 @@ if(ENABLE_ALL_WARNINGS)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra ${KNOWN_WARNINGS}")
 endif()
 
+# FBThrift is required by remote functions and LocalRunnerService.
+if(VELOX_ENABLE_REMOTE_FUNCTIONS OR VELOX_ENABLE_PARQUET OR VELOX_ENABLE_LOCAL_RUNNER_SERVICE)
+  set(VELOX_NEEDS_FBTHRIFT ON)
+endif()
+
+# Find FBThrift before cuDF is added. cuDF adds xxhash through CPM, which
+# redirects every later find_package(Xxhash) to an empty stub, so FBThrift
+# would never run its own FindXxhash.cmake and would lack Xxhash::xxhash.
+# TODO: Move this to use resolve_dependency(). For some reason, FBThrift
+# requires clients to explicitly install fizz and wangle.
+if(VELOX_NEEDS_FBTHRIFT)
+  find_package(fizz CONFIG REQUIRED)
+  find_package(wangle CONFIG REQUIRED)
+  find_package(FBThrift CONFIG REQUIRED)
+endif()
+
 if(VELOX_ENABLE_WAVE OR VELOX_ENABLE_CUDF)
   enable_language(CUDA)
   # Use same C++ standard throughout
@@ -777,18 +793,7 @@ if(${VELOX_BUILD_TESTING})
   velox_resolve_dependency(gRPC)
 endif()
 
-# FBThrift is required by remote functions and LocalRunnerService.
-if(VELOX_ENABLE_REMOTE_FUNCTIONS OR VELOX_ENABLE_PARQUET OR VELOX_ENABLE_LOCAL_RUNNER_SERVICE)
-  set(VELOX_NEEDS_FBTHRIFT ON)
-endif()
-
-# TODO: Move this to use resolve_dependency(). For some reason, FBThrift
-# requires clients to explicitly install fizz and wangle.
 if(VELOX_NEEDS_FBTHRIFT)
-  find_package(fizz CONFIG REQUIRED)
-  find_package(wangle CONFIG REQUIRED)
-  find_package(FBThrift CONFIG REQUIRED)
-
   # Workaround for a missing edge in the system-installed FBThrift's
   # exported CMake graph. At the pinned FB_OS_VERSION, fbthrift's own
   # `target_link_libraries(thriftmetadata PUBLIC ...)` block does not

@majetideepak

majetideepak commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

We probably can cleanup the Workaround for a missing edge in the system-installed FBThrift's as well.

@majetideepak

Copy link
Copy Markdown
Collaborator

For xxhash, I opened the real fix here NVIDIA/cudf#25124. But the above workaround works for now. We don't have to patch cuda or wait for a new release.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants