simpler dep handling + versions#887
Conversation
WalkthroughWalkthroughThe changes introduce two new functions, Changes
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
|
PhilipDeegan
force-pushed
the
releases
branch
from
eb910bb to
1261097
Compare
![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
Outside diff range, codebase verification and nitpick comments (2)
res/cmake/dep/pybind.cmake (1)
14-14: Consider using a specific version or tag instead ofmaster.Using a specific version or tag for
PYBIND11_VERSIONis generally recommended for better reproducibility and stability. Themasterbranch may contain unstable or breaking changes.For example, you can use the latest stable version:
set(PYBIND11_VERSION v2.10.4)Or, you can use a specific commit hash:
set(PYBIND11_VERSION 52d2c5e0b0b5b8e1197a08c9201b79ac1a21e75a)res/cmake/dep/highfive.cmake (1)
7-7: Consider using a specific version or tag instead of "master".Using a specific version or tag for the
HIGHFIVE_VERSIONvariable is generally recommended for better reproducibility and stability.set(HIGHFIVE_VERSION v2.3.1) # Replace with the desired version or tag
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- res/cmake/dep.cmake (1 hunks)
- res/cmake/dep/caliper.cmake (1 hunks)
- res/cmake/dep/cppdict.cmake (1 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- res/cmake/dep/phlop.cmake (1 hunks)
- res/cmake/dep/pybind.cmake (1 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
Additional comments not posted (10)
res/cmake/dep/cppdict.cmake (2)
4-4: Verify the usage of themasterbranch for theCPPDICT_VERSION.Using a branch name like
masterinstead of a specific version tag might lead to unexpected changes in the future. It's generally recommended to use specific version tags for stable builds.If you intend to use a stable version of
cppdict, consider using a specific version tag instead of themasterbranch. You can find the available version tags in the cppdict releases.
5-5: LGTM!The usage of the
phare_github_get_or_updatefunction simplifies the dependency management forcppdict. The function call looks correct and matches the expected usage based on the function name and arguments.res/cmake/dep/phlop.cmake (1)
5-5: LGTM! The change simplifies the dependency management.The change replaces the previous logic for managing the Phlop dependency with a single call to the
phare_github_get_or_updatefunction, which abstracts the cloning and updating process into one command. This simplifies the control flow and improves code reusability and maintainability.Verify that the
phare_github_get_or_updatefunction is implemented correctly and handles all the necessary cases, such as:
- Cloning the repository if it doesn't exist.
- Updating the repository to the latest version if it already exists.
- Handling any errors that may occur during the cloning or updating process.
You can use the following script to search for the function definition and analyze its implementation:
res/cmake/dep/pybind.cmake (1)
16-16: LGTM!The changes simplify the process of managing the pybind11 subproject by using the
phare_github_get_or_updatefunction. The function call provides the necessary arguments and is in line with the PR objectives.res/cmake/dep/caliper.cmake (1)
21-21: LGTM!The code change simplifies the management of the Caliper dependency by replacing the conditional logic with a single call to the
phare_github_get_or_updatefunction. This improves maintainability, readability, and aligns with the PR objective of simplifying dependency handling.res/cmake/dep.cmake (2)
3-14: LGTM!The code changes are approved. The function is correctly implemented and handles the cloning and updating of a Git repository based on the provided arguments and the
devModevariable. It uses the recommendedexecute_processcommand to run Git commands and themessagecommand to print a helpful message when downloading the latest updates.
16-18: LGTM!The code changes are approved. The function is correctly implemented and simplifies the process of updating a GitHub repository by constructing the URL based on the provided repository name and branch. It calls
phare_git_get_or_updatewith the constructed URL, which is a good way to reuse the functionality ofphare_git_get_or_updateand avoid code duplication.res/cmake/dep/highfive.cmake (1)
9-9: LGTM!The
phare_github_get_or_updatefunction call simplifies the management of the HighFive dependency and improves the maintainability and readability of the CMake configuration.res/cmake/dep/samrai.cmake (2)
13-13: LGTM!Setting a default version for SAMRAI is a good practice for maintainability.
17-17: LGTM!The new function call to
phare_github_get_or_updatesimplifies the process of cloning and updating the SAMRAI repository, which enhances maintainability and reduces complexity. The function arguments are correctly passed, including the repository owner and name, and the version.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
res/cmake/dep/cppdict.cmake (1)
4-4: Consider using a specific version tag instead of themasterbranch.Using a branch name like
mastermight lead to unexpected changes in the future. For stable builds, it's generally recommended to use specific version tags.set(CPPDICT_VERSION v1.0.0) # Replace with the desired version tag
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- res/cmake/dep.cmake (1 hunks)
- res/cmake/dep/caliper.cmake (1 hunks)
- res/cmake/dep/cppdict.cmake (1 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- res/cmake/dep/phlop.cmake (1 hunks)
- res/cmake/dep/pybind.cmake (1 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
- src/diagnostic/detail/h5writer.hpp (5 hunks)
- src/hdf5/detail/h5/h5_file.hpp (3 hunks)
- tests/diagnostic/test_diagnostics.hpp (3 hunks)
- tests/simulator/test_diagnostics.py (1 hunks)
Files skipped from review due to trivial changes (1)
- res/cmake/dep/pybind.cmake
Files skipped from review as they are similar to previous changes (4)
- res/cmake/dep.cmake
- res/cmake/dep/caliper.cmake
- res/cmake/dep/phlop.cmake
- res/cmake/dep/samrai.cmake
Additional context used
Path-based instructions (3)
src/hdf5/detail/h5/h5_file.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.tests/diagnostic/test_diagnostics.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/diagnostic/detail/h5writer.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (12)
res/cmake/dep/cppdict.cmake (1)
5-5: LGTM!The code changes are approved. The use of the
phare_github_get_or_updatefunction streamlines the dependency management and improves the maintainability of the code.res/cmake/dep/highfive.cmake (1)
9-9: LGTM!The code changes are approved. The
phare_github_get_or_updatefunction simplifies the process of managing the HighFive library.tests/simulator/test_diagnostics.py (1)
209-210: LGTM!The updated assertion for validating the
highfive_versionattribute is more flexible and allows for potential pre-release identifiers in the patch version while still ensuring that the major and minor versions are correctly formatted as digits.src/hdf5/detail/h5/h5_file.hpp (2)
16-16: LGTM!The new type alias
FileOpimproves code readability and type safety by providing a more descriptive name for theHighFive::File::AccessModetype.
46-46: LGTM!The changes to use the
FileOptype alias for theflagsparameter in thecreateHighFiveFilemethod and the constructor of theHighFiveFileclass improve type safety and clarity. The reformatting of thefile()method is also fine.Also applies to: 64-64, 72-72
tests/diagnostic/test_diagnostics.hpp (2)
19-19: LGTM!The change simplifies the file access logic by using a single access mode for overwriting files. This aligns with the PR objective of simplifying dependency handling.
50-50: Enhance type safety and clarity.The changes to the constructor parameter and member variable types enhance type safety and clarity:
- Using
auto constfor theflagsparameter allows the compiler to deduce the type while ensuring that the value cannot be modified.- Updating the
flags_member variable to be of typeHiFile::AccessMode constensures consistency with the constructor parameter.These changes adhere to the principles of clean code and expressiveness.
Also applies to: 94-95
src/diagnostic/detail/h5writer.hpp (5)
53-53: Improved type safety and expressiveness.The change from using an unsigned constant to the
HiFile::AccessModeenum improves type safety and code expressiveness.
59-59: Improved type safety in the constructor.Changing the
flagsparameter type toHiFile::AccessModein the constructor enhances type safety and maintains consistency with the rest of the class.
71-72: Improved type safety in themake_uniquemethod.Using
HiFile::AccessModefor theflagsvariable in themake_uniquemethod enhances type safety and maintains consistency with the rest of the class.
97-97: Improved type safety in themakeFilemethod andflagsmember variable.Changing the
file_flagparameter type toHiFile::AccessModein themakeFilemethod and theflagsmember variable type toHiFile::AccessModeenhances type safety and maintains consistency with the rest of the class.Also applies to: 171-171
181-181: Improved type safety in thefile_flagsmember variable.Changing the mapped type of the
file_flagsmember variable fromunsignedtoHiFile::AccessModeenhances type safety and maintains consistency with the rest of the class.
| if(HighFive) | ||
|
|
||
| set (HIGHFIVE_SRC ${CMAKE_CURRENT_SOURCE_DIR}/subprojects/highfive) | ||
| set (HIGHFIVE_VERSION master) |
There was a problem hiding this comment.
Use a specific version or tag instead of "master".
Setting the version to "master" may introduce breaking changes. For stability and reproducibility, it is recommended to use a specific version or tag.
Consider using a specific version or tag, for example:
set(HIGHFIVE_VERSION v2.3.1)There was a problem hiding this comment.
if you look at the diff, the version was always set to master, this mechanism is to allow us to set particular versions for releasing this software, which should happen soon
There was a problem hiding this comment.
@PhilipDeegan, understood! Thanks for the clarification. It's clear that the use of "master" is intentional for now, with plans to set specific versions during the release process. This approach makes sense during the development phase.
PhilipDeegan
force-pushed
the
releases
branch
from
1261097 to
ba78811
Compare
PhilipDeegan
force-pushed
the
releases
branch
from
ba78811 to
77a3335
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
res/cmake/dep/samrai.cmake (1)
17-17: Simplification throughphare_github_get_or_updateis beneficial, but ensure robust error handling.The introduction of
phare_github_get_or_updateto manage the SAMRAI repository simplifies the CMake configuration significantly. However, it's crucial to ensure that this function includes robust error handling to manage potential issues during the fetch or update processes.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- res/cmake/dep.cmake (1 hunks)
- res/cmake/dep/caliper.cmake (1 hunks)
- res/cmake/dep/cppdict.cmake (1 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- res/cmake/dep/phlop.cmake (1 hunks)
- res/cmake/dep/pybind.cmake (1 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- res/cmake/dep.cmake
- res/cmake/dep/caliper.cmake
- res/cmake/dep/cppdict.cmake
- res/cmake/dep/phlop.cmake
- res/cmake/dep/pybind.cmake
Additional comments not posted (2)
res/cmake/dep/highfive.cmake (1)
9-9: Streamlined dependency management withphare_github_get_or_update.Using
phare_github_get_or_updateto handle the HighFive library simplifies the dependency management process significantly. This change enhances maintainability and reduces the complexity of the script.res/cmake/dep/samrai.cmake (1)
13-15: Consider using a stable SAMRAI version for production builds.Setting the SAMRAI version to "develop" ensures that the latest development state is used, which might not always be stable. Consider using a stable release version for production builds to avoid potential issues with unstable features.
| if(HighFive) | ||
|
|
||
| set (HIGHFIVE_SRC ${CMAKE_CURRENT_SOURCE_DIR}/subprojects/highfive) | ||
| set (HIGHFIVE_VERSION master) |
There was a problem hiding this comment.
Acknowledgment of version setting to "master".
The setting of HIGHFIVE_VERSION to "master" is noted. While this is suitable for development, it's important to switch to a stable release version in production to avoid potential breaking changes.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Enhancements