build(deps): Migrate core's submodule-based dependencies to task-based installation.#858
Conversation
…ependency checksum files from a directory.
… abseil-cpp task to absl.
…me Catch2 task to catch.
…k; Rename json task to nlohmann_json.
… Rename ystdlib-cpp task to ystdlib.
…ernal-deps task to core-all-parallel; Remove obsolete comment about submodule download parallelization.
…created before checksums.
## Walkthrough
This change removes all embedded third-party submodules from the repository, including libraries such as Catch2, abseil-cpp, date, json, log-surgeon, outcome, simdjson, utfcpp, yaml-cpp, and ystdlib-cpp. The build system is refactored to use external package discovery through CMake's `find_package` and a new Taskfile-based dependency management system. The previous monolithic `taskfiles/deps.yaml` is replaced by a modular structure with `taskfiles/deps/main.yaml` and `taskfiles/deps/utils.yaml`. All relevant source and test files update their include paths to use system or installed package headers. GitHub Actions workflows and taskfile references are updated to reflect the new dependency structure.
## Changes
| Files / Groups | Change Summary |
|--------------------------------------------------------------------------------------------------|----------------|
| `.github/workflows/clp-core-build.yaml`, `.github/workflows/clp-core-build-macos.yaml` | Broadened workflow trigger paths from `taskfiles/deps.yaml` to `taskfiles/**`. |
| `.gitmodules`, `components/core/submodules/*` | Removed all submodules except `tools/yscope-dev-utils`. |
| `components/core/.clang-format` | Updated regex for library header grouping: replaced `json` with `nlohmann`. |
| `components/core/.gitignore` | Removed ignore rules for submodules and third-party directories; only `build/**` remains. |
| `components/core/CMakeLists.txt`, `components/core/src/clp/clg/CMakeLists.txt`, `components/core/src/clp/clo/CMakeLists.txt`, `components/core/src/clp/clp/CMakeLists.txt`, `components/core/src/clp_s/CMakeLists.txt`, `components/core/src/clp_s/indexer/CMakeLists.txt`, `components/core/src/clp_s/search/ast/CMakeLists.txt`, `components/core/src/glt/glt/CMakeLists.txt`, `components/core/src/make_dictionaries_readable/CMakeLists.txt`, `components/core/src/reducer/CMakeLists.txt`, `components/core/src/regex_utils/CMakeLists.txt` | Refactored build to remove submodule references, use `find_package`, update include/link paths for dependencies, and use variables for source/include directories. |
| `components/core/cmake/Modules/FindANTLR.cmake` | Removed hardcoded ANTLR jar path; now relies on `find_program`. |
| `components/core/src/**/*.[ch]pp` | Updated include directives for third-party headers to use system/installed package paths (e.g., `<nlohmann/json.hpp>`, `<catch2/catch.hpp>`, `<date/date.h>`, `<outcome/outcome.hpp>`, `<sqlite3/sqlite3.h>`, etc.). |
| `components/core/tests/*` | Updated Catch2 and nlohmann JSON include paths to standard installed package headers. |
| `taskfile.yaml` | Updated included deps taskfile reference from `taskfiles/deps.yaml` to `taskfiles/deps/main.yaml` and updated utility variable path. |
| `taskfiles/deps.yaml` | Removed: previously contained all dependency management tasks and variables. |
| `taskfiles/deps/main.yaml`, `taskfiles/deps/utils.yaml` | Added: new modular taskfiles for dependency management, installation, and checksum handling with parallelized and improved dependency tasks. |
| `taskfiles/lint.yaml` | Changed `yamllint` to lint the entire `taskfiles` directory instead of specific files. |
| `tools/scripts/deps-download/init.sh` | Updated `yscope-dev-utils` version and extracted directory name. |
| `components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh` | Added `unzip` and `xz` to prebuilt package installation scripts. |
| `components/core/tools/scripts/lib_install/ubuntu-focal/install-prebuilt-packages.sh` | Added `unzip` to prebuilt package installation scripts. |
| `components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh` | Added `unzip` to prebuilt package installation scripts. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Developer
participant Taskfile
participant DependencyInstaller
participant CMake
participant SystemPackages
Developer->>Taskfile: Run dependency tasks (e.g., core)
Taskfile->>DependencyInstaller: Download & extract dependencies (parallel)
DependencyInstaller->>SystemPackages: Install/verify packages (e.g., unzip, xz)
DependencyInstaller->>CMake: Generate settings, set variables for found packages
CMake->>SystemPackages: find_package for dependencies (Catch2, date, etc.)
CMake-->>Developer: Build proceeds using system/external dependenciesPossibly related PRs
Suggested reviewers
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
components/core/src/clp/clo/CMakeLists.txt (1)
158-163: Expand include directories for external dependencies
Adding${CLP_OUTCOME_SOURCE_DIRECTORY}and${CLP_SQLITE3_SOURCE_DIRECTORY}enables consumption of external packages. Consider removing the legacy${PROJECT_SOURCE_DIR}/submodulesentry if the embedded submodules have been fully removed to avoid stale include paths.components/core/src/glt/glt/CMakeLists.txt (1)
177-182: Expand include directories for external dependencies
The target now includes${CLP_OUTCOME_SOURCE_DIRECTORY}and${CLP_SQLITE3_SOURCE_DIRECTORY}. If all submodules are removed, consider dropping${PROJECT_SOURCE_DIR}/submodulesto clean up obsolete include paths.taskfiles/deps/utils.yaml (2)
20-30:combine-core-checksum-filesconcatenates individual checksums.The loop correctly excludes the combined file itself and rebuilds it from fresh
.md5files. Consider sorting input files for deterministic ordering.
48-79:install-remote-cmake-libimplements idempotent CMake install with checksum.You orchestrate remote tarball install, validate and compute checksums. To improve idempotency, consider removing or backing up an existing
INSTALL_PREFIXbefore re-extracting, or add acleanupflag var.taskfiles/deps/main.yaml (2)
7-20: Review variable definitions for consistency and usage
Centralising directory and checksum paths is great. However,G_DEPS_CORE_CMAKE_SETTINGS_FILE(line 19) isn’t referenced in any task—consider removing it or using it in place of hardcoded"all.cmake".
262-280: yscope stdlib download & CMake integration
The pattern matches ANTLR and Outcome. To reduce repetition, consider templating theecho-to-file step via a YAML anchor/alias.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
.github/workflows/clp-core-build-macos.yaml(2 hunks).github/workflows/clp-core-build.yaml(3 hunks).gitmodules(0 hunks)components/core/.clang-format(1 hunks)components/core/.gitignore(0 hunks)components/core/CMakeLists.txt(6 hunks)components/core/cmake/Modules/FindANTLR.cmake(0 hunks)components/core/src/clp/GlobalMetadataDBConfig.cpp(1 hunks)components/core/src/clp/SQLiteDB.hpp(1 hunks)components/core/src/clp/SQLitePreparedStatement.hpp(1 hunks)components/core/src/clp/TimestampPattern.cpp(1 hunks)components/core/src/clp/clg/CMakeLists.txt(1 hunks)components/core/src/clp/clo/CMakeLists.txt(2 hunks)components/core/src/clp/clo/clo.cpp(1 hunks)components/core/src/clp/clp/CMakeLists.txt(2 hunks)components/core/src/clp/ffi/KeyValuePairLogEvent.cpp(1 hunks)components/core/src/clp/ffi/KeyValuePairLogEvent.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/Deserializer.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/Serializer.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/Serializer.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/encoding_methods.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/utils.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/utils.hpp(1 hunks)components/core/src/clp/ir/LogEventDeserializer.cpp(1 hunks)components/core/src/clp/ir/LogEventDeserializer.hpp(1 hunks)components/core/src/clp/make_dictionaries_readable/CMakeLists.txt(0 hunks)components/core/src/clp/regex_utils/CMakeLists.txt(1 hunks)components/core/src/clp/regex_utils/regex_translation_utils.cpp(1 hunks)components/core/src/clp/regex_utils/regex_translation_utils.hpp(1 hunks)components/core/src/clp/streaming_archive/writer/Archive.cpp(1 hunks)components/core/src/clp_s/ArchiveWriter.cpp(1 hunks)components/core/src/clp_s/CMakeLists.txt(2 hunks)components/core/src/clp_s/JsonConstructor.cpp(1 hunks)components/core/src/clp_s/TimestampPattern.cpp(1 hunks)components/core/src/clp_s/clp-s.cpp(1 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks)components/core/src/clp_s/search/ast/CMakeLists.txt(1 hunks)components/core/src/glt/GlobalMetadataDBConfig.cpp(1 hunks)components/core/src/glt/SQLiteDB.hpp(1 hunks)components/core/src/glt/SQLitePreparedStatement.hpp(1 hunks)components/core/src/glt/TimestampPattern.cpp(1 hunks)components/core/src/glt/ffi/ir_stream/encoding_methods.cpp(1 hunks)components/core/src/glt/glt/CMakeLists.txt(2 hunks)components/core/src/glt/ir/LogEventDeserializer.cpp(1 hunks)components/core/src/glt/ir/LogEventDeserializer.hpp(1 hunks)components/core/src/glt/streaming_archive/writer/Archive.cpp(1 hunks)components/core/src/reducer/CMakeLists.txt(1 hunks)components/core/src/reducer/DeserializedRecordGroup.cpp(1 hunks)components/core/src/reducer/DeserializedRecordGroup.hpp(1 hunks)components/core/src/reducer/JsonArrayRecordIterator.hpp(1 hunks)components/core/src/reducer/JsonRecord.hpp(1 hunks)components/core/src/reducer/ServerContext.cpp(1 hunks)components/core/src/reducer/ServerContext.hpp(1 hunks)components/core/src/reducer/reducer_server.cpp(1 hunks)components/core/submodules/Catch2(0 hunks)components/core/submodules/abseil-cpp(0 hunks)components/core/submodules/date(0 hunks)components/core/submodules/json(0 hunks)components/core/submodules/log-surgeon(0 hunks)components/core/submodules/outcome(0 hunks)components/core/submodules/simdjson(0 hunks)components/core/submodules/utfcpp(0 hunks)components/core/submodules/yaml-cpp(0 hunks)components/core/submodules/ystdlib-cpp(0 hunks)components/core/tests/test-BoundedReader.cpp(1 hunks)components/core/tests/test-BufferedFileReader.cpp(1 hunks)components/core/tests/test-EncodedVariableInterpreter.cpp(1 hunks)components/core/tests/test-FileDescriptorReader.cpp(1 hunks)components/core/tests/test-Grep.cpp(1 hunks)components/core/tests/test-MemoryMappedFile.cpp(1 hunks)components/core/tests/test-NetworkReader.cpp(1 hunks)components/core/tests/test-ParserWithUserSchema.cpp(1 hunks)components/core/tests/test-SQLiteDB.cpp(1 hunks)components/core/tests/test-Segment.cpp(1 hunks)components/core/tests/test-Stopwatch.cpp(1 hunks)components/core/tests/test-StreamingCompression.cpp(1 hunks)components/core/tests/test-TimestampPattern.cpp(1 hunks)components/core/tests/test-Utils.cpp(1 hunks)components/core/tests/test-clp_s-end_to_end.cpp(1 hunks)components/core/tests/test-clp_s-search.cpp(1 hunks)components/core/tests/test-encoding_methods.cpp(1 hunks)components/core/tests/test-ffi_IrUnitHandlerInterface.cpp(1 hunks)components/core/tests/test-ffi_KeyValuePairLogEvent.cpp(1 hunks)components/core/tests/test-ffi_SchemaTree.cpp(1 hunks)components/core/tests/test-hash_utils.cpp(1 hunks)components/core/tests/test-ir_encoding_methods.cpp(1 hunks)components/core/tests/test-ir_parsing.cpp(1 hunks)components/core/tests/test-ir_serializer.cpp(1 hunks)components/core/tests/test-kql.cpp(1 hunks)components/core/tests/test-main.cpp(1 hunks)components/core/tests/test-math_utils.cpp(1 hunks)components/core/tests/test-query_methods.cpp(1 hunks)components/core/tests/test-regex_utils.cpp(1 hunks)components/core/tests/test-sql.cpp(1 hunks)components/core/tests/test-string_utils.cpp(1 hunks)components/core/tests/test-utf8_utils.cpp(1 hunks)components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-focal/install-prebuilt-packages.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)taskfile.yaml(2 hunks)taskfiles/deps.yaml(0 hunks)taskfiles/deps/main.yaml(1 hunks)taskfiles/deps/utils.yaml(1 hunks)taskfiles/lint.yaml(1 hunks)tools/scripts/deps-download/init.sh(1 hunks)
⛔ Files not processed due to max files limit (1)
- tools/yscope-dev-utils
💤 Files with no reviewable changes (15)
- components/core/.gitignore
- components/core/submodules/abseil-cpp
- components/core/submodules/date
- components/core/submodules/outcome
- components/core/cmake/Modules/FindANTLR.cmake
- components/core/submodules/log-surgeon
- components/core/submodules/Catch2
- components/core/submodules/ystdlib-cpp
- components/core/submodules/json
- components/core/submodules/utfcpp
- components/core/submodules/simdjson
- .gitmodules
- components/core/submodules/yaml-cpp
- components/core/src/clp/make_dictionaries_readable/CMakeLists.txt
- taskfiles/deps.yaml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hppcomponents/core/tests/test-MemoryMappedFile.cppcomponents/core/src/reducer/JsonRecord.hppcomponents/core/src/clp/SQLitePreparedStatement.hppcomponents/core/tests/test-hash_utils.cppcomponents/core/tests/test-ffi_SchemaTree.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/tests/test-ir_parsing.cppcomponents/core/src/reducer/DeserializedRecordGroup.cppcomponents/core/tests/test-SQLiteDB.cppcomponents/core/src/clp/SQLiteDB.hppcomponents/core/src/clp/TimestampPattern.cppcomponents/core/tests/test-Utils.cppcomponents/core/tests/test-sql.cppcomponents/core/src/clp/ffi/ir_stream/utils.cppcomponents/core/src/clp_s/JsonConstructor.cppcomponents/core/src/glt/streaming_archive/writer/Archive.cppcomponents/core/src/glt/SQLitePreparedStatement.hppcomponents/core/tests/test-kql.cppcomponents/core/tests/test-BoundedReader.cppcomponents/core/src/reducer/JsonArrayRecordIterator.hppcomponents/core/tests/test-math_utils.cppcomponents/core/tests/test-string_utils.cppcomponents/core/tests/test-FileDescriptorReader.cppcomponents/core/src/clp/ffi/ir_stream/encoding_methods.cppcomponents/core/src/clp/ir/LogEventDeserializer.hppcomponents/core/tests/test-ParserWithUserSchema.cppcomponents/core/src/reducer/DeserializedRecordGroup.hppcomponents/core/tests/test-regex_utils.cppcomponents/core/tests/test-Stopwatch.cppcomponents/core/tests/test-StreamingCompression.cppcomponents/core/src/reducer/ServerContext.cppcomponents/core/tests/test-clp_s-end_to_end.cppcomponents/core/src/clp_s/TimestampPattern.cppcomponents/core/src/reducer/reducer_server.cppcomponents/core/src/clp/GlobalMetadataDBConfig.cppcomponents/core/tests/test-encoding_methods.cppcomponents/core/src/clp/ir/LogEventDeserializer.cppcomponents/core/src/clp/streaming_archive/writer/Archive.cppcomponents/core/src/glt/ir/LogEventDeserializer.hppcomponents/core/tests/test-utf8_utils.cppcomponents/core/tests/test-query_methods.cppcomponents/core/src/clp/clo/clo.cppcomponents/core/src/clp/ffi/KeyValuePairLogEvent.hppcomponents/core/src/clp_s/ArchiveWriter.cppcomponents/core/tests/test-main.cppcomponents/core/tests/test-clp_s-search.cppcomponents/core/src/glt/ir/LogEventDeserializer.cppcomponents/core/src/clp/regex_utils/regex_translation_utils.cppcomponents/core/src/glt/ffi/ir_stream/encoding_methods.cppcomponents/core/src/reducer/ServerContext.hppcomponents/core/src/clp/ffi/ir_stream/Serializer.cppcomponents/core/src/glt/TimestampPattern.cppcomponents/core/tests/test-EncodedVariableInterpreter.cppcomponents/core/tests/test-ir_serializer.cppcomponents/core/src/glt/SQLiteDB.hppcomponents/core/tests/test-BufferedFileReader.cppcomponents/core/tests/test-NetworkReader.cppcomponents/core/tests/test-TimestampPattern.cppcomponents/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cppcomponents/core/src/glt/GlobalMetadataDBConfig.cppcomponents/core/tests/test-Grep.cppcomponents/core/tests/test-ffi_IrUnitHandlerInterface.cppcomponents/core/tests/test-ffi_KeyValuePairLogEvent.cppcomponents/core/src/clp/regex_utils/regex_translation_utils.hppcomponents/core/src/clp/ffi/ir_stream/Deserializer.hppcomponents/core/src/clp/ffi/ir_stream/utils.hppcomponents/core/src/clp/ffi/KeyValuePairLogEvent.cppcomponents/core/tests/test-Segment.cppcomponents/core/src/clp/ffi/ir_stream/Serializer.hppcomponents/core/tests/test-ir_encoding_methods.cpp
🧠 Learnings (6)
components/core/tests/test-encoding_methods.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
components/core/src/clp/ir/LogEventDeserializer.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/tests/test-ir_encoding_methods.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (135)
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (1)
23-24: Approve addition ofunzipandxzpackages: Including these utilities ensures that compressed archives can be properly extracted during the dependency installation process, maintaining consistency with the Ubuntu scripts.components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)
28-29: Verify necessity and scope of new packages: Addingunzipis appropriate for archive extraction, aligning with other platforms. However,software-properties-commonprovidesadd-apt-repositorybut is not used in this script. Please confirm whether it’s required here or if it should be installed only in the specific task or documented accordingly.components/core/src/clp/SQLitePreparedStatement.hpp (1)
6-6: Ensure system include path for SQLite3: Switching the include to<sqlite3.h>aligns with external package usage. Verify thatfind_package(SQLite3)or updated CMaketarget_include_directoriesfor theclptarget correctly exposes the system’s SQLite3 include directory so this header resolves.components/core/src/glt/SQLitePreparedStatement.hpp (1)
6-6: Validate include path for thegltcomponent: As with the CLP version, confirm that theglttarget’s CMakeLists now usesfind_package(SQLite3)or sets its include directories such that<sqlite3.h>is discoverable, preventing build failures.components/core/tests/test-Utils.cpp (1)
11-11: Update Catch2 include to external package path: The new#include <catch2/catch.hpp>matches external Catch2 usage after removing the submodule. Please ensure the CMake test targets now callfind_package(Catch2 REQUIRED)and update any documentation or CMake examples so this path is resolvable.components/core/src/clp_s/JsonConstructor.cpp (1)
12-12: Standardize JSON include path. Updated the include directive to<nlohmann/json.hpp>to use the external nlohmann_json package instead of the removed submodule.components/core/tests/test-StreamingCompression.cpp (1)
11-11: Update Catch2 include. Changed to<catch2/catch.hpp>to reference the installed Catch2 package rather than the embedded submodule.components/core/src/glt/ir/LogEventDeserializer.hpp (1)
6-6: Simplify Outcome include path. Replaced the nested submodule path with the single-header<outcome.hpp>for consistency with the external Outcome package.components/core/src/reducer/DeserializedRecordGroup.cpp (1)
5-5: Switch to public JSON header. Updated the include to<nlohmann/json.hpp>to align with the external nlohmann_json installation.components/core/src/clp/TimestampPattern.cpp (1)
7-7: Use external date header. Changed the include from the submodule path to<date/date.h>, matching the externaldatepackage layout.components/core/src/glt/TimestampPattern.cpp (1)
7-7: Include path updated to use external package.The include directive has been updated to use the standard include path for the date library (
<date/date.h>) instead of the embedded submodule path. This change aligns with the PR objective of migrating from submodule-based dependencies to task-based installation.components/core/tests/test-math_utils.cpp (1)
1-1: Include path updated for Catch2 framework.The include directive for Catch2 has been properly updated to use the standard system include path. This change is part of the PR objective to migrate from submodule-based dependencies to externally managed packages.
components/core/tests/test-ir_parsing.cpp (1)
1-1: Include path updated for Catch2 framework.The include directive for the Catch2 testing framework has been properly updated to use the standard system include path. This change is consistent with the PR's objective of removing embedded submodules and switching to externally managed packages.
components/core/src/glt/SQLiteDB.hpp (1)
6-7: Include path updated for SQLite3.The include directive has been properly updated to use the standard system include path for SQLite3. This change is part of the broader effort to migrate from submodule-based dependencies to task-based installation of external packages.
components/core/src/clp/SQLiteDB.hpp (1)
6-6: Update to system include path is appropriate.This change correctly updates the SQLite3 include from a relative path to a system path, aligning with the PR objective of migrating from submodule-based dependencies to task-based installation.
components/core/tests/test-ffi_SchemaTree.cpp (1)
3-3: Well-structured include path update for Catch2.The change from the submodule path to the standardized system include path is consistent with the PR's goal of removing embedded submodules and transitioning to task-based dependency management.
components/core/src/reducer/ServerContext.hpp (1)
10-10: Appropriate update to nlohmann/json include path.This change correctly updates the JSON library include from the submodule path to the standardized system path, which is consistent with the removal of the embedded JSON submodule and transition to external package management.
components/core/src/clp/ffi/ir_stream/encoding_methods.cpp (1)
3-3: JSON include path correctly updated.The update from the submodule path to the standardized system include path aligns with the project-wide migration to external dependency management. The code follows good practices, including the coding guideline of using
false == <expression>pattern instead of!<expression>as seen throughout the file.components/core/tests/test-Stopwatch.cpp (1)
3-3: Approve updated Catch2 include path
The include directive has been simplified from the embedded submodule path to the standard installed Catch2 package (<catch2/catch.hpp>), matching the new external dependency approach.components/core/tests/test-encoding_methods.cpp (1)
1-1: Approve updated Catch2 include path
Changed to<catch2/catch.hpp>to use the externally installed Catch2 package rather than the removed submodule.components/core/src/clp_s/TimestampPattern.cpp (1)
11-11: Approvedatelibrary include update
Switching from the submodule header to the system-installed<date/date.h>aligns with the new CMakefind_package(date)configuration.components/core/src/reducer/DeserializedRecordGroup.hpp (1)
8-8: Approve nlohmann JSON include update
Updating to<nlohmann/json.hpp>correctly uses the externally managed JSON library instead of the removed submodule path.components/core/src/reducer/JsonRecord.hpp (1)
4-4: Approve nlohmann JSON include update
The include directive now references the standard installed header (<nlohmann/json.hpp>), consistent with the external dependency migration.components/core/src/glt/GlobalMetadataDBConfig.cpp (1)
4-4: Include path simplified
The include for yaml-cpp has been updated to the public header<yaml-cpp/yaml.h>. This aligns with the external package layout—please ensure your CMakeLists usesfind_package(yaml-cpp REQUIRED)and correctly links or propagates the include directories.components/core/src/reducer/reducer_server.cpp (1)
9-9: Standardize nlohmann/json include
Switched to<nlohmann/json.hpp>to reflect the external nlohmann_json package. Verify that your CMakeLists callsfind_package(nlohmann_json REQUIRED)and linksnlohmann_json::nlohmann_jsonto thereducer-servertarget so this header resolves.components/core/tests/test-Segment.cpp (1)
4-4: Catch2 include path updated
Changed to<catch2/catch.hpp>to use the installed Catch2 package. Make sure the test CMakeLists invokesfind_package(Catch2 REQUIRED)and links withCatch2::Catch2.components/core/src/reducer/JsonArrayRecordIterator.hpp (1)
4-4: Align JSON include with external dependency
The header now uses<nlohmann/json.hpp>, matching the nlohmann_json package. Confirm that your build configuration exports the include path via thenlohmann_json::nlohmann_jsontarget.components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp (1)
9-9: Simplify Outcome include directive
Updated to#include <outcome.hpp>which matches the installed Outcome package layout. Ensure the corresponding CMakeLists invokes the correctfind_package(Outcome REQUIRED)(or similar) and exposes the include path.components/core/tests/test-main.cpp (1)
2-2: Update Catch2 include directive
The include path has been correctly updated to use the installed Catch2 package (<catch2/catch.hpp>), aligning with the new external dependency management approach.components/core/src/clp/streaming_archive/writer/Archive.cpp (1)
15-15: Standardise nlohmann JSON include
The JSON include path has been updated to<nlohmann/json.hpp>, matching the externalnlohmann_jsonpackage. Please confirm that your CMakefind_package(nlohmann_json)invocation exposes the correct include directory.components/core/tests/test-sql.cpp (1)
4-4: Update Catch2 include directive
The test include has been switched to<catch2/catch.hpp>, ensuring consistency with the external Catch2 installation. Verify that all test CMake targets link against the Catch2 package.components/core/src/clp/regex_utils/regex_translation_utils.cpp (1)
8-8: Simplify Outcome include path
The Outcome include directive now uses<outcome.hpp>, reflecting the external Outcome library. Ensure that the CMakeLists forregex_utilsadds the proper include directory (e.g.,${CLP_OUTCOME_SOURCE_DIRECTORY}or the system-installed path).components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp (1)
13-13: Simplify Outcome include path
The Outcome include has been streamlined to<outcome.hpp>, consistent with the new package-based dependency approach. Confirm that your build system’s include paths for Outcome are correctly configured.components/core/src/glt/ffi/ir_stream/encoding_methods.cpp (1)
3-3: Standardize JSON include path
The include directive has been updated to reference the externally installednlohmann/json.hpprather than the embedded submodule path, matching the new task-based dependency installation.components/core/tests/test-ParserWithUserSchema.cpp (1)
10-10: Standardize Catch2 include path
The Catch2 header include has been simplified to<catch2/catch.hpp>, aligning with the move away from the embedded Catch2 submodule and using the external package.components/core/tests/test-hash_utils.cpp (1)
4-4: Standardize Catch2 include in hash utils test
The test now includes<catch2/catch.hpp>instead of the old submodule path, consistent with the project-wide dependency migration.components/core/tests/test-SQLiteDB.cpp (1)
13-13: Update Catch2 include for SQLiteDB tests
The Catch2 framework is now referenced via<catch2/catch.hpp>, reflecting the external dependency installation and removal of the old submodule.components/core/src/clp/regex_utils/regex_translation_utils.hpp (1)
7-7: Switch Outcome include to external header
The Outcome include has been changed to<outcome.hpp>, matching the external package usage after removing the embedded submodule.components/core/src/clp/ffi/ir_stream/utils.cpp (1)
8-9: Migration to system-installed JSON library
Replacing the submodule-based include with<nlohmann/json.hpp>and adding<nlohmann/json_fwd.hpp>correctly aligns with external dependency management.components/core/src/clp_s/clp-s.cpp (1)
11-11: Update to external JSON include path
Good replacement of the submodule-based path with<nlohmann/json.hpp>to match the external package discovery approach.components/core/tests/test-TimestampPattern.cpp (1)
1-1: Standardize Catch2 include path
Updating to<catch2/catch.hpp>is consistent with the removal of the Catch2 submodule and external dependency usage.components/core/tests/test-ir_serializer.cpp (1)
8-8: Standardize Catch2 include path
Switching to<catch2/catch.hpp>aligns this test file with the external Catch2 package.components/core/tests/test-ffi_IrUnitHandlerInterface.cpp (1)
6-6: Standardize Catch2 include path
The updated include<catch2/catch.hpp>correctly reflects the external Catch2 installation.components/core/src/clp/GlobalMetadataDBConfig.cpp (1)
3-5: Include path updated for external yaml-cpp package
The include directive now uses<yaml-cpp/yaml.h>, matching the standard installation layout. Ensure that your CMakefind_package(yaml-cpp REQUIRED)is configuring the include directories so this header is resolved correctly.components/core/tests/test-MemoryMappedFile.cpp (1)
7-7: Standardised Catch2 include path
The test now includes<catch2/catch.hpp>to use the externally installed Catch2. Verify thatfind_package(Catch2 REQUIRED)andtarget_link_libraries(... PRIVATE Catch2::Catch2WithMain)are configured so this header is available.components/core/tests/test-string_utils.cpp (1)
5-5: Updated Catch2 include for external package
Switched to the canonical<catch2/catch.hpp>after migrating Catch2 out of a submodule. Confirm that your test target links against the external Catch2 target so this path is on the include search path.components/core/src/reducer/CMakeLists.txt (1)
44-54:Details
✅ Verification successful
Add JSON library to reducer-server
Linkingnlohmann_json::nlohmann_jsonis required for JSON support now that the embeddedjsonsubmodule is removed. Please verify thatfind_package(nlohmann_json REQUIRED)(or the equivalentfind_package(nlohmann_json CONFIG REQUIRED)) is called earlier in the CMake hierarchy so the target is available.Run this script from the repo root to confirm:
🏁 Script executed:
#!/bin/bash # Verify that nlohmann_json is located in the CMake configuration rg "find_package.*nlohmann_json" -n .Length of output: 113
Confirmed nlohmann_json is available in the CMake hierarchy
The
find_package(nlohmann_json REQUIRED)call exists at line 167 ofcomponents/core/CMakeLists.txt, which precedes the inclusion ofcomponents/core/src/reducer/CMakeLists.txt. No further changes are needed here.components/core/tests/test-query_methods.cpp (1)
3-3: Standardised Catch2 include directive
The Catch2 header path has been updated to<catch2/catch.hpp>to reflect external dependency usage. Ensure the corresponding CMake test target is linkingCatch2::Catch2WithMain(or similar) so this include resolves correctly.components/core/src/clp_s/ArchiveWriter.cpp (1)
7-7: Include path updated for nlohmann/json.The include directive has been updated from a submodule-based path to a standard installed package path, which aligns with the migration from submodule-based dependencies to task-based installation.
components/core/tests/test-clp_s-end_to_end.cpp (1)
8-8: Include path updated for Catch2.The include directive has been updated from a submodule-based path to a standard installed package path, which aligns with the migration from submodule-based dependencies to task-based installation.
components/core/tests/test-kql.cpp (1)
5-5: Include path updated for Catch2.The include directive has been updated from a submodule-based path to a standard installed package path, which aligns with the migration from submodule-based dependencies to task-based installation.
components/core/src/reducer/ServerContext.cpp (1)
12-12: Include path updated for nlohmann/json.The include directive has been updated from a submodule-based path to a standard installed package path, which aligns with the migration from submodule-based dependencies to task-based installation.
components/core/src/clp_s/search/ast/CMakeLists.txt (1)
65-65: Transition to fully qualified simdjson target
Usingsimdjson::simdjsonensures we link against the externally provided package rather than a local submodule.components/core/src/clp/clo/clo.cpp (1)
7-7: Update JSON include path to external package
Including<nlohmann/json.hpp>aligns with the shift to external dependency management and ensures compatibility with the found package.components/core/src/clp/ir/LogEventDeserializer.cpp (1)
5-6: Adopt standard package includes for JSON and Outcome
Replacing submodule-relative headers with<nlohmann/json.hpp>and<outcome.hpp>aligns this source with the external package discovery approach.components/core/tests/test-regex_utils.cpp (1)
3-3: Standardize Catch2 include path
Updating to<catch2/catch.hpp>matches the external Catch2 package and cleans up the include to a single, consistent form.components/core/tests/test-FileDescriptorReader.cpp (1)
7-7: Simplify Catch2 include for external package
Switching to<catch2/catch.hpp>ensures the test suite uses the externally managed Catch2 target.components/core/src/clp/ir/LogEventDeserializer.hpp (1)
6-6: Standardize Outcome include path. The updated directive<outcome.hpp>matches the external package and aligns with the project’s include style after removing the embedded Outcome submodule.components/core/src/glt/streaming_archive/writer/Archive.cpp (1)
13-13: Update nlohmann JSON include path. The new<nlohmann/json.hpp>header is the canonical include for the nlohmann_json package and aligns with the CMakefind_package(nlohmann_json …)approach.components/core/tests/test-BoundedReader.cpp (1)
6-6: Simplify Catch2 include path. Switching to<catch2/catch.hpp>ensures compatibility with the external Catch2 package and removes reliance on the embedded submodule.components/core/.clang-format (1)
7-9: Adjust library header regex for nlohmann. The regex now correctly recognisesnlohmanninstead of the removedjsonsubmodule, keeping include grouping in sync with project dependencies.components/core/tests/test-EncodedVariableInterpreter.cpp (1)
3-3: Simplify Catch2 include path. Updated to<catch2/catch.hpp>to use the external package convention and remove the submodule path.components/core/tests/test-Grep.cpp (1)
3-3: Good include standardizationThe include path for Catch2 has been properly updated to use the standard format that is more consistent with external package management rather than embedded submodules.
components/core/tests/test-utf8_utils.cpp (1)
10-11: Include paths properly standardizedThe include paths for both Catch2 and nlohmann/json have been correctly updated to use standard external package formats, which aligns with the migration away from submodule-based dependencies.
components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)
11-12: Library includes properly updatedThe include paths for nlohmann/json and outcome have been appropriately updated to use the standard paths expected when using external package management rather than embedded submodules.
components/core/src/clp/regex_utils/CMakeLists.txt (1)
18-18: Build configuration properly updatedThe addition of
${CLP_OUTCOME_SOURCE_DIRECTORY}as a public include directory correctly supports the transition to external package management for the Outcome library.components/core/tests/test-BufferedFileReader.cpp (1)
4-4: Catch2 include directive updated to external package
The include line has been changed to reference the installed Catch2 package (<catch2/catch.hpp>) instead of the removed submodule path. This aligns with the new CMakefind_package(Catch2)usage and ensures tests pick up the external Catch2 installation..github/workflows/clp-core-build-macos.yaml (1)
16-16: Broaden workflow trigger paths for taskfiles directory
Thepathsfilter for bothpull_requestandpushevents now includes"taskfiles/**", so any changes under thetaskfiles/directory will correctly trigger the macOS core build workflow.Also applies to: 30-30
components/core/tools/scripts/lib_install/ubuntu-focal/install-prebuilt-packages.sh (1)
30-32: Addunzipto prebuilt packages installation
Includingunzipguarantees the build environment can extract downloaded archives during task-based dependency installation. This addition matches updates in the other OS-specific scripts.components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1)
12-13: Update JSON and Outcome include paths to external packages
Switched from submodule-based headers to system-installed ones (<nlohmann/json.hpp>and<outcome.hpp>), reflecting the removal of embedded submodules and the use offind_packagefor these dependencies.components/core/src/glt/ir/LogEventDeserializer.cpp (1)
5-6: Use external JSON and Outcome headers
Replaced relative single-header paths with<nlohmann/json.hpp>and<outcome.hpp>to align with the transition to externally managed dependencies and CMakefind_packageconfiguration.tools/scripts/deps-download/init.sh (1)
13-15: Ensure the updated commit‐hash and extraction directory match GitHub’s archive layout.The URL has been updated to use
e300d1b.zipand the target folder toyscope-dev-utils-e300d1bab4c2f33cbf9ddf9f0c08185faf035070. Please confirm that downloading that archive from GitHub actually produces the directory with the full hash suffix as named here.components/core/src/clp/ffi/KeyValuePairLogEvent.cpp (1)
14-17: Include new external package headers consistently.Updating to
<nlohmann/json.hpp>,<nlohmann/json_fwd.hpp>, and<outcome.hpp>aligns with the migration to external dependencies. The ordering (full header before forward header) is acceptable, but consider swapping to forward first if you want lighter includes for compile-time savings.components/core/src/clp/ffi/ir_stream/utils.hpp (1)
12-13: Replace embedded single-header includes with external package paths.Switching to
<nlohmann/json.hpp>and<outcome.hpp>is correct under the new task-based dependency installation. Ensure CMakeLists.txt has been updated tofind_package(nlohmann_json)andfind_package(Outcome)so these headers are on the include path.taskfiles/lint.yaml (1)
121-121: Broaden YAML linting to the entiretaskfiles/directory.Changing the source to
taskfilesensures new dependency taskfiles (e.g.,taskfiles/deps/main.yaml) are included in the lint step. This keeps lint coverage in sync with the refactor.components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)
15-17: Align include directives with external dependency usage.Replacing
json/single_include/nlohmann/json.hppandoutcome/single-header/outcome.hppwith<nlohmann/json.hpp>and<outcome.hpp>matches the global shift. Confirm thatmsgpack.hppand these new includes remain in the correct order and that no missing forward declarations are needed.components/core/tests/test-clp_s-search.cpp (2)
12-12: Align Catch2 include with external package
Updating to<catch2/catch.hpp>correctly switches from the embedded submodule to the installed Catch2 package.
14-14: Use system-installed nlohmann JSON header
Switching to<nlohmann/json.hpp>aligns with the new external-package approach.components/core/tests/test-NetworkReader.cpp (2)
12-12: Standardise Catch2 include path
The include has been updated tocatch2/catch.hpp, reflecting the move to the external Catch2 package.
15-15: Update nlohmann JSON include
Replacing the submodule path with<nlohmann/json.hpp>properly references the installed JSON library.components/core/tests/test-ffi_KeyValuePairLogEvent.cpp (1)
13-15: Consolidate Catch2 and nlohmann JSON includes
Switching from submodule-based paths to<catch2/catch.hpp>,<nlohmann/json.hpp>, and adding<nlohmann/json_fwd.hpp>aligns with the external-package migration and reduces dependency on embedded headers.components/core/src/clp/ffi/KeyValuePairLogEvent.hpp (1)
10-11: Use forward-declarations and standard Outcome header
Including<nlohmann/json_fwd.hpp>and<outcome.hpp>replaces the prior single-header embeds, reducing direct header dependencies and relying on the external packages.components/core/src/clp_s/indexer/CMakeLists.txt (3)
92-92: Link against the date library
Addingdate::dateensures the indexer target uses the externally installed date package.
95-95: Adopt the simdjson imported target
Usingsimdjson::simdjsoncorrectly references the CMake-exported simdjson target.
97-97: Verify YAML-CPP target name
The targetyaml-cppmay not match the imported namespace target (oftenyaml-cpp::yaml-cpp). Please confirm the exact target provided byfind_package(yaml-cpp)to avoid linkage errors..github/workflows/clp-core-build.yaml (3)
12-12: Broaden pull_request trigger to include all taskfiles/
Replacing the specifictaskfiles/deps.yamlpath withtaskfiles/**ensures that changes to any modularised dependency taskfile will trigger the CI.
23-23: Broaden push trigger to include all taskfiles/
Applying the same update for thepushevent ensures that any change undertaskfiles/kicks off the CI pipeline.
91-91: Expand filter to detect changes in all taskfiles/
Updating theclpfilter’s paths totaskfiles/**aligns with the new modularised taskfile structure so that relevant modifications are captured.components/core/src/clp/clo/CMakeLists.txt (3)
122-124: Use external SQLite3 source directory variable
The new entries includesqlite3.c,sqlite3.h, andsqlite3ext.hfrom${CLP_SQLITE3_SOURCE_DIRECTORY}. Please verify thatCLP_SQLITE3_SOURCE_DIRECTORYis defined correctly (e.g., via afind_packageor CMake option) and points to the intended external source.
167-167: Link against the date library
Includingdate::datein the link libraries aligns with the external Date dependency.
172-172: Link against the nlohmann_json library
Addingnlohmann_json::nlohmann_jsonensures the JSON target is linked correctly.components/core/src/glt/glt/CMakeLists.txt (4)
154-156: Use external SQLite3 source directory variable
Including SQLite3 sources via${CLP_SQLITE3_SOURCE_DIRECTORY}matches the external dependency approach. Please confirm that this variable is properly set and points to the correct files in your dependency installation.
186-186: Link against the date library
Addingdate::datetotarget_link_librariesreflects the use of the external Date package.
192-192: Link against the nlohmann_json library
Includingnlohmann_json::nlohmann_jsonensures the JSON dependency is linked correctly.
195-195: Verify YAML-CPP imported target
The linked targetyaml-cppmay differ from the imported name (oftenyaml-cpp::yaml-cpporYAML::YamlCpp). Please confirm thatyaml-cppmatches the CMakefind_package(YAML-CPP)imported target and adjust if necessary.taskfile.yaml (3)
4-4: Update dependency taskfile include path
Thedepsinclude now points totaskfiles/deps/main.yaml, matching the new modular structure. Please verify thattaskfiles/deps/main.yamlexists and is reachable.
7-7: Update utils taskfile include path
Reference to the utils taskfile was changed totools/yscope-dev-utils/exports/taskfiles/utils/utils.yaml. Confirm that the repository layout reflects this new path.
30-30: Set G_UTILS_TASKFILE to updated utils taskfile
TheG_UTILS_TASKFILEvariable now points to the relocated utils taskfile underutils/utils.yaml. Ensure this aligns with the actual file location in theyscope-dev-utilssubmodule.components/core/tests/test-ir_encoding_methods.cpp (2)
15-15: Switch to external Catch2 include path
Replacing the submodule include with<catch2/catch.hpp>aligns with the externally managed Catch2 dependency.
17-17: Switch to external nlohmann::json include path
Using<nlohmann/json.hpp>matches the standard include path for the external JSON library.components/core/src/clp/clp/CMakeLists.txt (2)
150-151: Verify external SQLite3 source directory variable.You’ve replaced hardcoded submodule paths with
${CLP_SQLITE3_SOURCE_DIRECTORY}forsqlite3.candsqlite3.h. Ensure that${CLP_SQLITE3_SOURCE_DIRECTORY}is defined (e.g., via the dependency settings include) before this file is processed and points to the correct checkout/install location.
179-185: Link against imported CMake targets for date, JSON and YAML.You’ve added
date::date,nlohmann_json::nlohmann_jsonand simplifiedyaml-cpplinkage. Confirm thatfind_package(date),find_package(nlohmann_json), andfind_package(yaml-cpp)are invoked in the top-level CMakeLists and that their imported targets match these names.Also applies to: 187-187, 190-190
components/core/src/clp_s/CMakeLists.txt (2)
212-214: Ensuredate::datetarget is available.The
clp_s_TimestampPatternlibrary now linksdate::date. Verify thatfind_package(date REQUIRED)is in the top-level CMakeLists so that the target is defined and the appropriate include paths are propagated.
236-243: Updateclp-sto link JSON, simdjson and YAML targets.You’ve added
nlohmann_json::nlohmann_json,simdjson::simdjson, andyaml-cpptoclp-s. Please confirm that these packages are discovered upstream (find_package) and that the imported target names and versions align with the expected installs.components/core/src/clp/clg/CMakeLists.txt (2)
120-123: Use variable-based SQLite3 sources.You’ve introduced
${CLP_SQLITE3_SOURCE_DIRECTORY}forsqlite3.c,sqlite3.h, andsqlite3ext.h. Validate that this variable is set correctly (via dependency settings) before inclusion so the sources are found during configure/generate.
138-144: Align link libraries with imported targets.You’ve added
date::date,nlohmann_json::nlohmann_json, and replacedyaml-cpp::yaml-cppwithyaml-cpp. Ensure that the root CMakeLists invokes the appropriatefind_packagecalls so these imported targets exist and version boundaries are consistent.Also applies to: 148-148
components/core/CMakeLists.txt (10)
99-114: Conditional include for external dependency settings.The
if (PROJECT_IS_TOP_LEVEL)block loads CMake settings for external tasks, falling back if not found. This is a solid pattern to decouple subprojects. Ensure the relative path../../build/deps/core/cmake-settings/all.cmakematches your repo layout in all CI and local dev cases.
116-119: Discover Abseil viafind_package.Adding
find_package(absl REQUIRED)and printing the version is good for transparency. You might want to guard usage of specific Abseil components (e.g.,flat_hash_map) by listing them as imported targets if needed.
141-145: Add Catch2 to the test suite.You’ve introduced
find_package(Catch2 REQUIRED)and echo the version. Confirm that you link againstCatch2::Catch2in the test target—and not the legacyCatch2WithMain—so tests are driven by your own main.
146-150: Externalisedatedependency.Finding
dateexternally and printing its version ensures version consistency with other projects. The imported targetdate::dateshould now be used across all subdirectories.
162-166: Externaliselog_surgeon.Calling
find_package(log_surgeon REQUIRED)is correct. Just verify that downstream subprojects link tolog_surgeon::log_surgeon(or adjust if the target name differs).
167-170: Externalisenlohmann_json.Switching to
find_package(nlohmann_json)and linkingnlohmann_json::nlohmann_jsonis the recommended approach. No further changes needed here.
172-176: Externalisesimdjson.Analogous to JSON,
find_package(simdjson REQUIRED)is correct. Ensure you usesimdjson::simdjsondownstream.
261-265: Externaliseyaml-cpp.Adding
find_package(yaml-cpp REQUIRED)and using the simple targetyaml-cppmatches CMake’s modern usage.
267-269: Include YSTDLIB subdirectory via variable.You now drive
add_subdirectory("${CLP_YSTDLIB_SOURCE_DIRECTORY}" ...)rather than a hardcoded path. ConfirmCLP_YSTDLIB_SOURCE_DIRECTORYis passed correctly (e.g., via upstream CMake settings) so builds outside of CI still work.
689-702: Link test executable with modern imported targets.You’ve added
Catch2::Catch2,date::date,nlohmann_json::nlohmann_json,simdjson::simdjson, andyaml-cpp. Verify all imported targets align with those discovered viafind_package, and consider grouping them by functionality or adding comments for clarity.taskfiles/deps/utils.yaml (3)
3-5: Validate included utility taskfile path.You reference
../../tools/yscope-dev-utils/exports/taskfiles/utils/utils.yaml. Ensure this relative path resolves correctly in CI and local checkouts. A typo or misplaced folder could break checksum and install tasks.
11-19:clean-old-core-checksum-filestask clears stale checksums.This internal task removes all
.md5files underG_DEPS_CORE_CHECKSUMS_DIR. It’s the right approach to prevent orphaned checksum entries when deps change.
31-39:inittask bootstraps dependency directories.Creating directories for deps, core, checksums, and CMake settings is essential. Confirm that any permission issues are handled upstream or documented for first-time users.
taskfiles/deps/main.yaml (16)
1-2: Ensure correct Taskfile version directive
Taskfile specifiesversion: "3". Confirm your project’s Task runner supports v3 syntax (e.g., Task v3.x) to avoid version mismatch errors.
3-6: Verify included utility files exist and paths are accurate
Theincludes:block references./utils.yamland../../tools/yscope-dev-utils/exports/taskfiles/utils/utils.yaml. Please ensure these files are present at those relative locations to prevent include failures at runtime.
21-26: Default task orchestrates core and log-viewer
Having thedefaulttask depend on bothcoreandlog-vieweraligns with the PR’s goal of migrating to task-based installs.
27-39: Critical: Validate safe cleanup for CMake settings directory
Thecoretask runsrm -rf '{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}'. Double-check thatG_DEPS_CORE_CMAKE_SETTINGS_DIRis correctly scoped to avoid accidental deletion of unrelated directories.
40-51: Verify definition ofG_LOG_VIEWER_WEBUI_SRC_DIR
Thelog-viewertask writes to{{.G_LOG_VIEWER_WEBUI_SRC_DIR}}/yscope-log-viewer, but that variable isn’t set here. Ensureutils:init(or another include) definesG_LOG_VIEWER_WEBUI_SRC_DIR.
52-68: Parallelise dependency installation withcore-all-parallel
Using an internal, once-only task that depends on each individual dependency task is efficient and clean—nice abstraction.
69-82: Verify Abseil remote CMake install configuration
Theabsltask setsLIB_NAME: "absl". Confirm your CMakeLists usefind_package(absl)(or the correct target name) so that the generated settings align with your CMake expectations.
117-130: Catch2 installation is configured correctly
Disabling tests (-DBUILD_TESTING=OFF) and setting Release build ensures Catch2 won’t bloat the core build—approved.
131-143: Date library installation appears correct
The task mirrors the standard CMake-based pattern—no issues detected.
144-156: Log-surgeon CMake install configuration
Consistent use ofutils:install-remote-cmake-libhere fits the overall structure.
157-170: nlohmann::json CMake install parameters
You’ve turned off tests with-DJSON_BuildTests=OFF, which is recommended for a production build.
171-194: Outcome download & CMake settings
The two-step process (download/extract then write CMake snippet) is well-structured and clear.
195-207: Simdjson CMake installation is consistent
Follows the established pattern perfectly—no changes needed.
208-229: SQLite3 extraction & settings generation
Downloading the official amalgamation zip is appropriate. Confirm that the version inZIP_FILENAME_STEM(3360000) matches the version your CMakeLists expect.
230-247: UTFCPP extraction and CMake settings
This mirrors the other non-CMake downloads and looks correct.
248-261: YAML-CPP remote CMake installation
Disabling tests and using Release build is consistent with other CMake-based libraries.
LinZhihao-723
left a comment
There was a problem hiding this comment.
Overall lgtm. Some small comments.
Also verified that coderabit's comments about submodules are valid.
| ) | ||
| target_include_directories(unitTest | ||
| PRIVATE | ||
| ${CLP_OUTCOME_SOURCE_DIRECTORY} |
There was a problem hiding this comment.
Nit: Shall we use CLP_XXX_HEADER_DIRECTORY instead for non-cmake header-only libs?
It's a bit misleading that we include "source" directory where there're no source files being directly compiled.
There was a problem hiding this comment.
How about CLP_XXX_INCLUDES_DIRECTORY?
| sqlite3: | ||
| internal: true | ||
| vars: | ||
| LIB_NAME: "sqlite3" | ||
|
|
||
| # Paths | ||
| EXTRACTION_DIR: "{{.G_DEPS_CORE_DIR}}/{{.LIB_NAME}}-src" | ||
| ZIP_FILENAME_STEM: "sqlite-amalgamation-3360000" | ||
| SRC_DIR: "{{.EXTRACTION_DIR}}/{{.ZIP_FILENAME_STEM}}" | ||
| run: "once" | ||
| cmds: | ||
| - task: "yscope-dev-utils:remote:download-and-extract-zip" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.G_DEPS_CORE_CHECKSUMS_DIR}}/{{.LIB_NAME}}.md5" | ||
| FILE_SHA256: "999826fe4c871f18919fdb8ed7ec9dd8217180854dd1fe21eea96aed36186729" | ||
| OUTPUT_DIR: "{{.EXTRACTION_DIR}}" | ||
| URL: "https://www.sqlite.org/2021/{{.ZIP_FILENAME_STEM}}.zip" | ||
| - >- | ||
| echo "set( | ||
| CLP_SQLITE3_SOURCE_DIRECTORY \"{{.SRC_DIR}}\" | ||
| )" > "{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" |
There was a problem hiding this comment.
Can we set CLP_SQLITE3_SOURCE_DIRECTORY to splite3-src directly and rename sqlite-amalgamation-3360000 to sqlite3 to add one more layer to the sqlite header? In this way, we include sqlite headers using <sqlite3/splite3.h>, which could avoid potential conflicts if other subprojects also have <sqlite3.h> exposed to clp core.
There was a problem hiding this comment.
Same problem for <outcome.hpp>. But since we will move to use boost outcome soon, I'm ok to leave it without modification.
There was a problem hiding this comment.
If we rename the directories, the source directory's checksum will change and the task will think it needs to re-extract the source directory. I could create a new sqlite3-install/include/sqlite3 symlink to sqlite3-src/sqlite-amalgamation-3360000. What do you think?
There was a problem hiding this comment.
sure, I think that works.
| set(SOURCE_FILES_unitTest | ||
| "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3.c" | ||
| "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3.h" | ||
| "${CLP_SQLITE3_SOURCE_DIRECTORY}/sqlite3ext.h" |
There was a problem hiding this comment.
Might be out of this PR's scope: do we need sqlite3ext.h? It's not included in any of CLP core files, only referred in cmake configs.
There was a problem hiding this comment.
Iirc, I did this for CLion, but I guess we could remove it now.
| #include <nlohmann/json.hpp> | ||
| #include <nlohmann/json_fwd.hpp> |
There was a problem hiding this comment.
I see this is to resolve clang-tidy warnings. However, this becomes tricky to use since it's not that clear whether we should include json.hpp or json_fwd.hpp.
Shall we add a wrapped facade header for it later?
There was a problem hiding this comment.
Sounds good. @coderabbitai, can you open an issue in https://github.com/y-scope/ystdlib-cpp to add a wrapped facade header nlohmann/json.hpp that includes nlohamnn/json_fwd.hpp?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create an issue in the ystdlib-cpp repository
gh issue create \
--repo y-scope/ystdlib-cpp \
--title "Add wrapped facade header for nlohmann/json" \
--body "## Description
Add a wrapped facade header for nlohmann/json.hpp that includes nlohmann/json_fwd.hpp to make it clearer which header to include. This is to resolve clang-tidy warnings and simplify usage across the codebase.
## Context
During the migration from submodule-based dependencies to task-based installation in the CLP project, it became unclear whether to include json.hpp or json_fwd.hpp, leading to inconsistent usage.
## Links
- Referenced in PR: https://github.com/y-scope/clp/pull/858
- Requested in comment: https://github.com/y-scope/clp/pull/858#discussion_r2061711641
## Acceptance Criteria
- [ ] Create a facade header that provides a clear, single point of entry for nlohmann/json functionality
- [ ] Ensure it includes necessary forward declarations from json_fwd.hpp
- [ ] Document the usage pattern for consistent implementation across projects
" \
--assignee kirkrodrigues
Length of output: 1058
@kirkrodrigues I've created the issue in the ystdlib-cpp repository as requested. You can find it here: y-scope/ystdlib-cpp#60
The issue includes details about creating a wrapped facade header for nlohmann/json that includes nlohmann/json_fwd.hpp, along with context from this PR discussion and acceptance criteria. I've assigned the issue to you as well.
🧬 Code Graph Analysis Results
Relevant snippets for understanding components/core/src/clp/ffi/KeyValuePairLogEvent.cpp
File: components/core/src/clp/ffi/KeyValuePairLogEvent.hpp
Lines: 45-51, 65-101 (selected excerpts)
-
Declaration of
KeyValuePairLogEvent::createfactory method:create( std::shared_ptr<SchemaTree const> auto_gen_keys_schema_tree, std::shared_ptr<SchemaTree const> user_gen_keys_schema_tree, NodeIdValuePairs auto_gen_node_id_value_pairs, NodeIdValuePairs user_gen_node_id_value_pairs, UtcOffset utc_offset ) -> OUTCOME_V2_NAMESPACE::std_result<KeyValuePairLogEvent>;
-
Accessors for schema trees and node-value pairs:
[[nodiscard]] auto get_auto_gen_keys_schema_tree() const -> SchemaTree const&; [[nodiscard]] auto get_user_gen_keys_schema_tree() const -> SchemaTree const&; [[nodiscard]] auto get_auto_gen_node_id_value_pairs() const -> NodeIdValuePairs const&; [[nodiscard]] auto get_user_gen_node_id_value_pairs() const -> NodeIdValuePairs const&; [[nodiscard]] auto get_utc_offset() const -> UtcOffset;
These declarations provide the interface for creating and accessing the key-value pair log event data, which is implemented in the .cpp file.
File: components/core/src/clp/ffi/SchemaTree.hpp
Line: 166 (single line)
- Method to append a new child node ID to a schema tree node:
auto append_new_child(id_t child_id) -> void { m_children_ids.push_back(child_id); }
This is relevant because the SchemaTree and its nodes are heavily used in the .cpp file for traversing and validating schema nodes.
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp
Lines: 137-180 (summary)
-
Deserializer<IrUnitHandler>::createmethod parses metadata from a reader, validates JSON encoding and protocol version, and returns a deserializer instance or error codes. -
It uses
nlohmann::jsonfor JSON parsing and checks for protocol compatibility.
This snippet is relevant as it shows usage patterns of nlohmann::json and error handling with OUTCOME_V2_NAMESPACE::std_result, which is consistent with the style in KeyValuePairLogEvent.cpp.
File: components/core/src/clp/ffi/ir_stream/Serializer.cpp
Lines: 525-567 (summary)
-
Serializer<encoded_variable_t>::createmethod creates a serializer with optional user-defined metadata, serializes metadata into a buffer, and returns a serializer or error code. -
Uses
nlohmann::jsonto build metadata JSON objects.
This snippet is relevant for understanding how JSON metadata is constructed and serialized, similar to how KeyValuePairLogEvent.cpp serializes node-value pairs into JSON.
File: components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cpp
Lines: 458-482 (full snippet)
[[nodiscard]] auto deserialize_encoded_text_ast_and_insert_to_node_id_value_pairs(
ReaderInterface& reader,
SchemaTree::Node::id_t node_id,
KeyValuePairLogEvent::NodeIdValuePairs& node_id_value_pairs
) -> IRErrorCode {
encoded_tag_t tag{};
if (auto const err{deserialize_tag(reader, tag)}; IRErrorCode::IRErrorCode_Success != err) {
return err;
}
std::string logtype;
std::vector<encoded_variable_t> encoded_vars;
std::vector<std::string> dict_vars;
if (auto const err{deserialize_encoded_text_ast(reader, tag, logtype, encoded_vars, dict_vars)};
IRErrorCode::IRErrorCode_Success != err)
{
return err;
}
node_id_value_pairs.emplace(
node_id,
Value{ir::EncodedTextAst<encoded_variable_t>{logtype, dict_vars, encoded_vars}}
);
return IRErrorCode::IRErrorCode_Success;
}This snippet shows how encoded text ASTs are deserialized and inserted into the node ID-value pairs map, which is the data structure used in KeyValuePairLogEvent.cpp for storing values.
File: components/core/src/clp_s/JsonParser.cpp
Lines: 482-608 (summary)
-
JsonParser::parse()method reads JSON documents, validates them, and handles errors. -
Uses
nlohmann::jsonand simdjson for JSON parsing and error handling. -
Shows patterns of JSON parsing and error handling relevant to the JSON serialization and validation in
KeyValuePairLogEvent.cpp.
Summary of relevance:
-
The header
KeyValuePairLogEvent.hppprovides the interface and data types used in the.cppfile. -
SchemaTree.hppsnippet shows how schema nodes manage children, relevant for tree traversal in serialization. -
ir_stream/Deserializer.hppandSerializer.cppsnippets illustrate JSON metadata handling and error management patterns consistent with the.cppfile. -
ir_unit_deserialization_methods.cppsnippet shows deserialization of encoded text ASTs into the node-value pairs, directly related to theValuetype usage. -
JsonParser.cppsnippet provides context on JSON parsing and error handling, relevant to the JSON serialization logic in the.cppfile.
[End of relevant snippets.]
🧠 Learnings used
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
| INCLUDE_PATTERNS: | ||
| - "*/single-header" | ||
| OUTPUT_DIR: "{{.SRC_DIR}}" | ||
| URL: "https://github.com/ned14/outcome/archive/refs/tags/v2.2.9.tar.gz" |
There was a problem hiding this comment.
Although outcome v2.2.9 recommends not using this tar, this tar is available directly from GitHub rather than githubusercontent.com, meaning it shouldn't get blocked by some dev's firewalls. In addition, I verified that the single-header directory within the previous tar and this tar are identical.
….com; Add comments to document choice of GH-generated source tarball.
LinZhihao-723
left a comment
There was a problem hiding this comment.
I don't have more comments on the changes since the last review.
The PR title lgtm!
…d installation. (y-scope#858) Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Description
Continuing the effort started in #835, this PR migrates core's submodule-based dependencies to task-based installation.
This is similar to the work done in y-scope/spider#115
There are three types of dependencies:
For each dependency, we download it, extract it (if necessary), install it, and then generate a CMake-settings file that contains any CMake variables necessary to reference it. After all dependencies are installed, we generate a single CMake-settings file, that includes all the other settings files, and include this single settings file in core's CMakeLists.txt.
For each dependency, we also generate a checksum file and amalgamate all of these checksum files into a single checksum file that the package build can depend on to detect changes in any of the dependencies.
For CMake-based libraries, we reference them in core's CMakeLists.txt using
find_package.For non-CMake-based libraries, we reference them in core's CMakeLists files either using
add_subdirectory, or if the library is header-only, by adding them as include directories intarget_include_directories.Below are the dependencies migrated and any nuances of the migration:
find_packagerather thanadd_subdirectory.abseil-cpptoabslto match the name of the CMake target.ANTLR_EXECUTABLECMake variable to the dependency's CMake settings file.components/core/third-partydirectory from.gitignoresince it's no longer used.find_package.Catch2tocatch2to be consistent with our task-naming conventions.find_package.find_packagerather thanadd_subdirectory.find_package.jsonwithnlohmann_jsonin .clang-format.unzipandxzto the core dependency container images that require it.jsontonlohmann_jsonto match the name of the CMake target.find_packagerather thanadd_subdirectory.Included in core's CMakeLists.txt using
find_packagerather thanadd_subdirectory.Changed CMake-target from yaml-cpp:: yaml-cpp to yaml-cpp to resolve this error:
Changed include path from yaml-cpp/include/yaml-cpp/yaml.h to yaml-cpp/yaml.h.
Removed its submodule.
ystdlib-cpptoystdlibto match the name of the CMake target.To support the above, we also:
taskfiles/deps.yamltotaskfiles/deps/main.yamland split any utilities intotaskfiles/deps/utils.yaml.LAZYinstall messages to reduce the installation output.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
unzip,xz).