Create an example of including UnoDB as a CMake subdirectory #633
Conversation
WalkthroughThe pull request introduces comprehensive modifications to the build configuration across multiple files, focusing on restructuring how examples are built and managed. The changes primarily affect CI/CD workflows, CMake configurations, and dependency management. The modifications aim to provide more flexibility in building examples, conditional library inclusion, and streamline the build process across different platforms and compiler configurations. Changes
Sequence DiagramsequenceDiagram
participant CI as CI/CD Workflow
participant CMake as CMake Configuration
participant Deps as Dependencies
participant Examples as Example Builds
CI->>CMake: Check sanitizer flags
CMake-->>CI: Determine build conditions
CI->>Deps: Conditionally install dependencies
Deps-->>CI: Prepare build environment
CI->>Examples: Build examples if conditions met
Examples-->>CI: Report build status
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
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: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(2 hunks).gitignore(1 hunks)CMakeLists.txt(5 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[formatting] ~67-~67: Consider inserting a comma after an introductory phrase for better readability.
Context: ...ct, ON if you work on UnoDB itself. In this case the vendored development dependencies...
(IN_THAT_CASE_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: SonarCloud
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with TSan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Intel), macos-latest, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Intel), macos-latest, Debug, macos-clang, ON, OFF)
🔇 Additional comments (18)
.gitignore (1)
49-49: LGTM!Appropriate addition to ignore the compile commands database for the examples build directory.
examples/CMakeLists.txt (2)
8-10: LGTM! Well-structured project configuration.Good practice to include version, description, and homepage URL in the project configuration.
15-17: Verify package version compatibility.The find_package commands don't specify version requirements. Consider adding minimum version requirements to ensure compatibility.
Run this script to check the minimum required versions used in the main UnoDB CMakeLists.txt:
test/CMakeLists.txt (1)
29-30: LGTM! Improved Google Test dependency management.Good improvement to use namespaced targets (GTest::) instead of raw targets. This is a CMake best practice that provides better dependency management and clarity.
Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (1)
205-208: LGTM! Well-organized build directory structure.Good practice to create separate build directories for the main project and examples.
.circleci/config.yml (1)
70-118: LGTM! Well-structured example build configuration.The changes appropriately handle:
- Conditional example building when sanitizers are disabled
- Compiler-specific configurations for both Linux and macOS
- Proper package installation and build directory setup
Also applies to: 215-241
CMakeLists.txt (4)
348-350: LGTM! Well-structured conditional target addition.The conditional check for
Microsoft.GSL::GSLtarget prevents conflicts when UnoDB is included as a subdirectory in projects that already have GSL configured.
384-392: LGTM! Well-structured conditional Google Test integration.The changes demonstrate good CMake practices:
- Prevents duplicate target definitions
- Properly handles Windows-specific settings with
gtest_force_shared_crt- Correctly manages compiler flags using the ADD_CXX_FLAGS_FOR_SUBDIR macro
394-418: LGTM! Well-structured Google Benchmark integration.The changes demonstrate excellent CMake practices:
- Prevents duplicate target definitions
- Properly configures benchmark options (testing and installation)
- Handles LTO configuration comprehensively based on build type and compiler
- Correctly manages compiler flags using the ADD_CXX_FLAGS_FOR_SUBDIR macro
512-515: LGTM! Well-structured benchmark compile definitions.The conditional compile definitions for the benchmark target are properly scoped and handle GCC debug mode correctly.
.github/workflows/build.yml (4)
289-289: LGTM! Proper submodules handling.Setting
submodules: falsein the initial checkout aligns with making the dependencies optional.
359-365: LGTM! Proper macOS dependencies setup.The dependencies setup for macOS examples is well-structured with correct package names and proper conditions.
367-370: LGTM! Well-structured build environment setup.The creation of separate build directories for the main project and examples is clean and well-organized.
371-420: LGTM! Well-structured CMake configuration for examples.The configuration:
- Properly handles different compilers and their versions
- Correctly sets up LLVM toolchain for Release builds
- Has appropriate conditions for skipping builds with sanitizers
.github/workflows/old-compilers.yml (4)
449-449: LGTM! Consistent submodules handling.Setting
submodules: falsematches the changes in build.yml and aligns with making dependencies optional.
488-493: Package name typos already reported.The same package name typos exist here as in build.yml.
495-498: LGTM! Consistent build environment setup.The creation of separate build directories matches the structure in build.yml.
499-531: LGTM! Consistent CMake configuration for examples.The configuration matches the structure in build.yml with appropriate conditions for skipping builds with sanitizers.
laurynas-biveinis
force-pushed
the
parent-deps
branch
from
6872415 to
5092fb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
343-349:⚠️ Potential issueFix package name typos in apt-get install command.
The package names contain typos:
-sudo apt-get install -y libmsgsl-dev libgtest-dev libbencmark-dev +sudo apt-get install -y libmsgsl-dev libgtest-dev libbenchmark-dev
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(2 hunks).gitignore(1 hunks)CMakeLists.txt(5 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
🔇 Additional comments (13)
.gitignore (1)
49-49: LGTM!Appropriate addition to ignore the compile commands JSON file in the examples directory.
examples/CMakeLists.txt (2)
1-20: LGTM! Well-structured CMake configuration.The conversion to a top-level CMake script is well done, with clear project metadata and proper package dependencies.
32-34: Consider adding error handling for example execution.The example targets are executed directly without checking their exit status.
test/CMakeLists.txt (1)
29-30: LGTM! Good modernization of CMake targets.The update to use namespaced GTest targets (
GTest::gtest_main,GTest::gmock_main) follows CMake best practices.Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (2)
198-202: Fix package name typo.The package name contains a typo: 'libbencmark-dev' should be 'libbenchmark-dev'.
205-241: LGTM! Well-structured workflow for examples.The workflow correctly handles example builds when sanitizers are disabled, with proper environment setup and build steps.
.circleci/config.yml (2)
70-118: LGTM! Well-structured Linux example configuration.The Linux example build setup is properly configured with correct dependency installation and build steps.
215-241: LGTM! Well-structured macOS example configuration.The macOS example build setup correctly uses Homebrew for dependencies and maintains consistency with the Linux configuration.
README.md (1)
39-39: LGTM: CMake version update matches implementation.The minimum CMake version requirement of 3.16 aligns with the changes in CMakeLists.txt.
.github/workflows/old-compilers.yml (1)
495-498: LGTM: Two-phase build setup looks good.The workflow correctly implements a two-phase build strategy:
- First phase builds examples using system-provided dependencies
- Second phase builds UnoDB itself using submodules
This demonstrates both usage patterns effectively.
Also applies to: 499-524, 525-531
.github/workflows/build.yml (1)
359-365: LGTM: macOS dependency setup looks good.The macOS-specific package installation using Homebrew is correct and includes all necessary dependencies.
CMakeLists.txt (2)
348-350: LGTM: Conditional subdirectory additions are well-implemented.The changes correctly implement conditional subdirectory additions for GSL, Google Test, and Google Benchmark. This allows UnoDB to:
- Use existing targets when available from the parent project
- Fall back to bundled versions when needed
The implementation follows CMake best practices for dependency management.
Also applies to: 384-392, 394-418
512-515: LGTM: Conditional compile definitions are properly scoped.The compile definitions for the benchmark target are correctly wrapped in a conditional block to avoid modifying targets from the parent project.
laurynas-biveinis
force-pushed
the
parent-deps
branch
from
5092fb9 to
34e417a
Compare
✅ Actions performedFull review triggered. |
laurynas-biveinis
force-pushed
the
parent-deps
branch
6 times, most recently
from
9cc1ce4 to
d86fe48
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/msvc-build.yml(0 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(2 hunks).gitignore(1 hunks)CMakeLists.txt(5 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/msvc-build.yml
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with ASan (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
- GitHub Check: build (XCode Debug with cppcheck (Apple Silicon), macos-14, Debug, macos-clang, ON, OFF)
🔇 Additional comments (22)
.gitignore (1)
49-49: LGTM!Appropriate addition to ignore the compile commands database in the examples directory, consistent with the existing ignore pattern for the root directory.
examples/CMakeLists.txt (2)
1-10: LGTM!Clear project setup with appropriate documentation explaining this is a top-level CMake script demonstrating UnoDB integration.
32-34: Consider adding error handling for example execution.The example targets are executed directly without checking their exit status.
- env ./example_art - COMMAND env ./example_art_stats - COMMAND env ./example_olc_art + env bash -c './example_art || exit 1' + COMMAND env bash -c './example_art_stats || exit 1' + COMMAND env bash -c './example_olc_art || exit 1'test/CMakeLists.txt (1)
29-30: LGTM! Good modernization of CMake targets.Excellent update to use modern CMake namespaced targets (GTest::) consistently across all test-related targets.
Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (1)
198-202: Fix package name typo.The package name contains a typo: 'libmsgsl-dev' should be 'libmsgsl-dev'.
- run: sudo apt-get install -y libmsgsl-dev libgtest-dev libbenchmark-dev + run: sudo apt-get install -y libmsgsl-dev libgtest-dev libbenchmark-dev.circleci/config.yml (1)
70-118: LGTM! Well-structured example setup.The example setup is well-organized and consistently implemented for both Linux and macOS builds. The conditional logic for sanitizer checks is clear and maintainable.
Also applies to: 215-241
CMakeLists.txt (5)
348-350: LGTM! Well-structured conditional dependency inclusion.The conditional check for Microsoft.GSL::GSL target before adding the subdirectory follows CMake best practices for optional dependencies.
384-392: LGTM! Proper handling of Google Test for Windows builds.Good practice to:
- Preserve the Windows-specific setting for shared CRT
- Apply sanitizer flags before adding the subdirectory
- Restore flags after adding the subdirectory
394-418: LGTM! Comprehensive Google Benchmark configuration.Well-structured configuration that:
- Disables unnecessary features (testing, installation)
- Handles LTO appropriately for different build types and compilers
- Properly manages compiler flags
512-515: LGTM! Conditional compile definitions for benchmark target.The compile definitions are now properly guarded to avoid modifying an existing target.
738-738: LGTM! Simplified valgrind dependencies.Removed valgrind_examples dependency as it's now handled separately in the examples directory.
README.md (3)
39-39: LGTM! Updated minimum CMake version requirement.The minimum CMake version has been updated to 3.16, which aligns with the features used in the build system.
44-47: LGTM! Clear documentation of optional dependencies.The documentation now correctly describes GSL, Google Test, and Google Benchmark as optional git submodules that can be provided by the parent project.
70-74: LGTM! Clear standalone build instructions.The instructions for standalone builds are now clearer about when submodules are required and what features are enabled.
.github/workflows/old-compilers.yml (4)
449-449: LGTM! Disabled initial submodule checkout.Correctly disables submodule checkout initially to test example builds without vendored dependencies.
488-493: LGTM! Proper dependency setup for examples.Dependencies are installed only when sanitizers are not enabled, which is correct as examples should be built with system-provided dependencies.
494-520: LGTM! Well-structured CMake configuration for examples.The configuration properly:
- Creates a separate build directory for examples
- Sets up compiler-specific flags and tools
- Handles both GCC and Clang builds
521-527: LGTM! Clean separation of example and main builds.Good practice to:
- Build examples first without submodules
- Then checkout submodules for the main build
Also applies to: 528-530
.github/workflows/build.yml (4)
289-289: LGTM! Disabled initial submodule checkout.Correctly disables submodule checkout initially to test example builds without vendored dependencies.
343-349: LGTM! Platform-aware dependency setup.Well-structured platform-specific dependency installation:
- Linux uses apt-get for package installation
- macOS uses brew for package installation
- Both are properly conditioned on sanitizer and analysis flags
Also applies to: 359-365
366-406: LGTM! Comprehensive CMake configuration for examples.The configuration properly handles:
- Platform-specific compiler setup
- Build type configuration
- LLVM toolchain setup for clang builds
- macOS-specific settings
407-414: LGTM! Clean separation of example and main builds.Good practice to:
- Build examples first without submodules
- Then checkout submodules for the main build
Also applies to: 415-417
laurynas-biveinis
force-pushed
the
parent-deps
branch
from
d86fe48 to
512fc0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/msvc-build.yml(0 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(1 hunks).gitignore(1 hunks)CMakeLists.txt(5 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/msvc-build.yml
🔇 Additional comments (22)
.gitignore (1)
49-49: LGTM!Good addition to ignore the compilation database in the examples directory, consistent with the existing entry for the root directory.
examples/CMakeLists.txt (2)
33-35: 🧹 Nitpick (assertive)Consider adding error handling for example execution.
The example targets are executed directly without checking their exit status. Consider wrapping them in a script that checks return values.
- env ./example_art - COMMAND env ./example_art_stats - COMMAND env ./example_olc_art + env bash -c './example_art || exit 1' + COMMAND env bash -c './example_art_stats || exit 1' + COMMAND env bash -c './example_olc_art || exit 1'Likely invalid or redundant comment.
15-18: 🧹 Nitpick (assertive)Add version constraints for dependencies.
While finding the required packages is good, consider adding version requirements to ensure compatibility.
-find_package(Microsoft.GSL REQUIRED) -find_package(GTest REQUIRED) -find_package(benchmark REQUIRED) +find_package(Microsoft.GSL 4.0 REQUIRED) +find_package(GTest 1.14 REQUIRED) +find_package(benchmark 1.8 REQUIRED)Likely invalid or redundant comment.
test/CMakeLists.txt (1)
29-30: LGTM! Good use of namespaced targets.The changes correctly update the library linkage to use namespaced targets (
GTest::gtest_mainandGTest::gmock_main) instead of bare library names. This is a CMake best practice that ensures proper target propagation.Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (1)
203-204: LGTM! Explicit build directory handling.Good practice to explicitly create and change to the build directory, making the workflow more reliable and easier to understand.
.circleci/config.yml (2)
70-118: LGTM! Well-structured conditional example builds for Linux.Good implementation of conditional example builds when sanitizers are disabled. The setup properly handles:
- Build environment creation
- Dependency installation
- CMake configuration with compiler-specific settings
215-241: LGTM! Well-structured conditional example builds for macOS.Good implementation of conditional example builds for macOS, following the same pattern as Linux. The setup correctly:
- Creates build environment
- Installs macOS-specific dependencies using brew
- Configures CMake appropriately
CMakeLists.txt (5)
348-350: LGTM! Good improvement to GSL dependency handling.The conditional check ensures GSL is only added as a subdirectory if not already provided by the parent project, improving flexibility when UnoDB is used as a CMake subdirectory.
384-392: LGTM! Well-structured Google Test integration.The changes properly handle:
- Conditional inclusion of Google Test
- Windows-specific CRT settings
- Compilation flags management
394-418: LGTM! Comprehensive Google Benchmark setup.The implementation properly handles:
- Conditional inclusion of Google Benchmark
- Optimization of build by disabling unnecessary features
- Careful management of LTO support across different build configurations
512-515: LGTM! Safe handling of benchmark compile definitions.The conditional check prevents modifying compile definitions of a potentially parent-owned benchmark target.
738-738: LGTM! Appropriate update to valgrind target dependencies.Removed valgrind_examples dependency to align with the separation of examples into their own CMake project.
.github/workflows/old-compilers.yml (3)
449-449: LGTM! Appropriate update to checkout behavior.Disabled automatic submodule initialization to support optional dependencies when used as a subdirectory.
488-493: Fix package name typos in apt-get install command.The package names contain typos:
-sudo apt-get install libmsgsl-dev libgtest-dev libbencmark-dev +sudo apt-get install libmsgsl-dev libgtest-dev libbenchmark-dev
494-520: LGTM! Well-structured CMake configuration for examples.The configuration properly handles different compilers and build types while maintaining consistent paths and flags.
.github/workflows/build.yml (4)
289-289: LGTM! Consistent checkout behavior.Matches the submodule handling in old-compilers.yml.
343-349: Fix package name typos in apt-get install command.The package names contain typos:
-sudo apt-get install libmsgsl-dev libgtest-dev libbencmark-dev +sudo apt-get install libmsgsl-dev libgtest-dev libbenchmark-dev
359-365: LGTM! Good cross-platform support for examples.Properly handles macOS-specific package installation using brew.
366-406: LGTM! Excellent cross-platform CMake configuration.The configuration comprehensively handles:
- Multiple compiler types
- Platform-specific settings
- Build type variations
README.md (3)
39-39: LGTM! Appropriate CMake version requirement.The minimum CMake version of 3.16 aligns with the project's needs.
44-47: LGTM! Clear dependency documentation.The updated section clearly explains the optional nature of the dependencies and their relationship with parent projects.
70-74: LGTM! Clear build instructions.The documentation clearly explains when and why submodules are required, and the implications of the STANDALONE option.
laurynas-biveinis
force-pushed
the
parent-deps
branch
3 times, most recently
from
52ee611 to
e6c3056
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
examples/CMakeLists.txt (3)
2-2: 🧹 Nitpick (assertive)Consider increasing the minimum CMake version requirement.
The current minimum version (3.16) is from December 2019. Consider using a more recent version (e.g., 3.20) to access newer CMake features and improvements.
16-18: 🧹 Nitpick (assertive)Consider version constraints for dependencies.
While finding the required packages is good, consider adding version requirements to ensure compatibility.
33-35: 🧹 Nitpick (assertive)Consider adding error handling for example execution.
The example targets are executed directly without checking their exit status. Consider wrapping them in a script that checks return values.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/msvc-build.yml(0 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(1 hunks).gitignore(1 hunks)CMakeLists.txt(7 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/msvc-build.yml
🔇 Additional comments (12)
.gitignore (1)
49-49: LGTM!Appropriate addition to ignore the compile commands database in the examples directory.
test/CMakeLists.txt (1)
29-30: LGTM! Improved CMake target usage.Good improvement using modern CMake practices with namespaced targets for Google Test dependencies.
Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (1)
203-204: LGTM! Improved workflow organization.Good consolidation of build directory creation with the CMake configuration step.
.circleci/config.yml (1)
70-118: LGTM! Well-structured example build configuration for Linux.Good implementation of conditional example builds with proper dependency management and build isolation.
CMakeLists.txt (5)
350-352: LGTM! Well-implemented conditional GSL integration.The condition correctly checks for the existence of the GSL target before adding the subdirectory, allowing UnoDB to use GSL provided by the parent project.
386-394: LGTM! Well-implemented conditional Google Test integration.The implementation:
- Correctly checks for GTest target existence
- Properly handles Windows CRT settings
- Maintains consistent build flags
396-420: LGTM! Well-implemented conditional Google Benchmark integration.The implementation:
- Correctly checks for benchmark target existence
- Properly handles testing and installation settings
- Includes well-conditioned LTO support
- Maintains consistent build flags
514-517: LGTM! Proper handling of benchmark compile definitions.The condition correctly prevents modifying the benchmark target when it's provided by the parent project.
757-764: LGTM! Well-implemented conditional test and benchmark directory additions.The implementation:
- Correctly conditions subdirectory additions on target existence
- Provides helpful error messages for standalone builds
- Includes appropriate informational messages for non-standalone builds
Also applies to: 771-778
.github/workflows/build.yml (1)
343-351: LGTM! Well-structured workflow changes for building examples.The implementation:
- Properly sets up dependencies for both Linux and macOS
- Includes well-conditioned CMake configuration
- Maintains consistent build flags across platforms
Also applies to: 361-367, 368-408, 409-416
.github/workflows/old-compilers.yml (1)
488-495: LGTM! Consistent workflow changes for building examples.The implementation maintains consistency with build.yml while properly handling example builds for older compilers.
Also applies to: 496-522, 523-529
README.md (1)
39-39: LGTM! Clear and accurate documentation updates.The changes properly document:
- Updated CMake version requirement
- Optional nature of bundled dependencies
- Updated standalone build instructions
Also applies to: 44-47, 70-74
laurynas-biveinis
force-pushed
the
parent-deps
branch
3 times, most recently
from
26a38a6 to
43c944f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🛑 Comments failed to post (1)
CMakeLists.txt (1)
421-422:
⚠️ Potential issueFix invalid generator expression syntax.
The generator expression is malformed and causing build failures. The condition is missing between
<and:.Apply this fix:
- target_compile_definitions(benchmark PUBLIC - "$<${is_gxx_not_release_standalone}:_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC>") + target_compile_definitions(benchmark PUBLIC + "$<$<BOOL:${is_gxx_not_release_standalone}>:_GLIBCXX_DEBUG;_GLIBCXX_DEBUG_PEDANTIC>")The fix:
- Adds the missing
$<BOOL:...>condition- Separates multiple definitions with semicolons
📝 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.target_compile_definitions(benchmark PUBLIC "$<$<BOOL:${is_gxx_not_release_standalone}>:_GLIBCXX_DEBUG;_GLIBCXX_DEBUG_PEDANTIC>")🧰 Tools
🪛 GitHub Actions: sonarcloud
[error] 421-421: Invalid generator expression: $<:_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC>
🪛 GitHub Actions: CodeQL
[error] 421-421: Error evaluating generator expression: $<:_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC>. Expression did not evaluate to a known generator expression
[warning] Manually-specified variables were not used by the project: BUILD_DOCS, BUILD_DOCUMENTATION, CATKIN_ENABLE_TESTING
🪛 GitHub Actions: experimental-build
[error] 421-421: Invalid generator expression: '$<:_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC>' in target_compile_definitions
🪛 GitHub Actions: msvc-build
[error] 421-421: Error evaluating generator expression: $<:_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC>
[error] Building standalone configuration but no Google Test found - have you initialized git submodules?
laurynas-biveinis
force-pushed
the
parent-deps
branch
2 times, most recently
from
e471de6 to
9eda75e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/msvc-build.yml(0 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(1 hunks)CMakeLists.txt(9 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/msvc-build.yml
🔇 Additional comments (27)
examples/CMakeLists.txt (3)
15-18: Consider adding version requirements for dependencies.While finding the required packages is good, consider adding version requirements to ensure compatibility.
-find_package(Microsoft.GSL REQUIRED) -find_package(GTest) -find_package(benchmark) +find_package(Microsoft.GSL 4.0 REQUIRED) +find_package(GTest 1.14) +find_package(benchmark 1.8)
33-35: Consider adding error handling for example execution.The example targets are executed directly without checking their exit status.
- env ./example_art - COMMAND env ./example_art_stats - COMMAND env ./example_olc_art + env bash -c './example_art || exit 1' + COMMAND env bash -c './example_art_stats || exit 1' + COMMAND env bash -c './example_olc_art || exit 1'
4-7: LGTM: Clear documentation of the file's purpose.The comment clearly explains that this is a top-level CMake script demonstrating UnoDB integration.
test/CMakeLists.txt (1)
29-30: LGTM: Improved CMake target usage.The changes properly use modern CMake namespaced targets for GTest dependencies, which is the recommended approach.
Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (1)
203-204: LGTM: Improved build directory handling.Moving the build directory creation into the Configure CMake step improves organization and reduces script complexity.
.circleci/config.yml (3)
70-118: LGTM: Well-structured example build configuration.The conditional block properly handles example builds when sanitizers are disabled, with appropriate dependency management and build steps.
215-241: LGTM: Consistent macOS configuration.The macOS configuration mirrors the Linux setup while accounting for platform-specific package management.
84-85:⚠️ Potential issueFix package name for Microsoft GSL.
The package name should be
libmsgsl-dev.- sudo apt-get install -y libmsgsl-dev libgtest-dev \ - libgmock-dev + sudo apt-get install -y microsoft-gsl-dev libgtest-dev \ + libgmock-devLikely invalid or redundant comment.
README.md (2)
44-47: LGTM: Clear documentation of dependency options.The documentation clearly explains that dependencies can be provided either as git submodules or by the parent project.
70-74: 🧹 Nitpick (assertive)Consider improving the STANDALONE option documentation.
The explanation of the STANDALONE option and its implications could be clearer.
- if you work on UnoDB itself. In this case, the vendored development - dependencies must be present, i.e. `git submodule update --init --recursive`. - It will enable extra global debug checks that require entire programs to be - compiled with them. Currently, this consists of the libstdc++ debug mode. The - default is `OFF`. + if you work on UnoDB itself. When ON: + * The vendored development dependencies must be initialized using + `git submodule update --init --recursive` + * Extra global debug checks are enabled that require entire programs to be + compiled with them (currently, this consists of the libstdc++ debug mode) + The default is `OFF`.Likely invalid or redundant comment.
.github/workflows/old-compilers.yml (5)
449-449: LGTM: Disabling initial submodule checkoutThe change to disable submodule checkout during the initial checkout is correct, as submodules are checked out later when needed.
488-495: LGTM: Conditional installation of example dependenciesThe dependencies for examples are correctly installed only when sanitizers are disabled, which is appropriate as sanitizer builds don't need these dependencies.
496-522: LGTM: Well-structured CMake configuration for examplesThe configuration correctly:
- Creates a separate build directory for examples
- Sets up compiler variables based on compiler type
- Configures LLVM toolchain for Clang release builds
523-529: LGTM: Appropriate build configuration for examplesThe build step correctly uses parallel builds (-j3) and is properly conditioned on sanitizer flags.
530-532: LGTM: Appropriate timing for submodule checkoutEnabling submodule checkout at this point is correct, as it's needed for the main build.
.github/workflows/build.yml (6)
289-289: LGTM: Consistent submodule checkout configurationThe change to disable initial submodule checkout matches the configuration in old-compilers.yml.
343-351: LGTM: Comprehensive conditional dependency setup for LinuxThe dependencies for examples are correctly installed with appropriate conditions for sanitizers, static analysis, and coverage flags.
361-367: LGTM: Appropriate dependency setup for macOSThe dependencies are correctly installed using brew with proper conditions for sanitizers, static analysis, and coverage flags.
409-416: LGTM: Appropriate build configuration for examplesThe build step correctly uses parallel builds (-j3) and is properly conditioned on all relevant flags.
417-419: LGTM: Consistent submodule checkout timingEnabling submodule checkout at this point matches the configuration in old-compilers.yml.
426-427: LGTM: Standard build directory setupThe build directory creation is correctly placed before CMake configuration.
CMakeLists.txt (6)
4-5: LGTM: Added header check capabilityIncluding CheckIncludeFile is appropriate for the header availability checks used later.
350-352: LGTM: Proper handling of GSL dependencyThe conditional inclusion of GSL prevents double inclusion when UnoDB is used as a subdirectory.
424-433: LGTM: Proper Google Test integrationThe Google Test configuration correctly:
- Prevents double inclusion
- Handles Windows-specific settings
- Manages compiler flags appropriately
491-492: LGTM: Correct DeepState path configurationThe source and binary directory paths are correctly specified relative to the CMake context.
520-526: LGTM: Proper handling of third-party includesUsing SYSTEM for benchmark include directories is the correct approach to suppress warnings from third-party headers.
625-625: LGTM: Appropriate visibility for compile definitionsChanging the compile definitions from PRIVATE to PUBLIC is correct as these definitions need to be available to library consumers.
laurynas-biveinis
force-pushed
the
parent-deps
branch
from
9eda75e to
b3558a9
Compare
- Update UnoDB CMakeLists.txt to use GSL, Google Test, & Google Benchmark CMake targets if they are already set up by parent CMake. - Convert examples/CMakeLists.txt to be a top-level CMake script, showing an example of using UnoDB as a dependency. - Update STANDALONE CMake option to skip building benchmarks and tests when OFF. - Update CI to build examples from the top-level script.
laurynas-biveinis
force-pushed
the
parent-deps
branch
from
b3558a9 to
43934df
Compare
|
laurynas-biveinis
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
CMakeLists.txt (1)
Line range hint
360-399: Consider organizing generator expressions into logical groupsWhile comprehensive, the generator expressions could be better organized with comments grouping related expressions.
+# Platform detection set(is_release_genex "$<CONFIG:Release>") set(is_not_release_genex "$<NOT:${is_release_genex}>") set(is_not_windows_x86_64 "$<STREQUAL:${CMAKE_SYSTEM_PROCESSOR},x86_64>") set(is_windows_x86_64 "$<STREQUAL:${CMAKE_SYSTEM_PROCESSOR},AMD64>") set(is_windows_genex "$<PLATFORM_ID:Windows>") set(is_not_windows "$<NOT:${is_windows_genex}>") set(is_linux "$<PLATFORM_ID:Linux>") +# Compiler detection set(is_gxx_genex "$<CXX_COMPILER_ID:GNU>") set(is_clang_genex "$<CXX_COMPILER_ID:Clang>") set(is_any_clang_genex "$<CXX_COMPILER_ID:AppleClang,Clang>") set(is_msvc "$<CXX_COMPILER_ID:MSVC>") +# Compiler version checks set(cxx_ge_11 "$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,11.0>")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(2 hunks).github/workflows/build.yml(3 hunks).github/workflows/msvc-build.yml(0 hunks).github/workflows/old-compilers.yml(2 hunks).github/workflows/ubuntu-20.04.yml(1 hunks)CMakeLists.txt(9 hunks)README.md(2 hunks)examples/CMakeLists.txt(1 hunks)test/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/msvc-build.yml
🧰 Additional context used
🪛 LanguageTool
README.md
[typographical] ~73-~73: Two consecutive dots
Context: ...ment dependencies must be present, i.e. git submodule update --init --recursive. The default is OFF. * `-DSPINLOCK_LOO...
(DOUBLE_PUNCTUATION)
🔇 Additional comments (17)
examples/CMakeLists.txt (2)
29-31: Consider adding error handling for example execution.The example targets are executed directly without checking their exit status.
- env ./example_art - COMMAND env ./example_art_stats - COMMAND env ./example_olc_art + env bash -c './example_art || exit 1' + COMMAND env bash -c './example_art_stats || exit 1' + COMMAND env bash -c './example_olc_art || exit 1'
12-14: 🧹 Nitpick (assertive)Consider version constraints for dependencies.
While finding the required packages is good, consider adding version requirements to ensure compatibility.
-find_package(Microsoft.GSL REQUIRED) +find_package(Microsoft.GSL 4.0 REQUIRED)Likely invalid or redundant comment.
test/CMakeLists.txt (1)
29-30: LGTM! Good use of namespaced targets.The changes to use
GTest::gtest_mainandGTest::gmock_maininstead of bare targets follow modern CMake best practices and make dependencies more explicit.Also applies to: 36-36, 43-44
.github/workflows/ubuntu-20.04.yml (1)
203-204: LGTM! Clean build directory creation.Creating a fresh build directory before CMake configuration is good practice.
.circleci/config.yml (1)
70-105: LGTM! Well-structured example build setup.The changes properly handle example builds with:
- Clean separation of build directories
- Proper dependency management
- Correct conditional execution based on sanitizer flags
Also applies to: 202-228
README.md (1)
44-47: LGTM! Clear dependency documentation.The reorganization and clarification of dependencies improves understanding of:
- Which dependencies are optional git submodules
- How they can be provided by the parent project
- What each dependency is used for
Also applies to: 51-55
.github/workflows/old-compilers.yml (3)
449-449: LGTM! Optimized submodule handlingDisabling submodule fetching during initial checkout is correct since dependencies will be provided by the parent project.
494-513: LGTM! Well-structured CMake configuration for examplesThe configuration properly:
- Creates a separate build directory
- Sets up compiler variables based on the selected compiler
- Uses correct CMake arguments
- Has appropriate conditions for sanitization flags
514-520: LGTM! Clean examples build configurationThe build step is well-configured with:
- Correct working directory
- Proper parallel build settings (-j3)
- Appropriate conditions for sanitization flags
.github/workflows/build.yml (4)
343-349: LGTM! Comprehensive dependency setup for Linux examplesThe dependency installation is properly guarded with comprehensive conditions for:
- Sanitization flags
- Static analysis
- Coverage builds
359-365: LGTM! Well-structured macOS dependency setupThe macOS-specific dependency installation:
- Uses the correct package (cpp-gsl)
- Maintains consistency with Linux conditions
- Properly uses brew package manager
398-405: LGTM! Consistent examples build configurationThe build configuration maintains consistency with other workflow files and includes proper:
- Working directory setup
- Parallel build settings
- Conditional guards
380-391: 🧹 Nitpick (assertive)Consider using variables for compiler versions
The hardcoded compiler versions could be moved to variables at the top of the file for easier maintenance.
+# Compiler versions +env: + GCC_VERSION: 13 + CLANG_VERSION: 19 - V=13 + V="${GCC_VERSION}" export CC=gcc-$V export CXX=g++-$V elif [[ $COMPILER == "clang" ]]; then - V=19 + V="${CLANG_VERSION}" export CC=clang-$V export CXX=clang++-$VLikely invalid or redundant comment.
CMakeLists.txt (4)
46-46: LGTM! Improved STANDALONE option documentationThe updated description better clarifies the option's purpose and its effects on tests & benchmarks.
190-192: LGTM! Proper coverage configuration guardCorrectly restricts code coverage reporting to standalone builds with a clear error message.
355-356: LGTM! Smart GSL dependency handlingProperly prevents duplicate target definition by checking if GSL is already provided by the parent project.
750-754: LGTM! Streamlined valgrind configurationThe valgrind configuration is properly:
- Conditioned on STANDALONE mode
- Updated to focus on tests and benchmarks
|
|
||
| # An example of the parent setting up the required GSL dependency for UnoDB to | ||
| # use. Exact minimum supported versions will be added as necessary | ||
| find_package(Microsoft.GSL REQUIRED) |
There was a problem hiding this comment.
At some point span is part of c++ and we should adapt the build to that. It leaks through the API since the value_view (and soon the key) will be spans. So it is unfortunate that we do not have a stable type here. A configurable using declaration and include for the build might help with that. Just a call out. I can create an issue for this.
| the libstdc++ debug mode. The default is `OFF`. | ||
| if you work on UnoDB itself. This will build tests and benchmarks, and will | ||
| eanble extra global debug checks that require entire programs to be compiled | ||
| with them. Currently, this consists of the libstdc++ debug mode. The vendored |
There was a problem hiding this comment.
The vendored ... => When ON, the vendored ...
| ``` | ||
|
|
||
| There are some CMake options for users: | ||
| Out-of-source builds are recommended. There are some CMake options for users: |
There was a problem hiding this comment.
I think it is useful to be more explicit here and give an example of how to do the out of source build. Not everyone will read the README in depth. Some (guilty) will skip it looking for the build instructions :-)
| @@ -43,7 +43,7 @@ endif() | |||
| set_bool(is_not_msvc_clang ${is_clang} AND NOT ${is_msvc_clang}) | |||
|
|
|||
| option(STANDALONE | |||
| "Build UnoDB not for linking with outside code. Enables extra global checks") | |||
| "Build UnoDB as a standalone project. Builds tests & benchmarks, enables extra global checks") | |||
There was a problem hiding this comment.
Is IWYU really required for MAINTAINER mode? (a few lines below this). I think it is just recommended. clang-tidy by itself seems to do just fine (or better since not IWYU suggestions are valid). And with gcc you don't have any of it.
Do you require clang for development or is gcc also ok? Maybe call out in the README.md the aspects of development which do critically depend on clang.
Also, call out the requirement for clang-17 for the reformatting of text in the README and how to reflow the text during development. Not having that in place caused a huge amount of pain :-)
There was a problem hiding this comment.
As of today it is required: #629 - so maybe let's shave that yak first
Any compiler is OK for local dev, but CI will test everything. The second and third part of your comment are for CONTRIBUTING.md which I mean to start
|
STANDALONE=ON appears to enable TSan in master. You might look at that in your PR. When I tried running the micro benchmarks with just STANDALONE=ON, it was using TSan. This was also showing up in much slower test run times. |
targets if they are already set up by parent CMake.
example of using UnoDB as a dependency.
Summary by CodeRabbit
Build Configuration
Documentation
Dependency Management
Testing