build: Upgrade clp submodule to y-scope/clp@19cd534. - #26
Conversation
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
CMake/resolve_dependency_modules/clp.cmake (1)
40-45: Minor: prefer FetchContent_MakeAvailable unless you intentionally need a subdir-only add.Using Populate + add_subdirectory(core) is fine if you only want CLP core. If the intent is to pick specific components, keep it; otherwise FetchContent_MakeAvailable(clp) simplifies lifecycle and transitive properties.
CMake/resolve_dependency_modules/log_surgeon.cmake (1)
24-28: Guard and align the export fix; don’t assume a bare “GSL” target exists.The install error fix is brittle if the target is named Microsoft.GSL::GSL (common) or absent. Guard the install and rely on an alias created in the GSL module.
-# To address the error: CMake Error: install(EXPORT "log_surgeon-targets" ...) -# includes target "log_surgeon" which requires target "GSL" that is not in any -# export set. -install(TARGETS GSL EXPORT log_surgeon-targets) +# To address: +# "install(EXPORT 'log_surgeon-targets' ...) includes target 'log_surgeon' +# which requires target 'GSL' that is not in any export set." +# We only add GSL to the export if it exists (alias created in microsoft_gsl.cmake). +if (TARGET GSL) + install(TARGETS GSL EXPORT log_surgeon-targets) +endif()CMake/resolve_dependency_modules/zstd.cmake (1)
51-53: Fix: the created zstd target is a no-op INTERFACE; alias the built library instead.
add_library(zstd::libzstd_static INTERFACE IMPORTED)does not link the actual lib and will fail at link time. Create namespace aliases to the real zstd targets produced by the subproject.-FetchContent_MakeAvailable(zstd) -add_library(zstd::libzstd_static INTERFACE IMPORTED) +FetchContent_MakeAvailable(zstd) + +# Create namespaced aliases to actual zstd targets if they exist. +if (TARGET libzstd_static AND NOT TARGET zstd::libzstd_static) + add_library(zstd::libzstd_static ALIAS libzstd_static) +endif() +if (TARGET libzstd_shared AND NOT TARGET zstd::libzstd_shared) + add_library(zstd::libzstd_shared ALIAS libzstd_shared) +endif()CMake/resolve_dependency_modules/microsoft_gsl.cmake (1)
34-35: Create a compatibility alias “GSL” for consumers/export sets that expect it.log_surgeon’s export references “GSL”. Creating a local alias avoids install-time export errors and keeps link interfaces consistent.
FetchContent_MakeAvailable(Microsoft.GSL) + +# Provide a bare 'GSL' alias if the upstream only defines 'Microsoft.GSL::GSL'. +if (TARGET Microsoft.GSL::GSL AND NOT TARGET GSL) + add_library(GSL ALIAS Microsoft.GSL::GSL) +endif()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CMake/resolve_dependency_modules/clp.cmake(1 hunks)CMake/resolve_dependency_modules/log_surgeon.cmake(1 hunks)CMake/resolve_dependency_modules/microsoft_gsl.cmake(1 hunks)CMake/resolve_dependency_modules/zstd.cmake(1 hunks)CMakeLists.txt(1 hunks)velox/connectors/clp/search_lib/ClpQueryRunner.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- velox/connectors/clp/search_lib/ClpQueryRunner.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (1)
CMakeLists.txt (1)
547-552: Good: resolve microsoft_gsl before log_surgeon to satisfy transitive requirements.The ordering ensures GSL is available when log_surgeon configures. This should eliminate “unknown target GSL” at configure time once the alias exists.
After adding the GSL alias in the GSL module, re-configure to confirm the original export error is gone.
| FetchContent_Declare( | ||
| clp | ||
| GIT_REPOSITORY https://github.com/y-scope/clp.git | ||
| GIT_TAG 581bd46198a97a89b174849851cc3f1f2100e466) | ||
| GIT_REPOSITORY https://github.com/anlowee/clp.git | ||
| GIT_TAG 598ba4622e5a968f01d9c9a1a378df0b36e5bf08) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t pin CLP to a personal fork; make repo/tag overridable and default-safe.
Relying on a personal repository risks supply-chain drift and breakage. Make the repo and tag configurable via cache variables, keep your current defaults for now, and enable shallow clones for faster CI.
Apply this diff:
FetchContent_Declare(
clp
- GIT_REPOSITORY https://github.com/anlowee/clp.git
- GIT_TAG 598ba4622e5a968f01d9c9a1a378df0b36e5bf08)
+ # Allow callers/CI to override these safely.
+ GIT_REPOSITORY ${CLP_GIT_REPOSITORY}
+ GIT_TAG ${CLP_GIT_TAG}
+ GIT_SHALLOW TRUE
+ GIT_PROGRESS TRUE)
+
+# Defaults (can be overridden via -D on the CMake command line)
+set(CLP_GIT_REPOSITORY "https://github.com/anlowee/clp.git" CACHE STRING "CLP Git repository URL")
+set(CLP_GIT_TAG "598ba4622e5a968f01d9c9a1a378df0b36e5bf08" CACHE STRING "CLP Git commit/tag to build")To proceed safely, please verify whether the above commit exists in the upstream y-scope/clp repo so we can eventually switch the default:
🏁 Script executed:
#!/bin/bash
# Check if the commit exists upstream; exit code 0 means found.
git ls-remote https://github.com/y-scope/clp.git 598ba4622e5a968f01d9c9a1a378df0b36e5bf08Length of output: 91
I’ll explicitly check the upstream repo for the commit’s existence and report a clear result:
🏁 Script executed:
#!/bin/bash
# Check if the specific commit exists upstream, with explicit exit-code reporting.
echo "Verifying commit 598ba4622e5a968f01d9c9a1a378df0b36e5bf08 in y-scope/clp…"
if git ls-remote --exit-code https://github.com/y-scope/clp.git 598ba4622e5a968f01d9c9a1a378df0b36e5bf08; then
echo "FOUND"
else
echo "NOT FOUND"
fiLength of output: 289
Make CLP repo/tag configurable & note upstream commit isn’t available yet
The proposed override mechanism and shallow-clone flags should still be applied, but the commit 598ba4622e5a968f01d9c9a1a378df0b36e5bf08 isn’t present in https://github.com/y-scope/clp.git. Until upstream imports that SHA, keep your default pointing at anlowee’s fork and coordinate with upstream maintainers to pull in the change.
• File: CMake/resolve_dependency_modules/clp.cmake
• Lines: 16–20
Apply this diff to introduce override variables and enable shallow clones:
FetchContent_Declare(
clp
- GIT_REPOSITORY https://github.com/anlowee/clp.git
- GIT_TAG 598ba4622e5a968f01d9c9a1a378df0b36e5bf08)
+ # Allow callers/CI to override repository and commit/tag
+ GIT_REPOSITORY ${CLP_GIT_REPOSITORY}
+ GIT_TAG ${CLP_GIT_TAG}
+ GIT_SHALLOW TRUE
+ GIT_PROGRESS TRUE)
+
+# Defaults (override via –D on CMake command line)
+set(CLP_GIT_REPOSITORY "https://github.com/anlowee/clp.git" CACHE STRING "CLP Git repository URL")
+set(CLP_GIT_TAG "598ba4622e5a968f01d9c9a1a378df0b36e5bf08" CACHE STRING "CLP Git commit/tag to build")Next steps:
- Merge this refactor so CI and downstream consumers can override the source safely.
- Reach out to y-scope/clp maintainers to import commit 598ba4622e… and then update the default
CLP_GIT_REPOSITORY/CLP_GIT_TAGto point at the upstream repo.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/clp.cmake around lines 16–20, make the CLP
repo and tag configurable by introducing variables (e.g. CLP_GIT_REPOSITORY and
CLP_GIT_TAG) with defaults pointing to anlowee's fork and the current SHA, and
update FetchContent_Declare to use those variables instead of hardcoding the URL
and tag; also enable shallow clone flags (GIT_SHALLOW TRUE and related
FetchContent shallow options) so CI/downstream consumers can override the source
via -DCLP_GIT_REPOSITORY / -DCLP_GIT_TAG, but keep the default pointing at
anlowee until upstream imports the specified commit and then switch the defaults
to the upstream repo/tag per reviewer guidance.
| FetchContent_Declare( | ||
| log_surgeon | ||
| GIT_REPOSITORY https://github.com/y-scope/log-surgeon.git | ||
| GIT_TAG 85d4f2c09c0e55f1fb87cdc8b0f4d13fb1a733e1 | ||
| OVERRIDE_FIND_PACKAGE) | ||
|
|
||
| FetchContent_MakeAvailable(log_surgeon) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm log_surgeon’s public link interface only exposes what consumers need.
If log_surgeon PUBLICly links GSL, exporting GSL is reasonable; otherwise make the GSL link PRIVATE in log_surgeon and drop the export plumbing here.
I can open a follow-up to upstream a change to log_surgeon so its install/export set is self-consistent (avoiding top-level install hacks). Want me to draft it?
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/log_surgeon.cmake around lines 16-23,
confirm whether log_surgeon PUBLICly links GSL; if its public interface does not
require consumers to see GSL, change the log_surgeon target to link GSL PRIVATE
in the log_surgeon project (or adjust the target_link_libraries call there to
PRIVATE) and remove any export/install plumbing here that re-exports GSL; if you
cannot change upstream immediately, override by calling
target_link_libraries(log_surgeon PRIVATE GSL::gsl) or setting
INTERFACE_LINK_LIBRARIES appropriately after FetchContent_MakeAvailable to
ensure GSL is not propagated to consumers, and optionally prepare an upstream
patch to make the install/export set self-consistent so top-level projects don’t
have to hack exports.
| # Version you want to build | ||
| set(VELOX_GSL_BUILD_VERSION 4.0.0) | ||
| set(VELOX_GSL_BUILD_SHA256_CHECKSUM | ||
| f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9) | ||
| set(VELOX_GSL_SOURCE_URL | ||
| "https://github.com/microsoft/GSL/archive/refs/tags/v${VELOX_GSL_BUILD_VERSION}.tar.gz" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Expose GSL version/checksum as cache variables for override.
This makes it easy to test upgrades and ensures reproducibility.
-# Version you want to build
-set(VELOX_GSL_BUILD_VERSION 4.0.0)
-set(VELOX_GSL_BUILD_SHA256_CHECKSUM
- f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9)
+# Version you want to build (override via -D on the CMake command line)
+set(VELOX_GSL_BUILD_VERSION 4.0.0 CACHE STRING "Microsoft.GSL version to build")
+set(VELOX_GSL_BUILD_SHA256_CHECKSUM
+ f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9
+ CACHE STRING "SHA256 checksum of the Microsoft.GSL source tarball")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Version you want to build | |
| set(VELOX_GSL_BUILD_VERSION 4.0.0) | |
| set(VELOX_GSL_BUILD_SHA256_CHECKSUM | |
| f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9) | |
| set(VELOX_GSL_SOURCE_URL | |
| "https://github.com/microsoft/GSL/archive/refs/tags/v${VELOX_GSL_BUILD_VERSION}.tar.gz" | |
| ) | |
| # Version you want to build (override via -D on the CMake command line) | |
| set(VELOX_GSL_BUILD_VERSION 4.0.0 CACHE STRING "Microsoft.GSL version to build") | |
| set(VELOX_GSL_BUILD_SHA256_CHECKSUM | |
| f0e32cb10654fea91ad56bde89170d78cfbf4363ee0b01d8f097de2ba49f6ce9 | |
| CACHE STRING "SHA256 checksum of the Microsoft.GSL source tarball") | |
| set(VELOX_GSL_SOURCE_URL | |
| "https://github.com/microsoft/GSL/archive/refs/tags/v${VELOX_GSL_BUILD_VERSION}.tar.gz" | |
| ) |
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/microsoft_gsl.cmake around lines 16-22, the
GSL version, checksum and source URL are hardcoded; change their definitions to
CMake cache variables so they can be overridden (e.g., use set(... CACHE STRING
"description") for VELOX_GSL_BUILD_VERSION, VELOX_GSL_BUILD_SHA256_CHECKSUM and
VELOX_GSL_SOURCE_URL), include a clear description for each cache entry, and
keep the default values the same so tests can override versions/checksums for
upgrades and reproducible builds.
| # Version you want to build | ||
| set(VELOX_ZSTD_BUILD_VERSION 1.4.8) | ||
| set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM | ||
| 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24) | ||
| set(VELOX_ZSTD_SOURCE_URL | ||
| "https://github.com/facebook/zstd/releases/download/v${VELOX_ZSTD_BUILD_VERSION}/zstd-${VELOX_ZSTD_BUILD_VERSION}.tar.gz" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make zstd version/checksum overridable; consider upgrading from 1.4.8.
Pinning to 1.4.8 may miss bugfixes/CVEs and modern compiler tweaks. At minimum, expose cache variables so CI/users can bump without code changes.
-# Version you want to build
-set(VELOX_ZSTD_BUILD_VERSION 1.4.8)
-set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM
- 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24)
+# Version to build (override via -D on the CMake command line).
+set(VELOX_ZSTD_BUILD_VERSION 1.4.8 CACHE STRING "Zstd version to build")
+set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM
+ 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24
+ CACHE STRING "SHA256 checksum of the zstd source tarball")If CLP/log_surgeon are compatible, bump to a maintained 1.5.x. Please confirm required minimums before we change the default.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Version you want to build | |
| set(VELOX_ZSTD_BUILD_VERSION 1.4.8) | |
| set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM | |
| 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24) | |
| set(VELOX_ZSTD_SOURCE_URL | |
| "https://github.com/facebook/zstd/releases/download/v${VELOX_ZSTD_BUILD_VERSION}/zstd-${VELOX_ZSTD_BUILD_VERSION}.tar.gz" | |
| ) | |
| # Version to build (override via -D on the CMake command line). | |
| set(VELOX_ZSTD_BUILD_VERSION 1.4.8 CACHE STRING "Zstd version to build") | |
| set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM | |
| 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24 | |
| CACHE STRING "SHA256 checksum of the zstd source tarball") | |
| set(VELOX_ZSTD_SOURCE_URL | |
| "https://github.com/facebook/zstd/releases/download/v${VELOX_ZSTD_BUILD_VERSION}/zstd-${VELOX_ZSTD_BUILD_VERSION}.tar.gz" | |
| ) |
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/zstd.cmake around lines 16-22, the zstd
version, checksum and source URL are hardcoded; make these configurable by
changing the set() calls to use CACHE STRING (and a brief docstring) for
VELOX_ZSTD_BUILD_VERSION and VELOX_ZSTD_BUILD_SHA256_CHECKSUM so CI/users can
override them without editing the file, and ensure VELOX_ZSTD_SOURCE_URL
continues to derive from the (now cacheable) version variable; additionally,
after verifying compatibility with CLP/log_surgeon, consider updating the
default VELOX_ZSTD_BUILD_VERSION to a maintained 1.5.x and update the default
checksum accordingly.
| velox_set_source(microsoft_gsl) | ||
| velox_resolve_dependency(microsoft_gsl) | ||
|
|
||
| velox_set_source(log_surgeon) | ||
| velox_resolve_dependency(log_surgeon) | ||
|
|
||
| set(zstd_SOURCE BUNDLED) | ||
| velox_resolve_dependency(zstd) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Order and target normalisation for zstd when HIVE/DWIO and CLP are both enabled.
If HIVE/DWIO is ON, find_package(zstd REQUIRED) runs earlier and may produce targets that differ from the BUNDLED ones set here. Normalise to a single zstd::zstd alias after resolving sources to avoid mismatch.
Minimal patch within this block:
set(zstd_SOURCE BUNDLED)
velox_resolve_dependency(zstd)
+ # Ensure a consistent zstd target alias regardless of source.
+ if (NOT TARGET zstd::zstd)
+ if (TARGET ZSTD::ZSTD)
+ add_library(zstd::zstd ALIAS ZSTD::ZSTD)
+ elseif (TARGET zstd::libzstd_static)
+ add_library(zstd::zstd ALIAS zstd::libzstd_static)
+ elseif (TARGET libzstd_static)
+ add_library(zstd::zstd ALIAS libzstd_static)
+ endif()
+ endif()Please validate a matrix build (CLP=ON/OFF x HIVE=ON/OFF) to ensure a single canonical target is visible in all paths.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| velox_set_source(microsoft_gsl) | |
| velox_resolve_dependency(microsoft_gsl) | |
| velox_set_source(log_surgeon) | |
| velox_resolve_dependency(log_surgeon) | |
| set(zstd_SOURCE BUNDLED) | |
| velox_resolve_dependency(zstd) | |
| velox_set_source(microsoft_gsl) | |
| velox_resolve_dependency(microsoft_gsl) | |
| velox_set_source(log_surgeon) | |
| velox_resolve_dependency(log_surgeon) | |
| set(zstd_SOURCE BUNDLED) | |
| velox_resolve_dependency(zstd) | |
| # Ensure a consistent zstd target alias regardless of source. | |
| if (NOT TARGET zstd::zstd) | |
| if (TARGET ZSTD::ZSTD) | |
| add_library(zstd::zstd ALIAS ZSTD::ZSTD) | |
| elseif (TARGET zstd::libzstd_static) | |
| add_library(zstd::zstd ALIAS zstd::libzstd_static) | |
| elseif (TARGET libzstd_static) | |
| add_library(zstd::zstd ALIAS libzstd_static) | |
| endif() | |
| endif() |
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 547-555, when HIVE/DWIO and CLP can both enable
zstd earlier via find_package, the bundled set(zstd_SOURCE BUNDLED +
velox_resolve_dependency(zstd)) can produce mismatched targets; after resolving
zstd sources, create a single canonical alias target named zstd::zstd that maps
to whichever zstd target was produced (bundle or system) so downstream consumers
always reference the same target; implement the alias normalization immediately
after velox_resolve_dependency(zstd) and ensure it uses the resolved target name
(using an ALIAS or imported target mapping approach), then validate matrix
builds (CLP ON/OFF × HIVE ON/OFF) to confirm only zstd::zstd is visible in all
paths.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CMakeLists.txt (1)
512-519: Normalise zstd alias when DWIO/HIVE is ON; also honour ZSTD::ZSTD.This block doesn’t check for
ZSTD::ZSTDand assumes the bundledzstd::libzstd_*targets exist. Iffind_package(zstd)providedZSTD::ZSTD, the aliasing here won’t work. Align with the canonical aliasing that prefersZSTD::ZSTDif present.Apply:
- if(NOT TARGET zstd::zstd) - if(TARGET zstd::libzstd_static) - set(ZSTD_TYPE static) - else() - set(ZSTD_TYPE shared) - endif() - add_library(zstd::zstd ALIAS zstd::libzstd_${ZSTD_TYPE}) - endif() + if (NOT TARGET zstd::zstd) + if (TARGET ZSTD::ZSTD) + add_library(zstd::zstd ALIAS ZSTD::ZSTD) + elseif (TARGET zstd::libzstd_static) + add_library(zstd::zstd ALIAS zstd::libzstd_static) + elseif (TARGET libzstd_static) + add_library(zstd::zstd ALIAS libzstd_static) + endif() + endif()Follow-up: consider extracting this alias normalisation into a small CMake function and reuse it in both DWIO/HIVE and CLP blocks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
| find_package(zstd REQUIRED) | ||
| add_library(zstd::libzstd_static INTERFACE IMPORTED) | ||
|
|
There was a problem hiding this comment.
Incorrect zstd target: INTERFACE IMPORTED without backing library; normalise to a canonical alias.
Creating zstd::libzstd_static as an INTERFACE IMPORTED target here provides no actual library and will not satisfy link requirements. Also, it diverges from the canonical zstd::zstd alias pattern used elsewhere. Replace this with alias normalisation so CLP always links the same canonical target regardless of source (system or bundled).
Apply:
- find_package(zstd REQUIRED)
- add_library(zstd::libzstd_static INTERFACE IMPORTED)
+ find_package(zstd REQUIRED)
+ # Ensure a consistent zstd canonical alias regardless of source.
+ if (NOT TARGET zstd::zstd)
+ if (TARGET ZSTD::ZSTD)
+ add_library(zstd::zstd ALIAS ZSTD::ZSTD)
+ elseif (TARGET zstd::libzstd_static)
+ add_library(zstd::zstd ALIAS zstd::libzstd_static)
+ elseif (TARGET libzstd_static)
+ add_library(zstd::zstd ALIAS libzstd_static)
+ endif()
+ endif()This mirrors the aliasing used in other paths and ensures downstream link lines only reference zstd::zstd.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find_package(zstd REQUIRED) | |
| add_library(zstd::libzstd_static INTERFACE IMPORTED) | |
| find_package(zstd REQUIRED) | |
| # Ensure a consistent zstd canonical alias regardless of source. | |
| if (NOT TARGET zstd::zstd) | |
| if (TARGET ZSTD::ZSTD) | |
| add_library(zstd::zstd ALIAS ZSTD::ZSTD) | |
| elseif (TARGET zstd::libzstd_static) | |
| add_library(zstd::zstd ALIAS zstd::libzstd_static) | |
| elseif (TARGET libzstd_static) | |
| add_library(zstd::zstd ALIAS libzstd_static) | |
| endif() | |
| endif() |
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 553-555, don't create an INTERFACE IMPORTED
target with no backing library; instead normalize to a single canonical alias
(zstd::zstd) that points to the actual found target. Replace the
add_library(INTERFACE IMPORTED) approach with logic that checks for the real
zstd target emitted by find_package (e.g., the concrete libzstd target provided
by the system or the bundled build) and create an ALIAS named zstd::zstd that
refers to that real target so all downstream linklines reference zstd::zstd
consistently.
|
|
||
| using namespace clp_s; | ||
| using namespace clp_s::search; | ||
| using namespace clp_s::search::clp_search; |
There was a problem hiding this comment.
This is an unsolve symvol and will cause build fail.
| find_package(zstd REQUIRED) | ||
| add_library(zstd::libzstd_static INTERFACE IMPORTED) | ||
|
|
There was a problem hiding this comment.
need by clp_s, other wise it has libzstd_static cannot be found error
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMake/resolve_dependency_modules/geos/geos-cmakelists.patch (1)
27-41: Preserve make check/distcheck UX or provide compat stubs.Removing these targets can break downstream scripts. If they’re intentionally dropped, consider lightweight stubs to maintain compatibility, or document the change.
Proposed minimal stubs:
+#----------------------------------------------------------------------------- +# Compatibility stubs for legacy targets +#----------------------------------------------------------------------------- +if(NOT TARGET check) + add_custom_target(check + COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure + WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) +endif() + +if(NOT TARGET distcheck) + # Keep 'distcheck' as a no-op that ensures 'dist' config exists. + add_custom_target(distcheck DEPENDS dist) +endif()Would this meet your consumers’ expectations, or should we add a release note calling out the removal?
♻️ Duplicate comments (2)
CMake/resolve_dependency_modules/zstd.cmake (1)
16-22: Make zstd version/checksum overridable via cache (repeat of prior note).Expose these as CACHE variables so CI/users can bump without editing the file. Also handy for reproducible mirror pinning.
-# Version you want to build -set(VELOX_ZSTD_BUILD_VERSION 1.4.8) -set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM - 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24) +# Version to build (override with -D on the CMake command line). +set(VELOX_ZSTD_BUILD_VERSION 1.4.8 CACHE STRING "Zstd version to build") +set(VELOX_ZSTD_BUILD_SHA256_CHECKSUM + 32478297ca1500211008d596276f5367c54198495cf677e9439f4791a4c69f24 + CACHE STRING "SHA256 checksum of the zstd source tarball") set(VELOX_ZSTD_SOURCE_URL "https://github.com/facebook/zstd/releases/download/v${VELOX_ZSTD_BUILD_VERSION}/zstd-${VELOX_ZSTD_BUILD_VERSION}.tar.gz" )If CLP/log_surgeon allow, we can follow up with a default bump to 1.5.x.
CMakeLists.txt (1)
511-514: Normalise to a single canonical zstd target after resolution.When multiple paths resolve zstd (DWIO/Hive vs. CLP), downstreams should always link zstd::zstd regardless of source. Add a small alias shim after find_package.
set(zstd_SOURCE BUNDLED) velox_resolve_dependency(zstd) find_package(zstd REQUIRED) + # Canonicalise the zstd target for downstream link consistency. + if (NOT TARGET zstd::zstd) + if (TARGET ZSTD::ZSTD) + add_library(zstd::zstd ALIAS ZSTD::ZSTD) + elseif (TARGET zstd::libzstd_static) + add_library(zstd::zstd ALIAS zstd::libzstd_static) + elseif (TARGET libzstd_static) + add_library(zstd::zstd ALIAS libzstd_static) + endif() + endif()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CMake/Findzstd.cmake(0 hunks)CMake/resolve_dependency_modules/geos/geos-cmakelists.patch(2 hunks)CMake/resolve_dependency_modules/zstd.cmake(1 hunks)CMakeLists.txt(2 hunks)
💤 Files with no reviewable changes (1)
- CMake/Findzstd.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (2)
CMake/resolve_dependency_modules/geos/geos-cmakelists.patch (1)
10-19: Guarding uninstall target avoids duplicate-target errors — nice.This prevents clashes when a superproject already defines an uninstall target. Looks good as-is.
CMake/resolve_dependency_modules/zstd.cmake (1)
57-64: Good: provide concrete alias targets to real lib, not INTERFACE placeholders.Defining zstd::zstd and zstd::libzstd_static as ALIAS to libzstd_static fixes prior “libzstd_static cannot be found” link issues (e.g., clp_s) without inventing hollow IMPORTED INTERFACE targets.
| velox_set_source(microsoft_gsl) | ||
| velox_resolve_dependency(microsoft_gsl) | ||
|
|
||
| velox_set_source(log_surgeon) | ||
| velox_resolve_dependency(log_surgeon) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid duplicate GSL targets exported by log_surgeon. Please verify.
You resolve microsoft_gsl before log_surgeon, which is correct. Ensure log_surgeon only consumes GSL and does not export/install its own GSL target (common cause of duplicate-target/export-set errors).
If duplicates appear, I can provide a patch to make log_surgeon do find_package(GSL CONFIG REQUIRED) and link Microsoft.GSL::GSL without exporting it.
🏁 Script executed:
#!/bin/bash
# Look for GSL targets/installs inside log_surgeon resolver and subdirs.
rg -nC2 -iP '\b(find_package\s*\(\s*GSL\b|Microsoft\.GSL::GSL\b|install\s*\(\s*TARGETS\s+GSL\b|add_library\s*\(\s*GSL\b)'Length of output: 426
Remove GSL export from log_surgeon resolver
In CMake/resolve_dependency_modules/log_surgeon.cmake (line 27), the install(TARGETS GSL EXPORT log_surgeon-targets) is exporting the GSL target and will collide with the upstream Microsoft.GSL export. Instead, have log_surgeon call find_package(GSL CONFIG REQUIRED) and link against Microsoft.GSL::GSL without installing or exporting its own GSL target.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 543-548, the log_surgeon resolver currently
exports/installs a GSL target which will collide with the upstream Microsoft.GSL
export; open CMake/resolve_dependency_modules/log_surgeon.cmake and remove the
install(TARGETS GSL EXPORT log_surgeon-targets) (and any creation/export of a
local GSL target), replace that logic with a find_package(GSL CONFIG REQUIRED)
and ensure the log_surgeon target links to Microsoft.GSL::GSL (without
installing or exporting a second GSL target).
| set(zstd_SOURCE BUNDLED) | ||
| velox_resolve_dependency(zstd) | ||
| find_package(zstd REQUIRED) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY: factor repeated zstd bundling or guard the second block.
zstd is resolved both in DWIO/Hive and CLP paths. It’s harmless, but we can guard or factor to avoid repetition.
set(zstd_SOURCE BUNDLED)
velox_resolve_dependency(zstd)
find_package(zstd REQUIRED)
+ if (NOT TARGET zstd::zstd)
+ if (TARGET ZSTD::ZSTD)
+ add_library(zstd::zstd ALIAS ZSTD::ZSTD)
+ elseif (TARGET zstd::libzstd_static)
+ add_library(zstd::zstd ALIAS zstd::libzstd_static)
+ elseif (TARGET libzstd_static)
+ add_library(zstd::zstd ALIAS libzstd_static)
+ endif()
+ endif()Alternatively, wrap this in a helper (e.g., velox_normalise_zstd_target()) and call it in both places.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(zstd_SOURCE BUNDLED) | |
| velox_resolve_dependency(zstd) | |
| find_package(zstd REQUIRED) | |
| set(zstd_SOURCE BUNDLED) | |
| velox_resolve_dependency(zstd) | |
| find_package(zstd REQUIRED) | |
| if (NOT TARGET zstd::zstd) | |
| if (TARGET ZSTD::ZSTD) | |
| add_library(zstd::zstd ALIAS ZSTD::ZSTD) | |
| elseif (TARGET zstd::libzstd_static) | |
| add_library(zstd::zstd ALIAS zstd::libzstd_static) | |
| elseif (TARGET libzstd_static) | |
| add_library(zstd::zstd ALIAS libzstd_static) | |
| endif() | |
| endif() |
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 549-552, the zstd bundling/resolution block is
duplicated; factor it out or guard the second occurrence to avoid repetition.
Create a small idempotent helper (e.g., velox_normalise_zstd_target()) or wrap
the block with a guard such as checking a variable or TARGET existence (if(NOT
DEFINED zstd_SOURCE) or if(NOT TARGET zstd::zstd)) and move the
set/velox_resolve_dependency/find_package into that helper/guard, then call the
helper from both DWIO/Hive and CLP paths so resolution runs once and is safe to
call multiple times.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
CMake/Findzstd.cmake (4)
26-27: Fix malformed variable expansion in find_library.
${ZSTD_LIBRARYDIR}is misspelled as$ZSTD_LIBRARYDIR}causing a CMake parse/lookup error.Apply:
-find_library(ZSTD_LIBRARY_RELEASE zstd PATHS $ZSTD_LIBRARYDIR}) +find_library(ZSTD_LIBRARY_RELEASE zstd PATHS ${ZSTD_LIBRARYDIR})
45-52: Set IMPORTED_LOCATION per config; don’t assign a list to a single-location prop.
IMPORTED_LOCATIONexpects a single path. Use${ZSTD_LIBRARY}for the generic case and wireIMPORTED_LOCATION_{DEBUG,RELEASE}when available.Apply:
- set_target_properties( - zstd::zstd PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C" - IMPORTED_LOCATION "${ZSTD_LIBRARIES}") + set_target_properties( + zstd::zstd PROPERTIES + IMPORTED_LINK_INTERFACE_LANGUAGES "C" + IMPORTED_LOCATION "${ZSTD_LIBRARY}") + if(ZSTD_LIBRARY_RELEASE) + set_target_properties(zstd::zstd PROPERTIES + IMPORTED_LOCATION_RELEASE "${ZSTD_LIBRARY_RELEASE}") + endif() + if(ZSTD_LIBRARY_DEBUG) + set_target_properties(zstd::zstd PROPERTIES + IMPORTED_LOCATION_DEBUG "${ZSTD_LIBRARY_DEBUG}") + endif()
38-43: Robust static/shared detection.Extension-based check is brittle (e.g., .so.1.5.6, Windows .lib). Prefer a regex for static libs.
Apply:
-get_filename_component(libzstd_ext ${ZSTD_LIBRARY} EXT) -if(libzstd_ext STREQUAL ".a") +if(ZSTD_LIBRARY MATCHES "\\.(a|lib)$") set(libzstd_type STATIC) else() set(libzstd_type SHARED) endif()
18-21: Nit: comment typo.“Glog” → “zstd”.
♻️ Duplicate comments (1)
CMakeLists.txt (1)
539-544: Good: resolve GSL before log_surgeon; watch for GSL re-export from log_surgeon.Ordering is correct. Ensure
log_surgeondoesn’t export/install its own GSL target to avoid duplicate-target/export collisions.Suggested change in CMake/resolve_dependency_modules/log_surgeon.cmake (outside this hunk):
# Inside log_surgeon resolver: find_package(Microsoft.GSL CONFIG REQUIRED) target_link_libraries(log_surgeon PUBLIC Microsoft.GSL::GSL) # Remove any lines that create/install/export a GSL target, e.g.: # install(TARGETS GSL EXPORT log_surgeon-targets)#!/bin/bash # Verify log_surgeon doesn’t define/export GSL targets. rg -nC2 -iP '\b(add_library|install|export)\s*\(\s*GSL\b|Microsoft\.GSL::GSL\b' CMake/resolve_dependency_modules
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CMake/Findzstd.cmake(1 hunks)CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
| # To address the error: CMake Error: install(EXPORT "log_surgeon-targets" ...) | ||
| # includes target "log_surgeon" which requires target "GSL" that is not in any | ||
| # export set. | ||
| install(TARGETS GSL EXPORT log_surgeon-targets) |
There was a problem hiding this comment.
@coderabbitai Open an issue in y-scope/log-surgeon to investigate why this is necessary here.
There was a problem hiding this comment.
🧩 Analysis chain
@kirkrodrigues I've created an issue in the y-scope/log-surgeon repository to investigate the CMake export configuration problem with the GSL target. The issue documents the error message, the current workaround being used in this PR, and references this PR for context.
🏁 Script executed:
#!/bin/bash
# Create an issue in y-scope/log-surgeon repository
gh issue create \
--repo y-scope/log-surgeon \
--title "CMake export configuration issue: GSL target not included in export set" \
--body "## Issue Description
When using log-surgeon as a dependency via FetchContent, there's a CMake export configuration issue where the GSL target is not properly included in the export set, causing build errors.
## Error Message
\`\`\`
CMake Error: install(EXPORT \"log_surgeon-targets\" ...) includes target \"log_surgeon\" which requires target \"GSL\" that is not in any export set.
\`\`\`
## Current Workaround
Downstream projects need to manually export the GSL target:
\`\`\`cmake
install(TARGETS GSL EXPORT log_surgeon-targets)
\`\`\`
## Expected Behavior
The log-surgeon CMake configuration should properly handle GSL dependency exports so downstream projects don't need this manual workaround.
## References
- Related PR: https://github.com/y-scope/velox/pull/26
- Comment: https://github.com/y-scope/velox/pull/26#discussion_r2312595002
- Reported by: @kirkrodrigues" \
--assignee kirkrodrigues
Length of output: 1081
@kirkrodrigues The issue has been successfully created!
Issue facebookincubator#155 in y-scope/log-surgeon is now tracking the CMake export configuration problem: y-scope/log-surgeon#155
The issue documents the specific error, the current workaround being used in this PR, and includes references back to this PR and comment thread for context. It's been assigned to you as requested.
| velox_set_source(microsoft_gsl) | ||
| velox_resolve_dependency(microsoft_gsl) |
There was a problem hiding this comment.
- Can we move this above
nlohmann_jsonso they're alphabetical? - Can we add comments to group all dependencies into "Independent dependencies" and "Dependencies which depend on other dependencies"?
| clp | ||
| GIT_REPOSITORY https://github.com/y-scope/clp.git | ||
| GIT_TAG 581bd46198a97a89b174849851cc3f1f2100e466) | ||
| GIT_TAG 8e0712553dbaebbc86ba70f8aeff3de8484e70df) |
There was a problem hiding this comment.
Can we use the v0.5.0 commit? If not, can we use the latest clp commit?
5cc1c1a to
1fd0b64
Compare
1fd0b64 to
6acd5ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
CMake/Findzstd.cmake (1)
54-62: Drop the defensive TARGET guard; always create the aliases after ensuring zstd::zstd existsIn a FindModule,
zstd::zstdis guaranteed by Lines 45–52. The extraif(TARGET zstd::zstd)risks skipping alias creation and hides real config errors. Create the aliases unconditionally, guarding only the alias targets themselves.-# Create canonical alias targets regardless of how zstd::zstd was provided. -if(TARGET zstd::zstd) - if(NOT TARGET zstd::libzstd_shared) - add_library(zstd::libzstd_shared ALIAS zstd::zstd) - endif() - if(NOT TARGET zstd::libzstd_static) - add_library(zstd::libzstd_static ALIAS zstd::zstd) - endif() -endif() +# Create canonical alias targets regardless of how zstd::zstd was provided. +if(NOT TARGET zstd::libzstd_shared) + add_library(zstd::libzstd_shared ALIAS zstd::zstd) +endif() +if(NOT TARGET zstd::libzstd_static) + add_library(zstd::libzstd_static ALIAS zstd::zstd) +endif()CMakeLists.txt (2)
528-530: Order is correct (GSL before log_surgeon); verify no duplicate GSL exportGood to resolve Microsoft.GSL before
log_surgeon. Please ensure thelog_surgeonresolver does not install/export a GSL target (link toMicrosoft.GSL::GSLviafind_package(GSL CONFIG REQUIRED)), otherwise installs may fail with duplicate-target/export-set issues.
543-546: Add log_surgeon: confirm it consumes, not re-exports, GSLTo avoid target clashes,
log_surgeonshould only linkMicrosoft.GSL::GSLand must not export/install its ownGSLtarget.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 5cc1c1a4620884e67da75f80af55d92b2280208d and 6acd5ff.
📒 Files selected for processing (3)
CMake/Findzstd.cmake(1 hunks)CMake/resolve_dependency_modules/clp.cmake(1 hunks)CMakeLists.txt(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T19:25:37.231Z
Learnt from: kirkrodrigues
PR: y-scope/velox#26
File: CMake/Findzstd.cmake:53-59
Timestamp: 2025-08-31T19:25:37.231Z
Learning: In CMake FindModules, when creating alias targets after ensuring the main target exists, avoid overly defensive guards like `if(TARGET main_target)`. The FindModule should assume the main target exists at that point, and missing targets indicate real configuration issues that shouldn't be silently ignored.
Applied to files:
CMake/Findzstd.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (1)
CMakeLists.txt (1)
518-518: Nice: dependency grouping comment improves navigabilityClear separation of CLP’s independent vs dependent deps. LGTM.
| FetchContent_Declare( | ||
| clp | ||
| GIT_REPOSITORY https://github.com/y-scope/clp.git | ||
| GIT_TAG 581bd46198a97a89b174849851cc3f1f2100e466) | ||
| GIT_TAG 19cd534e629d746395efc64343a60f768b0c9a2d) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make CLP repo/tag overridable and enable shallow clone for faster CI
Avoid hardcoding and allow consumers/CI to override; also add GIT_SHALLOW and progress for speed/observability.
-FetchContent_Declare(
- clp
- GIT_REPOSITORY https://github.com/y-scope/clp.git
- GIT_TAG 19cd534e629d746395efc64343a60f768b0c9a2d)
+set(CLP_GIT_REPOSITORY "https://github.com/y-scope/clp.git" CACHE STRING "CLP Git repository URL")
+set(CLP_GIT_TAG "19cd534e629d746395efc64343a60f768b0c9a2d" CACHE STRING "CLP Git commit/tag to build")
+FetchContent_Declare(
+ clp
+ GIT_REPOSITORY ${CLP_GIT_REPOSITORY}
+ GIT_TAG ${CLP_GIT_TAG}
+ GIT_SHALLOW TRUE
+ GIT_PROGRESS TRUE)Run to confirm the pinned commit exists upstream:
🏁 Script executed:
#!/bin/bash
git ls-remote --exit-code https://github.com/y-scope/clp.git 19cd534e629d746395efc64343a60f768b0c9a2d >/dev/null && echo "Commit found" || echo "NOT FOUND"Length of output: 132
Update CLP_GIT_TAG to a valid existing commit or tag
The SHA “19cd534e629d746395efc64343a60f768b0c9a2d” wasn’t found in the upstream repo—please verify and correct CLP_GIT_TAG to a valid commit or tag before making it overridable and enabling GIT_SHALLOW/GIT_PROGRESS.
🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/clp.cmake around lines 16 to 20, the GIT_TAG
value currently set to "19cd534e629d746395efc64343a60f768b0c9a2d" does not exist
upstream; replace CLP_GIT_TAG with a valid commit SHA or annotated tag from
https://github.com/y-scope/clp (verify by checking the repo history), make the
variable configurable (set a default via set(... CACHE STRING ...) so it can be
overridden), and then enable GIT_SHALLOW and GIT_PROGRESS in the
FetchContent_Declare call (add GIT_SHALLOW TRUE and GIT_PROGRESS TRUE) so
fetching is efficient and shows progress.
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CMake/Findzstd.cmake (2)
26-26: Fix typo and brace error in ZSTD library search.This breaks detection on most systems.
Apply:
-find_library(ZSTD_LIBRARY_RELEASE zstd PATHS $ZSTD_LIBRARYDIR}) +find_library(ZSTD_LIBRARY_RELEASE zstd PATHS ${ZSTD_LIBRARYDIR})
49-51: Use ZSTD_LIBRARY (singular) for IMPORTED_LOCATION.select_library_configurations() sets ZSTD_LIBRARY; using ZSTD_LIBRARIES is incorrect and may mispoint or be empty.
Apply:
set_target_properties( zstd::zstd PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C" - IMPORTED_LOCATION "${ZSTD_LIBRARIES}") + IMPORTED_LOCATION "${ZSTD_LIBRARY}")
♻️ Duplicate comments (1)
CMake/resolve_dependency_modules/log_surgeon.cmake (1)
24-25: Gate the GSL export workaround to avoid configure/install surprises.Keep the workaround but only install if the target actually exists; reduces coupling and failure modes.
Apply:
-# To work around y-scope/log-surgeon#155 -install(TARGETS GSL EXPORT log_surgeon-targets) +# To work around y-scope/log-surgeon#155 +if (TARGET GSL) + install(TARGETS GSL EXPORT log_surgeon-targets) +endif()If we can accept a slightly larger change, I can draft a follow-up that strips GSL from log_surgeon’s INTERFACE on our side during install, removing the need for this export entirely. Want that patch?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CMake/Findzstd.cmake(1 hunks)CMake/resolve_dependency_modules/log_surgeon.cmake(1 hunks)CMakeLists.txt(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T19:25:37.231Z
Learnt from: kirkrodrigues
PR: y-scope/velox#26
File: CMake/Findzstd.cmake:53-59
Timestamp: 2025-08-31T19:25:37.231Z
Learning: In CMake FindModules, when creating alias targets after ensuring the main target exists, avoid overly defensive guards like `if(TARGET main_target)`. The FindModule should assume the main target exists at that point, and missing targets indicate real configuration issues that shouldn't be silently ignored.
Applied to files:
CMake/Findzstd.cmake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (2)
CMake/Findzstd.cmake (1)
54-60: LGTM: canonical aliases for CLP expectations.Creating zstd::libzstd_{shared,static} as aliases to zstd::zstd is a clean compatibility layer.
CMakeLists.txt (1)
518-518: Nice grouping comment.Helps readability around the CLP dependency block.
| FetchContent_Declare( | ||
| log_surgeon | ||
| GIT_REPOSITORY https://github.com/y-scope/log-surgeon.git | ||
| GIT_TAG 85d4f2c09c0e55f1fb87cdc8b0f4d13fb1a733e1 | ||
| OVERRIDE_FIND_PACKAGE) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add progress output; avoid shallow; include FetchContent locally.
Safer logging without the shallow-fetch breakage you hit. Also, make this file self-sufficient.
Apply:
FetchContent_Declare(
log_surgeon
GIT_REPOSITORY https://github.com/y-scope/log-surgeon.git
GIT_TAG 85d4f2c09c0e55f1fb87cdc8b0f4d13fb1a733e1
- OVERRIDE_FIND_PACKAGE)
+ OVERRIDE_FIND_PACKAGE
+ GIT_PROGRESS TRUE)Add near the top (outside this hunk):
include(FetchContent)🤖 Prompt for AI Agents
In CMake/resolve_dependency_modules/log_surgeon.cmake around lines 16-21, make
the file self-sufficient by adding an include(FetchContent) near the top (before
any FetchContent_Declare), and update the FetchContent_Declare call to disable
shallow clones and enable progress (set GIT_SHALLOW to FALSE and PROGRESS to
TRUE) and then call FetchContent_MakeAvailable (or FetchContent_GetProperties +
FetchContent_Populate) so the content is fetched locally; this ensures safer
logging/output and avoids shallow-fetch breakage.
| velox_set_source(microsoft_gsl) | ||
| velox_resolve_dependency(microsoft_gsl) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Order is correct (GSL before log_surgeon); verify target naming consistency.
Ensure microsoft_gsl defines the target name(s) that log_surgeon expects (GSL vs Microsoft.GSL::GSL) to avoid alias churn and export collisions.
Run:
If the resolver exposes only Microsoft.GSL::GSL, add a local ALIAS named GSL where needed to match log_surgeon until upstream fixes land.
Also applies to: 543-546
🏁 Script executed:
#!/bin/bash
# Inspect GSL target names and log_surgeon linkage/exports
rg -n -C2 -g 'CMake/**' -P $'(^|\\b)(add_library|add_library\\s*\\().*GSL|Microsoft\\.GSL::GSL|^\\s*install\\(\\s*TARGETS\\s+GSL\\b|target_link_libraries\\(\\s*log_surgeon\\b.*\\)' Length of output: 420
🏁 Script executed:
#!/bin/bash
# List available dependency resolver modules to find microsoft_gsl
ls CMake/resolve_dependency_modules | grep -i gslLength of output: 71
🏁 Script executed:
#!/bin/bash
# Inspect microsoft_gsl resolver for target definitions and aliases
rg -n -C3 'add_library\(' CMake/resolve_dependency_modules/microsoft_gsl.cmake
rg -n -C3 'add_library.*ALIAS' CMake/resolve_dependency_modules/microsoft_gsl.cmakeLength of output: 166
🏁 Script executed:
#!/bin/bash
# Show contents of the microsoft_gsl resolver to verify what targets it defines or aliases
sed -n '1,200p' CMake/resolve_dependency_modules/microsoft_gsl.cmake
# Search for find_package and alias patterns in the resolver
rg -n -C3 'find_package' CMake/resolve_dependency_modules/microsoft_gsl.cmake
rg -n -C3 'add_library.*ALIAS' CMake/resolve_dependency_modules/microsoft_gsl.cmake
rg -n -C3 'add_library' CMake/resolve_dependency_modules/microsoft_gsl.cmakeLength of output: 1513
Add missing GSL alias in microsoft_gsl resolver
In CMake/resolve_dependency_modules/microsoft_gsl.cmake, immediately after
FetchContent_MakeAvailable(Microsoft.GSL)insert
add_library(GSL ALIAS Microsoft.GSL::GSL)so that the install(TARGETS GSL …) in log_surgeon.cmake succeeds. Applies to both the calls at lines 528–530 and 543–546.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 528–530 (and similarly for the other call at
543–546), the microsoft_gsl resolver does a
FetchContent_MakeAvailable(Microsoft.GSL) but does not create the expected GSL
target alias; add a step immediately after the FetchContent_MakeAvailable call
to create an alias for Microsoft.GSL::GSL using add_library(GSL ALIAS
Microsoft.GSL::GSL) so that downstream install(TARGETS GSL ...) in
log_surgeon.cmake can find the GSL target; apply this insertion in both
locations mentioned.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
build: Upgrade clp submodule to y-scope/clp@19cd534.
kirkrodrigues
changed the title
7c55762
into
y-scope:presto-0.293-clp-connector
3 participants
Description
To upgrade the CLP submodule, we have to add the dependency of
log_surgeonand the dependencies it needs. Also there is aliblzmaerror when buildingmake releasecaused by one line inclp.cmake, which is fixed in this PR as well.Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit