cmake release dep version overrides#920
Conversation
📝 WalkthroughWalkthroughThis pull request updates multiple CMake configuration files to use conditional version variables for several dependencies. Hardcoded dependency versions have been replaced with variables set to default values when not defined. The changes update calls to functions retrieving dependencies from GitHub, modify target link libraries, adjust header inclusions and type definitions in tests, add release documentation and instructions, and include minor formatting adjustments throughout. Changes
Sequence Diagram(s)sequenceDiagram
participant CMake
participant Func as phare_git_get_or_update
participant FS as FileSystem
participant GitHub
CMake->>Func: Call dependency update (with version variable)
Func->>Func: Check if version variable is set
Func->>FS: Check if dependency directory exists
alt Directory exists
Func->>GitHub: Pull updates from repo
else Directory missing
Func->>Func: Print "cloning ${url} ${branch}"
Func->>GitHub: Clone repository using specified branch/version
end
sequenceDiagram
participant CMake as Top-Level CMakeLists.txt
participant FS as FileSystem
participant Release as release.cmake
CMake->>FS: Check for existence of release.ver.txt
alt File exists
CMake->>Release: Include release.cmake
else
CMake->>CMake: Proceed without release inclusion
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 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: 7
🧹 Outside diff range and nitpick comments (5)
res/cmake/dep/pybind.cmake (1)
12-14: Consider using a specific version tag for reproducible builds.
Using "master" as the default version can lead to inconsistent builds as it points to different commits over time. Consider using a specific version tag as the default.
Apply this diff to set a specific version:
if(NOT DEFINED PHARE_PYBIND11_VERSION)
- SET(PHARE_PYBIND11_VERSION "master")
+ SET(PHARE_PYBIND11_VERSION "v2.13.6") # Or another stable version
endif()res/cmake/dep/samrai.cmake (1)
13-16: Consider adding a brief comment explaining the version override mechanism.
It would be helpful to add a comment explaining that this version can be overridden (e.g., in release.cmake) and what the "develop" version implies.
+# PHARE_SAMRAI_VERSION can be overridden externally (e.g., in release.cmake)
+# Defaults to "develop" for development builds
if(NOT DEFINED PHARE_SAMRAI_VERSION)
SET(PHARE_SAMRAI_VERSION "develop")
endif()src/initializer/data_provider.hpp (1)
Line range hint 57-100: Consider adding serialization format versioning.
For long-term maintainability and backward compatibility, consider adding version information to the serialized format. This will help manage future schema changes and ensure proper migration paths.
Key considerations:
- Add a version field to the serialized format
- Document the serialization format
- Consider providing migration utilities for future format changes
- Add validation for the deserialized data
tests/initializer/phare_dict.cpp (2)
84-85: Consider extracting magic numbers as named constants.
The temperature value 0.12 should be defined as a named constant for better maintainability and clarity.
constexpr double ELECTRON_TEMPERATURE = 0.12;
// ...
dict["electrons"]["pressure_closure"]["Te"] = ELECTRON_TEMPERATURE;96-96: Remove commented code.
The commented line should be removed if it's no longer needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- res/cmake/dep/cppdict.cmake (1 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- res/cmake/dep/pybind.cmake (1 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
- res/cmake/release.cmake (1 hunks)
- src/initializer/data_provider.hpp (3 hunks)
- tests/initializer/phare_dict.cpp (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- res/cmake/release.cmake
🧰 Additional context used
📓 Path-based instructions (1)
src/initializer/data_provider.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (8)
res/cmake/dep/cppdict.cmake (1)
8-9: Verify the repository change and version selection
The change from LaboratoryOfPlasmaPhysics/cppdict to PhilipDeegan/cppdict raises several concerns:
- Moving from an organizational to a personal repository might impact stability and maintenance
- The version change from "master" to "superserial" needs justification
- Long-term maintenance implications should be considered
Please provide rationale for these changes and confirm if this is a temporary solution or permanent change.
Let's verify the repository and version:
✅ Verification successful
Let me gather more information about the version changes and the relationship between these repositories.
Repository change appears to be a collaborative effort with proper maintenance
Based on the verification results, the concerns about the repository change can be addressed:
- The personal fork (PhilipDeegan/cppdict) is actively maintained and the author (Philip Deegan) is actually a contributor to the original repository, as evidenced by his commits in both repositories.
- The "superserial" branch contains specific improvements tested with PHARE, as shown in the commit message "tested with phare dict" and includes serialization enhancements.
- The changes are well-structured, with proper code organization (moving serialization to a separate header) and added test coverage.
- The original repository badges and CI infrastructure are preserved in the fork
- The code quality and maintenance standards appear to be maintained
- The version change to "superserial" adds needed functionality for PHARE
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the personal fork exists and contains the required version
gh repo view PhilipDeegan/cppdict 2>/dev/null || echo "Repository not found"
gh api repos/PhilipDeegan/cppdict/branches/superserial 2>/dev/null || echo "Branch not found"
# Check if there are any other dependencies still using the original repository
rg -l "LaboratoryOfPlasmaPhysics/cppdict" res/cmake/
Length of output: 7873
Script:
#!/bin/bash
# Check the commit history and differences between repositories
gh api repos/LaboratoryOfPlasmaPhysics/cppdict/branches/master 2>/dev/null || echo "Original repo branch not found"
# Check for version references
rg "PHARE_CPPDICT_VERION" -A 1 -B 1
# Check if there are any specific changes in the superserial branch
gh api repos/PhilipDeegan/cppdict/commits/superserial 2>/dev/null || echo "Cannot fetch commit info"
Length of output: 32322
res/cmake/dep/highfive.cmake (1)
12-12: LGTM! Good use of the configurable version variable.
The change properly integrates the new version variable, maintaining functionality while adding flexibility for version management.
res/cmake/dep/samrai.cmake (1)
14-16: LGTM! Good approach to version management.
The introduction of PHARE_SAMRAI_VERSION with a default value provides good flexibility for version control while maintaining a sensible default for development.
src/initializer/data_provider.hpp (2)
14-14: LGTM!
The serializer include is correctly placed and follows the include order convention.
93-99:
Address thread safety and ownership concerns in PHAREDictHandler methods.
Several issues need attention:
- The singleton pattern with static methods needs thread safety guarantees
- The load method's ownership semantics are unclear
- The comment "// ?" suggests implementation uncertainty
Consider this safer implementation:
- void dump(std::string const& filename) const { dump_phare_dict(*phareDict, filename); }
+ void dump(std::string const& filename) const
+ {
+ if (!phareDict) {
+ throw std::runtime_error("Cannot dump: dictionary not initialized");
+ }
+ dump_phare_dict(*phareDict, filename);
+ }
- static auto& load(std::string const& filename)
+ static PHAREDict& load(std::string const& filename)
{
- INSTANCE().phareDict = std::make_unique<PHAREDict>(); // reset
- return (*INSTANCE().phareDict = load_phare_dict(filename)); // ?
+ auto& instance = INSTANCE();
+ std::lock_guard<std::mutex> lock(instance.mutex_); // Add mutex_ as class member
+ instance.phareDict = std::make_unique<PHAREDict>(load_phare_dict(filename));
+ return *instance.phareDict;
}Let's verify if there are any other usages of these methods that might be affected:
tests/initializer/phare_dict.cpp (3)
1-11: LGTM: Headers and namespace usage are appropriate.
The includes and type aliases are well-organized and properly scoped for the test requirements.
42-44: Verify the identical charge configuration.
Both populations are configured with charge -1.0, which seems unusual as alpha particles typically have a different charge than protons.
Please confirm if this is intentional or if it needs adjustment.
Also applies to: 72-74
107-107: Document serialization limitations.
The comment indicates that x_component is not serializable, but this limitation should be properly documented in the relevant API documentation.
PhilipDeegan
force-pushed
the
releases
branch
from
2f32ac1 to
6325940
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/initializer/data_provider.hpp (2)
57-74: Reconsider using inline for I/O operations.
The inline keyword is typically used for small, frequently called functions. For I/O operations that are relatively expensive and less frequently called, using inline might not be beneficial and could potentially increase code size without performance benefits.
95-99: Improve clarity of the load method implementation.
The current implementation combines reset and assignment in a way that's not immediately clear. Consider splitting these operations for better readability:
static auto& load(std::string const& filename)
{
INSTANCE().phareDict = std::make_unique<PHAREDict>(); // reset
- return (*INSTANCE().phareDict = load_phare_dict(filename)); // ?
+ *INSTANCE().phareDict = load_phare_dict(filename);
+ return *INSTANCE().phareDict;
}Also, the comment "// ?" suggests uncertainty about the implementation. Please either remove it or replace it with a meaningful comment explaining the implementation choice.
tests/initializer/phare_dict.cpp (2)
96-96: Remove commented-out code.
Dead code should be removed rather than commented out. If this alternative loading method is important, consider adding it as a separate test case.
107-107: Enhance the "NOT SERIALIZABLE" comment.
The current comment lacks context about why the component is not serializable and what developers should do about it.
Consider expanding the comment:
// InitFunction types are not serializable by design - they represent runtime function objects
// This check verifies that serialization correctly handles this limitation📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- res/cmake/dep/cppdict.cmake (1 hunks)
- res/cmake/dep/highfive.cmake (1 hunks)
- res/cmake/dep/pybind.cmake (1 hunks)
- res/cmake/dep/samrai.cmake (1 hunks)
- res/cmake/release.cmake (1 hunks)
- src/initializer/data_provider.hpp (3 hunks)
- tests/initializer/phare_dict.cpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- res/cmake/dep/cppdict.cmake
- res/cmake/dep/highfive.cmake
- res/cmake/dep/pybind.cmake
- res/cmake/dep/samrai.cmake
- res/cmake/release.cmake
🧰 Additional context used
📓 Path-based instructions (1)
src/initializer/data_provider.hpp (1)
Pattern **/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
src/initializer/data_provider.hpp (2)
14-15: LGTM: Include statement is properly placed.
The serializer include follows proper include ordering and is necessary for the new functionality.
57-74: Previous review comments about documentation and error handling are still applicable.
tests/initializer/phare_dict.cpp (2)
1-11: LGTM! Headers and declarations are well organized.
The includes are appropriate for the test's requirements and the type alias improves code readability.
12-88: Consider extracting magic numbers as named constants.
The function uses magic numbers that could be better expressed as named constants:
100fornbrPartPerCell0.12for electron temperatureTe
This would improve maintainability and make the test's intent clearer.
Example:
constexpr int PARTICLES_PER_CELL = 100;
constexpr double ELECTRON_TEMPERATURE = 0.12;
nicolasaunai
left a comment
nicolasaunai
left a comment
There was a problem hiding this comment.
I suspect this is not finished since I see cppdict points to your repo and the phare dict test is not plugged to cmake/gtest and only tests some of the dict values.
yes this branch is currently hijacked to test serialization in a cppdict fork |
PhilipDeegan
force-pushed
the
releases
branch
from
6325940 to
30ee602
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
RELEASE.md(1 hunks)res/cmake/def.cmake(1 hunks)res/cmake/dep.cmake(1 hunks)res/cmake/dep/cppdict.cmake(1 hunks)res/cmake/dep/highfive.cmake(1 hunks)res/cmake/dep/pybind.cmake(1 hunks)res/cmake/dep/samrai.cmake(1 hunks)res/cmake/release.cmake(1 hunks)src/diagnostic/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- res/cmake/dep/cppdict.cmake
- res/cmake/dep/highfive.cmake
- res/cmake/dep/pybind.cmake
- res/cmake/dep/samrai.cmake
- res/cmake/release.cmake
🧰 Additional context used
🪛 LanguageTool
RELEASE.md
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...cmake/release.cmake - [ ] Update to use latest (if safe) versions - [ ] PR updates to ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (4)
src/diagnostic/CMakeLists.txt (1)
Line range hint 11-19: Verify build configuration across different HighFive settings
The integration of HighFive affects both header inclusion and library linking. Let's verify the build configuration in different scenarios.
Also applies to: 24-24
✅ Verification successful
Let me gather more information to verify the build configuration thoroughly.
Let me check one more aspect to ensure proper conditional compilation.
Build configuration for HighFive appears to be properly isolated
The verification shows that:
- HighFive is only referenced in CMake configuration files
- No unconditional includes of HighFive headers were found
- All HighFive-dependent code is properly contained within the h5* files that are conditionally included
- The type writers and diagnostic writers are template-based, allowing for different backend implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential build configuration issues
# Check if there are any direct includes of HighFive headers outside the conditional block
echo "Checking for unconditional HighFive includes..."
rg -l "include.*HighFive" src/diagnostic/
# Check for any usage of HighFive features in non-conditional code
echo "Checking for HighFive usage in main headers..."
rg -l "HighFive::" src/diagnostic/diagnostic_{manager,writer,props}.hpp
# Look for other potential dependencies on HighFive
echo "Checking for other HighFive dependencies..."
rg -l "HighFive" src/diagnostic/ --type cmake
Length of output: 554
Script:
#!/bin/bash
# Check for conditional includes and usage of HighFive in implementation files
echo "Checking for HighFive includes in implementation files..."
rg -l "#ifdef.*HighFive|#if.*HighFive|#ifndef.*HighFive" src/diagnostic/
# Check the content of h5writer.hpp to verify proper conditional compilation
echo "Checking h5writer.hpp content..."
rg -A 5 "#ifdef.*HighFive|#if.*HighFive|#ifndef.*HighFive" src/diagnostic/detail/h5writer.hpp
# Check if there are any HighFive-related symbols or types used without proper guards
echo "Checking for HighFive-related symbols..."
rg "H5::|HighFive::" src/diagnostic/
# Check for any other CMake files that might reference HighFive
echo "Checking for HighFive in parent CMake files..."
fd CMakeLists.txt -x rg -l "HighFive" {}
Length of output: 1297
Script:
#!/bin/bash
# Check the content of diagnostic headers for proper HighFive conditional compilation
echo "Checking diagnostic headers for proper guards..."
rg -C 5 "class.*Writer|struct.*Writer" src/diagnostic/detail/h5*.hpp
# Check if the HighFive-dependent types are properly guarded
echo "Checking type definitions..."
rg -C 5 "class|struct|typedef" src/diagnostic/detail/types/*.hpp
# Check for any direct dependencies in the implementation files
echo "Checking implementation files..."
fd -e cpp -e hpp . --full-path src/diagnostic/ --exec rg -l "h5writer|h5typewriter|HighFive" {}
Length of output: 8005
res/cmake/dep.cmake (2)
5-5: LGTM! The added message improves build process visibility.
The new message provides clear feedback when cloning repositories, which is consistent with the existing message for updates.
Line range hint 20-39: Verify dependency version management across included CMake files.
Since this file is the central point for dependency management and includes several dependency-specific CMake files, we should verify the version management changes mentioned in the AI summary.
✅ Verification successful
Dependency version management is properly implemented across CMake files
The verification shows that version management is well-structured across the dependency files:
- Each dependency has a configurable version variable (e.g.,
PHARE_PYBIND11_VERSION,PHARE_HIGHFIVE_VERSION,PHARE_SAMRAI_VERSION) - Default versions are set when not explicitly defined
- Consistent use of
phare_github_get_or_updatefunction for fetching dependencies - Proper version checks where needed (e.g.,
PYBIND_MIN_VERSIONfor pybind11) - Fallback mechanisms are in place (e.g., trying to find installed packages before downloading)
The dependency management system appears to be robust and well-maintained, with no issues requiring attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version variables and their usage in dependency CMake files
# List all dependency CMake files
echo "=== Listing dependency CMake files ==="
fd -t f . "res/cmake/dep" -x echo "Found: {}"
# Check for version variables and their usage
echo -e "\n=== Checking version variables ==="
rg -A 3 "VERSION|version" "res/cmake/dep/*.cmake"
# Check for phare_git_get_or_update usage
echo -e "\n=== Checking dependency fetch calls ==="
rg -A 3 "phare_g[i]t[h]*_get_or_update" "res/cmake/dep/*.cmake"
Length of output: 761
Script:
#!/bin/bash
# Let's try with individual file paths instead of glob pattern
echo "=== Checking version variables in each dependency file ==="
for file in res/cmake/dep/pybind.cmake res/cmake/dep/highfive.cmake res/cmake/dep/phlop.cmake res/cmake/dep/caliper.cmake res/cmake/dep/cppdict.cmake res/cmake/dep/samrai.cmake; do
echo -e "\nFile: $file"
echo "----------------------------------------"
rg -A 3 "VERSION|version" "$file" || echo "No version information found"
done
echo -e "\n=== Checking dependency fetch mechanisms ==="
for file in res/cmake/dep/pybind.cmake res/cmake/dep/highfive.cmake res/cmake/dep/phlop.cmake res/cmake/dep/caliper.cmake res/cmake/dep/cppdict.cmake res/cmake/dep/samrai.cmake; do
echo -e "\nFile: $file"
echo "----------------------------------------"
rg -A 3 "git_clone|FetchContent|ExternalProject_Add|phare_git" "$file" || echo "No fetch mechanism found"
done
# Also check the content of these files to understand their structure
echo -e "\n=== Showing content of dependency files ==="
for file in res/cmake/dep/pybind.cmake res/cmake/dep/highfive.cmake res/cmake/dep/phlop.cmake res/cmake/dep/caliper.cmake res/cmake/dep/cppdict.cmake res/cmake/dep/samrai.cmake; do
echo -e "\nFile: $file"
echo "----------------------------------------"
cat "$file"
done
Length of output: 15400
res/cmake/def.cmake (1)
90-96: Conditional inclusion of release configuration is appropriate.
Including release.cmake when CMAKE_BUILD_TYPE is set to "Release" outside of devMode ensures that release-specific configurations are applied correctly.
PhilipDeegan
force-pushed
the
releases
branch
from
30ee602 to
1b0cb75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
RELEASE.md(1 hunks)res/cmake/def.cmake(1 hunks)res/cmake/dep.cmake(1 hunks)res/cmake/dep/cppdict.cmake(1 hunks)res/cmake/dep/highfive.cmake(1 hunks)res/cmake/dep/pybind.cmake(1 hunks)res/cmake/dep/samrai.cmake(1 hunks)res/cmake/release.cmake(1 hunks)src/diagnostic/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- RELEASE.md
- res/cmake/dep.cmake
- res/cmake/dep/cppdict.cmake
- res/cmake/dep/highfive.cmake
- res/cmake/dep/pybind.cmake
- res/cmake/dep/samrai.cmake
- res/cmake/release.cmake
- src/diagnostic/CMakeLists.txt
🔇 Additional comments (1)
res/cmake/def.cmake (1)
93-95: Consider test implications of release version overrides
Given the PR comments about incomplete test integration, particularly for cppdict, ensure that the version overrides in release.cmake don't affect test execution. Consider:
- Adding test cases that verify behavior with release versions
- Documenting any version-specific test requirements
Let's check for version-dependent test configurations:
PhilipDeegan
force-pushed
the
releases
branch
from
1b0cb75 to
97fd182
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/amr/tagging/test_tagging.cpp (1)
Line range hint
208-208: Address TODO comment for tagging testsThe comment "TODOmaybe find a way to test the tagging?" suggests that critical functionality might be untested. The commented-out test implementation below provides a good starting point for implementing these tests.
Would you like me to help implement the tagging tests based on the commented-out code structure? I can help:
- Uncomment and fix the existing test implementation
- Add additional test cases for edge cases
- Create a GitHub issue to track this task
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
RELEASE.md(1 hunks)res/cmake/def.cmake(1 hunks)res/cmake/dep.cmake(1 hunks)res/cmake/dep/cppdict.cmake(1 hunks)res/cmake/dep/highfive.cmake(1 hunks)res/cmake/dep/pybind.cmake(1 hunks)res/cmake/dep/samrai.cmake(1 hunks)res/cmake/release.cmake(1 hunks)src/diagnostic/CMakeLists.txt(1 hunks)tests/amr/tagging/test_tagging.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- RELEASE.md
- res/cmake/dep.cmake
- res/cmake/dep/cppdict.cmake
- res/cmake/dep/highfive.cmake
- res/cmake/dep/pybind.cmake
- res/cmake/dep/samrai.cmake
- res/cmake/release.cmake
- src/diagnostic/CMakeLists.txt
🔇 Additional comments (4)
tests/amr/tagging/test_tagging.cpp (3)
26-30: LGTM! Type alias update and consistent formatting
The type alias change to PHARE::solver::PHARE_Types aligns with the header reorganization. The dictionary initialization uses well-aligned assignments.
172-172: LGTM! Consistent type alias update
The type alias change to PHARE::solver::PHARE_Types maintains consistency with the previous changes.
5-5: Verify header dependencies after include path change
The change from "phare/phare.hpp" to "phare_solver.hpp" suggests a reorganization of the header structure.
✅ Verification successful
Header change is consistent with type system reorganization
The change from "phare/phare.hpp" to "phare_solver.hpp" is part of a consistent reorganization where:
- The new header
phare_solver.hppis used in related solver components - All necessary type definitions are available through the new include path
- The change aligns with the migration from
PHARE::PHARE_TypestoPHARE::solver::PHARE_Typesnamespace - The test file has all required dependencies through the include hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all necessary declarations are available in the new header
# and that it's consistently used across the codebase
# Check if old header still exists and is used elsewhere
rg -l "phare/phare\.hpp"
# Check the new header's usage pattern
rg -l "phare_solver\.hpp"
# Look for any potential include-related compilation flags or definitions
rg -l "PHARE_SOLVER|PHARE_INCLUDE" --type cmake
Length of output: 435
Script:
#!/bin/bash
# Let's analyze the content of both headers and their dependencies
# Check the content of phare_solver.hpp
rg -p "class|struct|namespace|using|typedef" "src/simulator/phare_types.hpp"
# Check the content and dependencies in the old header
rg -p "class|struct|namespace|using|typedef" "src/phare/phare.cpp"
# Check the specific types used in the test file
rg -p "PHARE::solver::PHARE_Types|PHARE::PHARE_Types" "tests/amr/tagging/test_tagging.cpp"
# Look at the include dependencies in the test file
rg -p "#include" "tests/amr/tagging/test_tagging.cpp"
Length of output: 3374
res/cmake/def.cmake (1)
90-96: Verify the integration with release.cmake
The conditional inclusion of release.cmake for Release builds is logically placed in the non-dev mode branch. However, there are a few considerations:
- The change assumes
release.cmakeexists and contains valid CMake code - There's a known typo in the dependency version variable (
PHARE_CPPDICT_VERIONvsPHARE_CPPDICT_VERSION) that needs to be fixed
Let's verify the integration:
#!/bin/bash
# Description: Verify release.cmake integration and variable usage
# Check if release.cmake exists and show its content
if [ -f "res/cmake/release.cmake" ]; then
echo "File exists. Content:"
cat "res/cmake/release.cmake"
# Check for version variables and their usage
echo -e "\nChecking for version variable usage:"
rg "PHARE_CPPDICT_VER[SI]ON" --type cmake
fi
|
this branch is no longer hijacked to test the dict serialization |
|
closes #886 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CMakeLists.txt (1)
27-29: Enhance Release Version Inclusion Block and DocumentationThe conditional block correctly checks for the existence of
release.ver.txtand conditionally includesres/cmake/release.cmakefor dependency version configuration. Consider revising the in-line comment for clarity (for example, "Include release configuration ifrelease.ver.txtis present") to better explain the purpose of this block.RELEASE.md (1)
1-7: Clear Release Checklist with Minor Grammar SuggestionThe release checklist clearly outlines the necessary steps for releasing a new version of PHARE. To improve grammatical precision, consider updating the bullet point on line 4 to "Update to use the latest (if safe) versions" (adding the article "the").
🧰 Tools
🪛 LanguageTool
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...cmake/release.cmake - [ ] Update to use latest (if safe) versions - [ ] PR updates to ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CMakeLists.txt(1 hunks)RELEASE.md(1 hunks)res/cmake/def.cmake(1 hunks)res/cmake/dep.cmake(1 hunks)res/cmake/dep/cppdict.cmake(1 hunks)res/cmake/dep/highfive.cmake(1 hunks)res/cmake/dep/pybind.cmake(1 hunks)res/cmake/dep/samrai.cmake(1 hunks)res/cmake/options.cmake(1 hunks)res/cmake/release.cmake(1 hunks)src/diagnostic/CMakeLists.txt(1 hunks)tests/amr/tagging/test_tagging.cpp(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- res/cmake/options.cmake
- res/cmake/def.cmake
🚧 Files skipped from review as they are similar to previous changes (8)
- src/diagnostic/CMakeLists.txt
- res/cmake/dep.cmake
- res/cmake/dep/samrai.cmake
- res/cmake/dep/highfive.cmake
- tests/amr/tagging/test_tagging.cpp
- res/cmake/dep/cppdict.cmake
- res/cmake/release.cmake
- res/cmake/dep/pybind.cmake
🧰 Additional context used
🪛 LanguageTool
RELEASE.md
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...cmake/release.cmake - [ ] Update to use latest (if safe) versions - [ ] PR updates to ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-13)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
res/cmake/release.cmake (1)
1-17: Well-structured approach to dependency version management.This new file establishes a centralized location for managing dependency versions, which is a best practice for maintaining consistent versioning across the project. The inclusion of URLs to the specific GitHub releases is particularly helpful for documentation and version verification.
A few suggestions for consideration:
- Following CMake conventions, consider using lowercase for commands (
setinstead ofSET).- You might want to add a brief comment explaining how these variables should be used in other CMake files.
- Consider adding a section at the top indicating when these versions were last reviewed/updated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
res/cmake/release.cmake(1 hunks)src/python3/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (python)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (macos-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (1)
src/python3/CMakeLists.txt (1)
31-31: Enhance cpp_etc module dependenciesThe addition of
phare_diagnosticalongsidephare_amrin the linking for targetcpp_etcis a significant update. This change appears to extend the module’s capabilities by incorporating diagnostic functionality. Please verify that:
- The
phare_diagnostictarget is correctly defined and built in the project.- There are no linkage order conflicts, especially if
phare_amrandphare_diagnostichave inter-dependencies.- Any conditional behaviors or additional configuration required by
phare_diagnostic(e.g., dependency on HighFive) are appropriately accounted for in its own CMake configuration.Overall, the update aligns with the PR objective to manage dependency versions more flexibly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/release.yaml (1)
9-11: Repository Checkout and Release Tag File Creation
The steps correctly useactions/checkout@v4and then create arelease.ver.txtfile containing the release tag by echoing${{ github.event.release.tag_name }}. For added robustness—especially if the tag could contain spaces—it is recommended to wrap the variable in quotes. Consider the following diff:- - run: echo ${{ github.event.release.tag_name }} > release.ver.txt + - run: echo "${{ github.event.release.tag_name }}" > release.ver.txtThis is a minor enhancement to ensure consistent behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.yaml(1 hunks)CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: Analyze (cpp)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
🔇 Additional comments (3)
.github/workflows/release.yaml (3)
1-5: Workflow Trigger and Naming Verification
The workflow is clearly named and correctly configured to trigger on the release event with types set to[published]. This ensures that the workflow only executes when a new release is published.
6-8: Job Configuration and Environment Setup
The job is properly defined to run onubuntu-latest. The naming and placement within thejobssection follow best practices for GitHub Actions workflows.
12-17: Release Action Step Configuration
The step usingsoftprops/action-gh-release@v2is well-configured. Theif: startsWith(github.ref, 'refs/tags/')condition ensures that this step runs only when the reference is a tag, aligning perfectly with the workflow's intent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
RELEASE.md (1)
4-4: Improve Wording for Version Update StepThe checklist item "Update to use latest (if safe) versions" can be made slightly clearer by adding the article "the." This will help the sentence read better. Consider applying the following change:
- - [ ] Update to use latest (if safe) versions + - [ ] Update to use the latest (if safe) versions🧰 Tools
🪛 LanguageTool
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...cmake/release.cmake - [ ] Update to use latest (if safe) versions - [ ] PR updates to ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CMakeLists.txt(1 hunks)RELEASE.md(1 hunks)res/cmake/def.cmake(1 hunks)res/cmake/dep.cmake(1 hunks)res/cmake/dep/cppdict.cmake(1 hunks)res/cmake/dep/highfive.cmake(1 hunks)res/cmake/dep/pybind.cmake(1 hunks)res/cmake/dep/samrai.cmake(1 hunks)res/cmake/options.cmake(1 hunks)res/cmake/release.cmake(1 hunks)src/diagnostic/CMakeLists.txt(1 hunks)src/python3/CMakeLists.txt(1 hunks)tests/amr/tagging/test_tagging.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- res/cmake/def.cmake
- res/cmake/options.cmake
- src/diagnostic/CMakeLists.txt
- res/cmake/dep.cmake
- CMakeLists.txt
- res/cmake/dep/samrai.cmake
- res/cmake/dep/highfive.cmake
- res/cmake/dep/pybind.cmake
- src/python3/CMakeLists.txt
- tests/amr/tagging/test_tagging.cpp
- res/cmake/dep/cppdict.cmake
- res/cmake/release.cmake
🧰 Additional context used
🪛 LanguageTool
RELEASE.md
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...cmake/release.cmake - [ ] Update to use latest (if safe) versions - [ ] PR updates to ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (ubuntu-latest, gcc)
- GitHub Check: build (ubuntu-latest, clang)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze (cpp)
🔇 Additional comments (5)
RELEASE.md (5)
1-1: Clear Release Documentation HeadingThe heading "## Releasing a new version of PHARE" clearly introduces the new release process section.
3-3: Review of Release Checklist ItemThe checklist item to review the file
res/cmake/release.cmakeis clear and ensures that the release process is closely tied with the updated CMake configuration.
5-5: Clarify PR Updates StepThe checklist entry "PR updates to master" is concise; however, ensure that its intent (whether to merge updates into master or to update the PR itself) is communicated clearly to the team.
6-6: Branch Cutting Instruction VerificationThe instruction "Cut branch from master as $MAJ.$MIN.x if required" follows the expected release process. Confirm that the version variable format meets your project's versioning standards.
7-7: Tagging Version Consistency CheckThe final checklist item "Tag branch as $MAJ.$MIN.$PATCH" correctly refers to tagging with the proper version format. Make sure that the version variables are updated correctly during the release process.
new highfive github url
new highfive github url
new highfive github url
new highfive github url
new highfive github url
2 participants
Summary by CodeRabbit
New Features
cppdict,HighFive,Pybind11, andSAMRAI.Bug Fixes
Documentation
RELEASE.mdwith a checklist for new version releases.Chores
phare_diagnosticproject to includeHighFive.