Fix find_package(FBThrift) failure#651
Open
kou wants to merge 1 commit into
Open
Conversation
facebook@a6e835d added `find_dependency(Xxhash REQUIRED)`. It uses our `build/fbcode_builder/CMake/FindXxhash.cmake` but our `FindXxhash.cmake` isn't installed. So `find_dependency(Xxhash REQUIRED)` is failed when `xxHash/xxHashConfig.cmake` isn't installed by xxHash. For example, xxHash package in CentOS 9 stream doesn't install `xxHash/xxHashConfig.cmake`. See also: facebookincubator/velox#13153 We can fix this by install our `FindXxhash.cmake` and use it in `FBThriftConfig.cmake`.
| DESTINATION ${CMAKE_INSTALL_DIR} | ||
| ) | ||
| # This is used in FBThriftConfig.cmake. | ||
| install( |
There was a problem hiding this comment.
@kou will this work outside of fbcode_builder?
Contributor
Author
There was a problem hiding this comment.
Yes. Because https://github.com/facebook/fbthrift/blob/main/build/fbcode_builder/CMake/FindXxhash.cmake uses only built-in CMake features.
There was a problem hiding this comment.
But the path hardcodes fb specific infra path ../build/fbcode_builder?
Contributor
Author
There was a problem hiding this comment.
You refer ../build/fbcode_builder in https://github.com/facebook/fbthrift/pull/651/files#diff-72e4eecafeb87f57668ee019db6f11aa91d85647850c63e4bfb4d0ae2c38341eR43 , right?
We can use ../build/fbcode_builder in install(FILES) here because it's a static path that is only used in this project. It doesn't depend on installed system and projects that use this such as Velox. Because users including Velox use only DESTINATION path (${CMAKE_INSTALL_DIR}) via find_package(FBThrift).
pratikpugalia
pushed a commit
to pratikpugalia/velox
that referenced
this pull request
May 15, 2026
…ld issues
Summary:
Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Five profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug.
## The bash export bug
The previous ubuntu-debug step did:
```bash
EXTRA_CMAKE_FLAGS=(...) # array
export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar
```
Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time.
The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes.
`ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. Migrated as part of this diff.
## Adoption in this diff
| File | Change |
|---|---|
| `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. |
| `.github/workflows/macos.yml` | Configure Build step sources the script. |
| `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script. **Fixes the silent-export bug** — declared `-D` flags reach cmake for the first time. The existing inline list matches the `ubuntu-debug` profile exactly, so direct adoption works. |
## Latent ubuntu-debug build fixes
With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed:
1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve.
2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally).
3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release.
`REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld.
Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 15, 2026
…ld issues (facebookincubator#17531) Summary: Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Five profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug. ## The bash export bug The previous ubuntu-debug step did: ```bash EXTRA_CMAKE_FLAGS=(...) # array export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar ``` Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time. The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes. `ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. Migrated as part of this diff. ## Adoption in this diff | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script. **Fixes the silent-export bug** — declared `-D` flags reach cmake for the first time. The existing inline list matches the `ubuntu-debug` profile exactly, so direct adoption works. | ## Latent ubuntu-debug build fixes With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve. 2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally). 3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release. `REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 15, 2026
…ld issues (facebookincubator#17531) Summary: Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Five profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug. ## The bash export bug The previous ubuntu-debug step did: ```bash EXTRA_CMAKE_FLAGS=(...) # array export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar ``` Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time. The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes. `ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. Migrated as part of this diff. ## Adoption in this diff | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script. **Fixes the silent-export bug** — declared `-D` flags reach cmake for the first time. The existing inline list matches the `ubuntu-debug` profile exactly, so direct adoption works. | ## Latent ubuntu-debug build fixes With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve. 2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally). 3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release. `REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 15, 2026
…ld issues (facebookincubator#17531) Summary: Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Five profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug. ## The bash export bug The previous ubuntu-debug step did: ```bash EXTRA_CMAKE_FLAGS=(...) # array export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar ``` Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time. The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes. `ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. Migrated as part of this diff. ## Adoption in this diff | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script. **Fixes the silent-export bug** — declared `-D` flags reach cmake for the first time. The existing inline list matches the `ubuntu-debug` profile exactly, so direct adoption works. | ## Latent ubuntu-debug build fixes With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve. 2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally). 3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release. `REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 16, 2026
…ld issues (facebookincubator#17531) Summary: Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Five profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug. ## The bash export bug The previous ubuntu-debug step did: ```bash EXTRA_CMAKE_FLAGS=(...) # array export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar ``` Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time. The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes. `ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. Migrated as part of this diff. ## Adoption in this diff | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script. **Fixes the silent-export bug** — declared `-D` flags reach cmake for the first time. The existing inline list matches the `ubuntu-debug` profile exactly, so direct adoption works. | ## Latent ubuntu-debug build fixes With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve. 2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally). 3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release. `REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld. Differential Revision: D105292587
pratikpugalia
pushed a commit
to pratikpugalia/velox
that referenced
this pull request
May 16, 2026
…ld issues
Summary:
Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Six profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `ubuntu-bundled-deps`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug.
## The bash export bug
The previous ubuntu-debug step did:
```bash
EXTRA_CMAKE_FLAGS=(...) # array
export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar
```
Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time.
The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes.
`ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. The list it copy-pasted from ubuntu-debug includes `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON`, but velox's BUNDLED dep mechanism doesn't ship resolvers for the FB_OS suite (fbthrift / fizz / wangle / mvfst). The flag was a contradiction in BUNDLED-deps mode — it required system-installed FB_OS that isn't there on a bare runner. The bash bug masked it; nobody noticed. New `ubuntu-bundled-deps` profile mirrors `ubuntu-debug` minus REMOTE_FUNCTIONS to reflect what's actually testable.
## Adoption in this diff
| File | Change |
|---|---|
| `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. |
| `.github/workflows/macos.yml` | Configure Build step sources the script. |
| `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script with the new `ubuntu-bundled-deps` profile (ubuntu-debug minus REMOTE_FUNCTIONS, since velox doesn't bundle the FB_OS suite). Install Dependencies step is unchanged (`install_apt_deps && install_faiss_deps`) — without REMOTE_FUNCTIONS, no need for fbthrift/fizz/wangle/mvfst from `install_velox_deps`. |
## Latent ubuntu-debug build fixes
With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed:
1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve.
2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally).
3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release.
`REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld.
Differential Revision: D105292587
pratikpugalia
pushed a commit
to pratikpugalia/velox
that referenced
this pull request
May 16, 2026
…ld issues
Summary:
Adds `.github/scripts/cmake-flags.sh`, a sourced bash helper that populates a `CMAKE_FLAGS` array per `BUILD_PROFILE`. Six profiles: `adapters`, `dep-graph`, `ubuntu-debug`, `ubuntu-bundled-deps`, `fedora-debug`, `macos`. Eliminates several copies of inline `EXTRA_CMAKE_FLAGS=(...)` arrays scattered across workflows, makes per-profile flag changes a one-line edit, and (critically) fixes a long-standing bash bug.
## The bash export bug
The previous ubuntu-debug step did:
```bash
EXTRA_CMAKE_FLAGS=(...) # array
export EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS[*]}" # tries to export same name as scalar
```
Bash silently refuses to flatten an array variable into a scalar via `export VAR=...` when `VAR` is already declared as an array, so the export does nothing — the variable doesn't make it into the env, `make` reads `${EXTRA_CMAKE_FLAGS}` as empty, and cmake runs with **none** of the intended `-D` flags. ubuntu-debug has been silently building with cmake defaults for a long time.
The new pattern uses a different name for the array (`CMAKE_FLAGS`, set by the script) versus the export (`EXTRA_CMAKE_FLAGS`), avoiding the same-name overwrite. With this fix in place, the previously-ignored flags actually take effect — which exposed three real link-order bugs that this diff also fixes.
`ubuntu-bundled-deps.yml` had the same shape of bug (different variant — declared the array but never even attempted to export it), so it was also silently dropping all its `-D` flags. The list it copy-pasted from ubuntu-debug includes `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON`, but velox's BUNDLED dep mechanism doesn't ship resolvers for the FB_OS suite (fbthrift / fizz / wangle / mvfst). The flag was a contradiction in BUNDLED-deps mode — it required system-installed FB_OS that isn't there on a bare runner. The bash bug masked it; nobody noticed. New `ubuntu-bundled-deps` profile mirrors `ubuntu-debug` minus REMOTE_FUNCTIONS to reflect what's actually testable.
## Adoption in this diff
| File | Change |
|---|---|
| `.github/workflows/linux-build-base.yml` | adapters / ubuntu-debug / fedora-debug "Make Build" steps now `source .github/scripts/cmake-flags.sh`. ubuntu-debug also gets `uv tool install --force cmake@3.31.1` so `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY` is honored (added in CMake 3.31). ubuntu-debug env adds `faiss_SOURCE: BUNDLED` mirroring adapters/fedora-debug. |
| `.github/workflows/macos.yml` | Configure Build step sources the script. |
| `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script with the new `ubuntu-bundled-deps` profile (ubuntu-debug minus REMOTE_FUNCTIONS, since velox doesn't bundle the FB_OS suite). Install Dependencies step is unchanged (`install_apt_deps && install_faiss_deps`) — without REMOTE_FUNCTIONS, no need for fbthrift/fizz/wangle/mvfst from `install_velox_deps`. |
## Latent ubuntu-debug build fixes
With ubuntu-debug actually applying `-DVELOX_BUILD_SHARED=ON` + `-DVELOX_MONO_LIBRARY=ON` + `-DVELOX_ENABLE_REMOTE_FUNCTIONS=ON` + `-DVELOX_ENABLE_PARQUET=ON` for the first time, three system-installed CMake config bugs surfaced. Fixed:
1. **`CMake/resolve_dependency_modules/gflags.cmake`**: force `GFLAGS_BUILD_STATIC_LIBS=ON` unconditionally. The container's pre-installed folly was linked against `gflags_static` (its INTERFACE_LINK_LIBRARIES references it). With `VELOX_BUILD_SHARED=ON`, `cmake_dependent_option` forces `VELOX_BUILD_STATIC=OFF`, which previously meant BUNDLED gflags only emitted shared variants and CMake fell back to a literal `-lgflags_static` that the linker couldn't resolve.
2. **`CMake/FindArrow.cmake`**: declare `INTERFACE_LINK_LIBRARIES arrow` on the imported `arrow_testing` target. Without this edge in the dependency graph, CMake doesn't know to position `arrow_testing` before `arrow` in the link line, ld processes `libarrow.a` before any reference to `arrow::ipc::internal::json::*` symbols exists, and `velox_dwio_arrow_parquet_writer_test` fails to link. Visible only with `MONO=ON` (where libvelox doesn't drag those symbols in incidentally).
3. **`CMakeLists.txt`** (inline after `find_package(FBThrift)`): append xxhash to `FBThrift::thriftmetadata`'s INTERFACE_LINK_LIBRARIES via `set_property APPEND`. At the pinned `FB_OS_VERSION`, fbthrift's own `target_link_libraries(thriftmetadata PUBLIC ...)` block does not declare an xxhash dep on `thriftmetadata` — the conditional `if(Xxhash_FOUND)` block was added to fbthrift's CMakeLists *after* this version. The compiled `libthriftmetadata.a` still calls `XXH3_64bits()` (header is on disk via xxhash-devel/libxxhash-dev), so the binary needs xxhash but the exported `FBThriftTargets.cmake` doesn't declare it — surfaces as `undefined reference to symbol 'XXH3_64bits'` on `velox_functions_remote_client_test`. Tracking: [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). Removable when `FB_OS_VERSION` is bumped past the commit that added the `if(Xxhash_FOUND)` block AND the velox-dev container is rebuilt against that fixed fbthrift release.
`REORDER_FREELY` (enabled by the cmake bump) plus the three new edges in the dependency graph allow CMake to position the static archives correctly for ld.
Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 19, 2026
…ld issues (facebookincubator#17531) Summary: ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, three CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMake/FindArrow.cmake`** (and `CMake/resolve_dependency_modules/arrow/CMakeLists.txt` for BUNDLED arrow): Adds `INTERFACE_LINK_LIBRARIES arrow` to `arrow_testing` target, ensuring proper link order and fixing test failures. 3. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 19, 2026
…ld issues (facebookincubator#17531) Summary: Pull Request resolved: facebookincubator#17531 ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, three CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMake/FindArrow.cmake`** (and `CMake/resolve_dependency_modules/arrow/CMakeLists.txt` for BUNDLED arrow): Adds `INTERFACE_LINK_LIBRARIES arrow` to `arrow_testing` target, ensuring proper link order and fixing test failures. 3. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 19, 2026
…ld issues (facebookincubator#17531) Summary: ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, two CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 19, 2026
…ld issues (facebookincubator#17531) Summary: ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, two CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Differential Revision: D105292587
pratikpugalia
added a commit
to pratikpugalia/velox
that referenced
this pull request
May 19, 2026
…ld issues (facebookincubator#17531) Summary: ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, three CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMake/FindArrow.cmake`** (and `CMake/resolve_dependency_modules/arrow/CMakeLists.txt` for BUNDLED arrow): Appends `${ARROW_LIB}` (path, not target) to `arrow_testing`'s `INTERFACE_LINK_LIBRARIES`. The path-based form ensures libarrow.a appears after libarrow_testing.a in the link line — required so `ld --as-needed` resolves `arrow::ipc::internal::json::*` and `arrow::Initialize` referenced from `gtest_util.cc.o`. **Workaround for an ODR violation**: the velox-dev:adapters image installs both Apache Thrift (at `/usr/local/include/thrift/transport/`) and FBThrift (at `/usr/local/include/thrift/lib/cpp/transport/`); both inject `apache::thrift::transport::TMemoryBuffer` into the same namespace with different inline bodies, and using the `arrow` target name here (instead of `${ARROW_LIB}`) reorders CMake's dep graph to drag FBThrift's definition ahead of Apache's, breaking `velox_dwio_arrow_parquet_writer_test` with "Insufficient space in external MemoryBuffer". Proper upstream fix would be FBThrift moving its types out of `apache::thrift::*`. 3. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Differential Revision: D105292587
meta-codesync Bot
pushed a commit
to facebookincubator/velox
that referenced
this pull request
May 19, 2026
…ld issues (#17531) Summary: Pull Request resolved: #17531 ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, three CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMake/FindArrow.cmake`** (and `CMake/resolve_dependency_modules/arrow/CMakeLists.txt` for BUNDLED arrow): Appends `${ARROW_LIB}` (path, not target) to `arrow_testing`'s `INTERFACE_LINK_LIBRARIES`. The path-based form ensures libarrow.a appears after libarrow_testing.a in the link line — required so `ld --as-needed` resolves `arrow::ipc::internal::json::*` and `arrow::Initialize` referenced from `gtest_util.cc.o`. **Workaround for an ODR violation**: the velox-dev:adapters image installs both Apache Thrift (at `/usr/local/include/thrift/transport/`) and FBThrift (at `/usr/local/include/thrift/lib/cpp/transport/`); both inject `apache::thrift::transport::TMemoryBuffer` into the same namespace with different inline bodies, and using the `arrow` target name here (instead of `${ARROW_LIB}`) reorders CMake's dep graph to drag FBThrift's definition ahead of Apache's, breaking `velox_dwio_arrow_parquet_writer_test` with "Insufficient space in external MemoryBuffer". Proper upstream fix would be FBThrift moving its types out of `apache::thrift::*`. 3. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Reviewed By: kgpai Differential Revision: D105292587 fbshipit-source-id: ef865e53e4b6594bab64e5f4e0d9f1751b8616c0
Alan-S-Andrade
pushed a commit
to Alan-S-Andrade/velox
that referenced
this pull request
Jun 2, 2026
…ld issues (facebookincubator#17531) Summary: Pull Request resolved: facebookincubator#17531 ### Summary - Introduces `.github/scripts/cmake-flags.sh`, a bash helper that sets a `CMAKE_FLAGS` array per `BUILD_PROFILE` (adapters, ubuntu-debug, ubuntu-bundled-deps, fedora-debug, macos). - Replaces scattered inline `EXTRA_CMAKE_FLAGS=(...)` arrays, enabling one-line per-profile flag changes and fixing a long-standing bash export bug. - Pure refactor for adapters / fedora-debug / macos (flag sets preserved byte-for-byte from main). ubuntu-bundled-deps drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` from its declared set (see Workflow Adoption); previously declared but never reached cmake due to the bash bug. The bash-export fix is what unmasks the latent ubuntu-debug issues addressed below. ### Bash Export Bug Fix - Previously, exporting an array as a scalar with the same name failed silently, causing `make` to receive empty flags and cmake to use defaults. - The new script uses distinct names for the array (`CMAKE_FLAGS`) and export (`EXTRA_CMAKE_FLAGS`), ensuring flags are properly passed. ### Workflow Adoption | File | Change | |---|---| | `.github/workflows/linux-build-base.yml` | "Make Build" steps for adapters, ubuntu-debug, fedora-debug now source the script. Ubuntu-debug installs cmake@3.31.1 for `CMAKE_LINK_LIBRARIES_STRATEGY=REORDER_FREELY`. Ubuntu-debug env adds `faiss_SOURCE: BUNDLED`. | | `.github/workflows/macos.yml` | Configure Build step sources the script. | | `.github/workflows/ubuntu-bundled-deps.yml` | Make Debug Build step sources the script, fixing the silent-export bug. Flags now reach cmake. Profile drops `VELOX_ENABLE_REMOTE_FUNCTIONS=ON` — velox doesn't bundle the FB_OS suite (fbthrift / fizz / wangle / mvfst), so REMOTE_FUNCTIONS has no business in BUNDLED-deps mode (matches de facto behavior since the flag never reached cmake before). | ### Latent Ubuntu-Debug Build Fixes With correct flags applied, three CMake config bugs surfaced and were fixed: 1. **`CMake/resolve_dependency_modules/gflags.cmake`**: Forces `GFLAGS_BUILD_STATIC_LIBS=ON` to resolve linking issues with folly and gflags_static. 2. **`CMake/FindArrow.cmake`** (and `CMake/resolve_dependency_modules/arrow/CMakeLists.txt` for BUNDLED arrow): Appends `${ARROW_LIB}` (path, not target) to `arrow_testing`'s `INTERFACE_LINK_LIBRARIES`. The path-based form ensures libarrow.a appears after libarrow_testing.a in the link line — required so `ld --as-needed` resolves `arrow::ipc::internal::json::*` and `arrow::Initialize` referenced from `gtest_util.cc.o`. **Workaround for an ODR violation**: the velox-dev:adapters image installs both Apache Thrift (at `/usr/local/include/thrift/transport/`) and FBThrift (at `/usr/local/include/thrift/lib/cpp/transport/`); both inject `apache::thrift::transport::TMemoryBuffer` into the same namespace with different inline bodies, and using the `arrow` target name here (instead of `${ARROW_LIB}`) reorders CMake's dep graph to drag FBThrift's definition ahead of Apache's, breaking `velox_dwio_arrow_parquet_writer_test` with "Insufficient space in external MemoryBuffer". Proper upstream fix would be FBThrift moving its types out of `apache::thrift::*`. 3. **`CMakeLists.txt`**: Appends xxhash to `FBThrift::thriftmetadata`'s `INTERFACE_LINK_LIBRARIES`. Required due to missing xxhash dependency in the pinned FBThrift version. See [fbthrift#651](facebook/fbthrift#651), [velox#13153](facebookincubator#13153). > These fixes, along with `REORDER_FREELY`, allow CMake to correctly position static archives for the linker. Reviewed By: kgpai Differential Revision: D105292587 fbshipit-source-id: ef865e53e4b6594bab64e5f4e0d9f1751b8616c0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
a6e835d added
find_dependency(Xxhash REQUIRED). It uses ourbuild/fbcode_builder/CMake/FindXxhash.cmakebut ourFindXxhash.cmakeisn't installed. Sofind_dependency(Xxhash REQUIRED)is failed whenxxHash/xxHashConfig.cmakeisn't installed by xxHash.For example, xxHash package in CentOS 9 stream doesn't install
xxHash/xxHashConfig.cmake.See also: facebookincubator/velox#13153
We can fix this by install our
FindXxhash.cmakeand use it inFBThriftConfig.cmake.