chore(core): Add ystdlib-cpp library as a submodule.
#730
Conversation
WalkthroughThis pull request integrates the "ystdlib-cpp" library as a new dependency across the repository. A new submodule entry is added in the repository’s submodule configuration, and the build process is updated in the CMake files to include this dependency. The dependency management tasks are modified to download, validate, and integrate the library, and the project’s formatting configuration and documentation are updated to recognize the new library and adjust related version information. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant BuildSystem
participant DependencyManager
participant GitHubRepo
Developer->>BuildSystem: Initiate build
BuildSystem->>DependencyManager: Execute dependency tasks
DependencyManager->>GitHubRepo: Download "ystdlib-cpp" from repository
GitHubRepo-->>DependencyManager: Provide submodule repository snapshot
DependencyManager->>BuildSystem: Validate checksum using G_YSTDLIB_CPP_CHECKSUM_FILE
BuildSystem->>Developer: Proceed with build including "ystdlib-cpp"
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
ystdlib-cpp as a git submodule and migrate to use ystdlib::error_handling::ErrorCode.ystdlib-cpp as a git submodule and migrate to use ystdlib::error_handling::ErrorCode.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/core/CMakeLists.txt (1)
219-223: New ystdlib-cpp Submodule Integration with BUILD_TESTING ToggleThe submodule for ystdlib-cpp is added here by temporarily disabling tests (using
set(BUILD_TESTING OFF)) before including the submodule and then re-enabling testing afterwards. Please verify that this global toggle does not inadvertently affect tests in other parts of the project. It might be beneficial to consider a submodule-specific variable (for example,YSTDLIB_BUILD_TESTING) to localise this behaviour rather than modifying the globalBUILD_TESTINGflag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/core/CMakeLists.txt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
695-696: Linking against the New ystdlib::error_handling LibraryThe update to include
ystdlib::error_handlingin thetarget_link_librariessection reflects the migration of error handling to the new structure. Ensure that all references and dependencies match the symbols provided by the new library and that there are no lingering conflicts with previous error handling components.
There was a problem hiding this comment.
We should update deps-tasks.yml as well to include ystdlib-cpp. Check this PR for reference (which added utfcpp): #701
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deps-tasks.yml (1)
470-499: Ystdlib-CPP Task Definition
A new task definition forystdlib-cpphas been introduced. It mirrors the structure of similar submodule tasks by setting:
internal: true- Defining
varsforCHECKSUM_FILEandDEST(which usesG_YSTDLIB_CPP_CHECKSUM_FILEandG_CORE_COMPONENT_SUBMODULES_DIR)- Listing the required
sources(download script and task files)- Including a dependency on
:initand checksum validation via:utils:validate-checksum- Specifying commands that leverage the
download-dependencytask and then compute the checksum.Please verify that the
SRC_NAME(ystdlib-cpp-95d89d148312c7c447f4164766ad35dd7996b1cc) andSRC_URL(https://github.com/y-scope/ystdlib-cpp/archive/95d89d1.zip) accurately reflect the intended version of the library. If further versioning information is available, consider adding clarifying comments or documentation for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitmodules(1 hunks)components/core/CMakeLists.txt(2 hunks)components/core/submodules/ystdlib-cpp(1 hunks)deps-tasks.yml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/core/submodules/ystdlib-cpp
- .gitmodules
- components/core/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build (macos-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (4)
deps-tasks.yml (4)
23-23: New Checksum Variable Addition
The new variableG_YSTDLIB_CPP_CHECKSUM_FILEis added in thevarssection in a style consistent with the existing dependencies. This ensures that the ystdlib-cpp checksum file is tracked during dependency generation.
45-45: Include in Core Task Sources
TheG_YSTDLIB_CPP_CHECKSUM_FILEvariable is now incorporated into the list of sources for thecoretask. This inclusion is consistent with similar dependencies, ensuring its checksum is accounted for during the core dependency processing.
62-62: Checksum Command Update
The checksum file for ystdlib-cpp is appended into thecmdslist of thecoretask (i.e. passed to thecatcommand). Verify that its position in the concatenated list meets the intended build and ordering requirements.
105-105: Internal Dependency Task List Integration
The new taskystdlib-cppis now added to theall-internal-depstask list. This ensures that the submodule is processed as part of the sequential download and checksum verification steps for internal dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/clp-core-build-macos.yaml(1 hunks).github/workflows/clp-core-build.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/clp-core-build.yaml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/clp-core-build-macos.yaml
73-73: property "os" is not defined in object type {runner: string; use_shared_libs: bool}
(expression)
73-73: property "os" is not defined in object type {runner: string; use_shared_libs: bool}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (1)
.github/workflows/clp-core-build-macos.yaml (1)
61-65: Approval: Coreutils and Apple Clang 16 Installation StepThis new step correctly installs the necessary dependencies (coreutils and Apple Clang 16) required for macOS builds. The use of the multi-line command with the
|-syntax is appropriate.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Bill-hbrhbr
force-pushed
the
migrate-error-handling
branch
from
a592641 to
86670f4
Compare
There was a problem hiding this comment.
Can we only add ystdlib-cpp in this PR and not modify/fix other issues?
There was a problem hiding this comment.
This change seems to have not been made even though you resolved it. Did something go wrong?
components/core/CMakeLists.txt
Outdated
| add_subdirectory(submodules/yaml-cpp EXCLUDE_FROM_ALL) | ||
|
|
||
| # Add ystdlib-cpp | ||
| option(YSTDLIB_CPP_BUILD_TESTING "" OFF) |
There was a problem hiding this comment.
No change necessary, just a question for my own understanding.
Using option here someone could override this and build the tests for ystdlib-cpp right? (As opposed to using set which would enforce no tests built without editing the cmake scripts.)
There was a problem hiding this comment.
set(YSTDLIB_CPP_BUILD_TESTING OFF)
set(YSTDLIB_CPP_BUILD_TESTING ON CACHE BOOL "")
option(YSTDLIB_CPP_BUILD_TESTING "" ON)
Tried this code snippet. Neither cache setting can override the first line. So I guess
set(YSTDLIB_CPP_BUILD_TESTING OFF)
is good
Bill-hbrhbr
changed the title
ystdlib-cpp as a git submodule and migrate to use ystdlib::error_handling::ErrorCode.ystdlib-cpp library as a submodule.
Description
ystdlib-cppas a git submodule.ystdlib-cpp.ystdlibto.clang-formatas 3rd party lib includes.add_subdirectory()in CLP CMakeLists.txt.Checklist
breaking change.
Validation performed
ystdlib-cppcan be downloaded and successfully compiled along withclp-core.Summary by CodeRabbit
New Features
ystdlib-cpp, to expand project capabilities.Chores
ystdlib-cpp.ystdliblibrary.Documentation
ystdlib-cppwith its GitHub reference.