Skip to content

build(clp_s::ffi::sfa): Add search dependencies and make OpenTelemetry and Libarchive optional for downstream WebAssembly. - #2474

Open
Bill-hbrhbr wants to merge 5 commits into
y-scope:mainfrom
Bill-hbrhbr:add-ffi-sfa-search-disable-telemetry-libarchvie
Open

build(clp_s::ffi::sfa): Add search dependencies and make OpenTelemetry and Libarchive optional for downstream WebAssembly.#2474
Bill-hbrhbr wants to merge 5 commits into
y-scope:mainfrom
Bill-hbrhbr:add-ffi-sfa-search-disable-telemetry-libarchvie

Conversation

@Bill-hbrhbr

@Bill-hbrhbr Bill-hbrhbr commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Description

This change prepares clp_s::ffi::sfa for downstream WebAssembly search functionalities by providing flags to disable OpenTelemetry and Libarchive support. Downstream clp-ffi-js will link against clp_s::search without the need to cross-compile these two libraries.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Summary by CodeRabbit

  • New Features
    • Added an option to enable OpenTelemetry-based search telemetry.
    • Added support for configuring search dependencies through the build system.
    • Added no-op telemetry behavior when OpenTelemetry is disabled, preserving search functionality without telemetry overhead.
  • Build Improvements
    • Improved dependency linking for search-related functionality.
    • Updated feature configuration so FFI SFA can use search capabilities.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The build adds an OpenTelemetry C++ option for search. Search telemetry now uses either real or no-op implementations. Dependency wiring, FFI SFA linkage, KQL validation, and libarchive include placement are updated.

Changes

Search telemetry build

Layer / File(s) Summary
Telemetry dependency selection
components/core/cmake/Options/options.cmake
The new option controls OpenTelemetry C++ and XXHash dependencies. Search no longer enables these dependencies unconditionally.
Conditional telemetry implementations
components/core/src/clp_s/search/CMakeLists.txt, components/core/src/clp_s/search/SearchTelemetryNoop.cpp, components/core/src/clp_s/search/TelemetryContextNoop.cpp
The search target builds real telemetry sources when enabled and no-op implementations when disabled.
Target and include integration
components/core/cmake/Options/options.cmake, components/core/src/clp_s/ffi/CMakeLists.txt, components/core/src/clp_s/InputConfig.cpp
FFI SFA validation requires KQL, FFI SFA links publicly to search, and libarchive includes move into the conditional include block.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant BuildOption as Search telemetry build option
  participant SearchCMake as clp_s search CMake
  participant RealTelemetry as Real telemetry sources
  participant NoopTelemetry as No-op telemetry sources
  BuildOption->>SearchCMake: Select telemetry mode
  SearchCMake->>RealTelemetry: Compile when enabled
  SearchCMake->>NoopTelemetry: Compile when disabled
Loading

Suggested reviewers: junhaoliao

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: adding search dependencies and making OpenTelemetry and Libarchive optional for downstream WebAssembly builds.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Bill-hbrhbr Bill-hbrhbr changed the title build(clp_s::ffi::sfa): Add CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP build option to support downstream WebAssembly compatibility. build(clp_s::ffi::sfa): Add search dependencies and CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP build option to support downstream WebAssembly compatibility. Aug 11, 2026
@Bill-hbrhbr Bill-hbrhbr changed the title build(clp_s::ffi::sfa): Add search dependencies and CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP build option to support downstream WebAssembly compatibility. build(clp_s::ffi::sfa): Add search dependencies and make OpenTelemetry and Libarchive optional for downstream WebAssembly. Aug 11, 2026
@Bill-hbrhbr
Bill-hbrhbr requested a review from junhaoliao August 11, 2026 18:09
@Bill-hbrhbr
Bill-hbrhbr marked this pull request as ready for review August 11, 2026 18:41
@Bill-hbrhbr
Bill-hbrhbr requested review from a team and gibber9809 as code owners August 11, 2026 18:41

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@components/core/cmake/Options/options.cmake`:
- Around line 414-419: Update
set_clp_s_search_enable_opentelemetry_cpp_dependencies so it sets
CLP_NEED_OPENTELEMETRY_CPP and CLP_NEED_XXHASH only when CLP_BUILD_CLP_S_SEARCH
is enabled; preserve the existing dependency flags for search builds while
leaving them unset for unrelated builds.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e8af5db7-2b89-415f-b049-1c73ee454e08

📥 Commits

Reviewing files that changed from the base of the PR and between 93d23c4 and e351129.

📒 Files selected for processing (6)
  • components/core/cmake/Options/options.cmake
  • components/core/src/clp_s/InputConfig.cpp
  • components/core/src/clp_s/ffi/CMakeLists.txt
  • components/core/src/clp_s/search/CMakeLists.txt
  • components/core/src/clp_s/search/SearchTelemetryNoop.cpp
  • components/core/src/clp_s/search/TelemetryContextNoop.cpp

Comment thread components/core/cmake/Options/options.cmake

@gibber9809 gibber9809 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we also use the CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY flag in clp_s/CommandLineArguments.cpp to remove the enable-telemetry flag using some #if macros when open telemetry is disabled in the build?

Comment on lines +99 to +100
CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP
"Include open telemetry support for clp_s::search."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP
"Include open telemetry support for clp_s::search."
CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY
"Include open telemetry support for clp-s."

Could we rename the flag (and other corresponding functions, etc.) to this? If we did add opentelemetry support to other parts of clp-s we'd probably want to share the same flag, instead of relying on a different flag for each sub-component, so we might as well just create a flag at the clp-s level now.

clp_s_ffi_sfa
PUBLIC
clp_s::archive_reader
clp_s::search

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
clp_s::search

Not used in this PR? Should probably be part of the PR where you add search to the ffi.

Comment on lines +70 to +71
SearchTelemetryNoop.cpp
TelemetryContextNoop.cpp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of adding new cpp files let's use the more standard approach of doing:

target_compile_definitions(
        clp_s_search
        PRIVATE
        CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY=$<BOOL:${CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY}>
)

Then use #if ... macros to stub out implementations in the existing cpp files.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants