build!: Make third-party libraries system dependencies #3949
Conversation
WalkthroughHmm, a comprehensive update to third-party library configurations, this pull request is! Minimum CMake version set to 3.25 across multiple libraries, it has been. The Changes
Suggested labels
Suggested reviewers
Poem
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (12)
🔇 Additional comments (2)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
thirdparty/FRNN/CMakeLists.txt (1)
9-9: Architectural wisdom, share I must!Consistent approach across all dependencies, implemented well it is. But remember these points, you must:
- CMake 3.25 requirement, communicate clearly to all users, you should
- CI/CD pipelines, update they may need
- Documentation of this system dependency approach, beneficial it would be
In harmony with modern CMake practices, these changes are. Well done, young padawan!
thirdparty/detray/CMakeLists.txt (1)
9-9: Consider documenting CMake version requirement, you should.In root CMakeLists.txt, document the reason for CMake 3.25 requirement, we must. Help future maintainers understand this decision, it will.
Add this comment to root CMakeLists.txt:
# CMake 3.25 or higher is required for FetchContent SYSTEM flag # which helps suppress warnings from third-party dependencies cmake_minimum_required(VERSION 3.25)thirdparty/Annoy/CMakeLists.txt (1)
35-35: From INTERFACE to PRIVATE, the compile options have shifted.More precise control over warning suppression, this change brings. Only to Annoy's internal compilation, these options now apply. Wise decision this is, as leak to dependent targets, these options should not.
Consider documenting the rationale for these specific warning suppressions in comments, future Padawans will thank you.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
thirdparty/Annoy/CMakeLists.txt(3 hunks)thirdparty/FRNN/CMakeLists.txt(1 hunks)thirdparty/OpenDataDetector(1 hunks)thirdparty/actsvg/CMakeLists.txt(1 hunks)thirdparty/algebra-plugins/CMakeLists.txt(1 hunks)thirdparty/covfie/CMakeLists.txt(2 hunks)thirdparty/detray/CMakeLists.txt(1 hunks)thirdparty/eigen3/CMakeLists.txt(2 hunks)thirdparty/nlohmann_json/CMakeLists.txt(1 hunks)thirdparty/pybind11/CMakeLists.txt(1 hunks)thirdparty/traccc/CMakeLists.txt(2 hunks)thirdparty/vecmem/CMakeLists.txt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- thirdparty/OpenDataDetector
🔇 Additional comments (16)
thirdparty/pybind11/CMakeLists.txt (1)
1-1: Wise changes these are, approve them I do!
Minimum CMake version and SYSTEM keyword, properly aligned with the force they are. Silence the warnings of third-party dependencies, this change will.
Also applies to: 9-9
thirdparty/nlohmann_json/CMakeLists.txt (1)
1-1: Consistent with the ways of the force, these changes are!
The path of system dependencies, this configuration follows. Harmony with other third-party libraries, it maintains.
Also applies to: 9-9
thirdparty/eigen3/CMakeLists.txt (2)
Line range hint 14-20: Consistent with other ExternalProject_Add instances, this path must be.
Proper setup of the IMPORTED library, I sense. Yet verify other instances of ExternalProject_Add in our archives, we should.
✅ Verification successful
Let me search deeper into the Force of the codebase to understand the external project configurations better.
Hmm, deeper into the CMake files, we must look. The Force guides us to examine the third-party dependencies more closely.
Consistent with the Force, your ExternalProject_Add usage is
Through the Force, I have seen that all third-party dependencies use either FetchContent_Declare with SYSTEM flag or ExternalProject_Add with SYSTEM flag, they do. Your Eigen3 configuration, aligned with the ways of other dependencies, it is. No further changes required, there are.
The path you have chosen:
- SYSTEM flag used consistently across all external dependencies
- Proper alignment with project standards, maintained it is
- Other third-party components, following the same pattern, they are
Balance in the Force of your build system, I sense. Proceed with confidence, you may.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find other ExternalProject_Add instances that might need SYSTEM keyword
# Search for other ExternalProject_Add usage
rg "ExternalProject_Add\(" --type cmake
# Compare with those already using SYSTEM
rg "ExternalProject_Add.*SYSTEM" --type cmake
Length of output: 143
Script:
#!/bin/bash
# Search for any CMake files that might contain external project configurations
fd -e cmake -x cat {} \; -X echo "=== {} ==="
# This will help us see all CMake configurations and better understand the external dependencies
Length of output: 232
Script:
#!/bin/bash
# Search for CMake files first
fd -e cmake
# Then let's examine the content of thirdparty directory specifically
ls -R thirdparty/
# And finally look at the content of any CMake files in thirdparty
find thirdparty -name "*.cmake" -o -name "CMakeLists.txt" -exec cat {} \;
Length of output: 13979
1-1: Verify the ways of the SYSTEM keyword with ExternalProject_Add, we must!
Different from FetchContent's path, this one is. Ensure the same effect on compiler warnings, we should.
Run this script to check ExternalProject_Add documentation and usage:
Also applies to: 11-12
thirdparty/actsvg/CMakeLists.txt (1)
9-9: Hmmmm, wise changes these are!
Correctly aligned with the force, these changes are. The minimum CMake version 3.25, required for SYSTEM keyword it is. Third-party warnings, silence they will.
Verify the CMake version in production environment, we must:
Also applies to: 17-17
✅ Verification successful
The force guides us to search deeper into the repository, hmm? Check other CMake files and CI configurations we must, to ensure harmony in the minimum version requirements.
Harmonious with the Force, this CMake version requirement is!
In perfect balance, all third-party dependencies are, requiring CMake 3.25 they do. Main project and test configurations, lower versions they accept, flexibility they maintain.
- All third-party components in
thirdparty/: CMake 3.25 they require - Main
CMakeLists.txt: Version 3.14 it accepts - Test configurations: Version 3.11 they need
CI workflows, prepared they are, with proper CMake commands. No conflicts in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check minimum CMake version in CI/production
if command -v cmake &> /dev/null; then
cmake --version | head -n 1
fi
Length of output: 18
Script:
#!/bin/bash
# Search for CMake minimum version requirements across the repository
rg -g "CMakeLists.txt" "^cmake_minimum_required" -A 1
# Also check if there are any GitHub workflow files that specify CMake version
fd -g "*.yml" -g "*.yaml" . .github/workflows --exec rg "cmake" -A 2 -B 2
Length of output: 6992
thirdparty/vecmem/CMakeLists.txt (1)
9-9: Strong with the Force, this configuration is!
Like its siblings, this file follows the path of wisdom. System dependency status, VecMem now has. Testing and benchmarking options, untouched they remain, as they should.
Also applies to: 20-20
thirdparty/FRNN/CMakeLists.txt (1)
9-9: Unique path this one follows, yet aligned with our ways it is!
Though custom configuration this dependency requires, the SYSTEM flag and CMake version, properly set they are. But verify the propagation of SYSTEM attribute through custom CMakeLists.txt, we must.
Check the propagation of SYSTEM attribute, we shall:
Also applies to: 16-16
✅ Verification successful
Properly configured, the SYSTEM attribute is, young padawan!
In CMakeLists.txt.in, observe I do that target_include_directories uses SYSTEM flag correctly:
target_include_directories(frnn
SYSTEM PUBLIC
${TORCH_INCLUDE_DIRS}
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/frnn/csrc>
)And in FRNN's CMakeLists.txt, the SYSTEM attribute in FetchContent_Declare, present it is:
FetchContent_Declare(frnncontent SYSTEM ${ACTS_FRNN_SOURCE})Through the Force, see I do that the SYSTEM propagation flows properly from parent to child. No disturbance in the CMake, there is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SYSTEM attribute propagation in generated CMakeLists.txt
# Check if custom CMakeLists.txt.in contains system-related configurations
echo "Examining custom CMakeLists configuration:"
cat CMakeLists.txt.in
# Look for any include_directories or target_include_directories
echo "Checking include directory configurations:"
rg "include_directories|target_include_directories" CMakeLists.txt.in
Length of output: 393
Script:
#!/bin/bash
# Search for CMakeLists.txt in FRNN directory and examine its contents
echo "Examining FRNN CMakeLists.txt:"
cat thirdparty/FRNN/CMakeLists.txt
# Look for include directory configurations in FRNN CMakeLists
echo -e "\nChecking include directory configurations:"
rg "include_directories|target_include_directories" thirdparty/FRNN/CMakeLists.txt
# Check if there are any other CMake files in FRNN directory
echo -e "\nSearching for other CMake files in FRNN:"
fd -e cmake -e txt . thirdparty/FRNN/
# Examine parent CMakeLists.txt that includes FRNN
echo -e "\nExamining parent CMakeLists.txt that includes FRNN:"
rg -A 5 "FRNN" thirdparty/CMakeLists.txt
Length of output: 1655
Script:
#!/bin/bash
# Search for CMakeLists.txt.in in the repository
echo "Searching for CMakeLists.txt.in:"
fd "CMakeLists.txt.in" .
# If found, examine its contents
echo -e "\nExamining CMakeLists.txt.in contents:"
fd "CMakeLists.txt.in" . -x cat {}
# Check parent directory for FRNN configuration
echo -e "\nSearching for FRNN configuration in parent directories:"
fd "CMakeLists.txt" . | rg -v "thirdparty/FRNN" | xargs rg -l "FRNN"
# Examine found files for FRNN configuration
echo -e "\nExamining FRNN configurations:"
fd "CMakeLists.txt" . | rg -v "thirdparty/FRNN" | xargs rg -A 5 "FRNN"
Length of output: 2611
thirdparty/covfie/CMakeLists.txt (2)
9-9: CMake version requirement, consistent it is!
Match the project's needs for system dependency support, this does.
20-20: Simple yet powerful, this change is!
Treat covfie as system dependency, we now do. Clean build output without third-party warnings, this shall bring.
thirdparty/algebra-plugins/CMakeLists.txt (2)
9-9: In harmony with other files, this version requirement is!
Consistency across the project, we maintain.
17-17: The Force is strong with this SYSTEM keyword!
Treat algebra plugins as system dependency, this shall. Many options this file has, yet simple this change remains.
✅ Verification successful
Consistent with the Force, this change is!
Verified, I have, that SYSTEM keyword usage aligns with other third-party dependencies it does. Throughout the repository, a pattern emerges - all FetchContent_Declare commands in thirdparty components, the SYSTEM keyword they embrace. CMake version requirements, consistent they remain at 3.25 across all files. Wisdom in this implementation, there is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify minimum CMake version consistency across all CMakeLists.txt files
echo "Check CMake version requirements across all files:"
rg "cmake_minimum_required\(.*3\.25.*\)" --type cmake
echo -e "\nVerify SYSTEM keyword usage in FetchContent_Declare:"
rg "FetchContent_Declare.*SYSTEM" --type cmake
Length of output: 1813
thirdparty/traccc/CMakeLists.txt (2)
9-9: Wise choice, this version requirement is! Hmmmm.
Required for the SYSTEM flag in FetchContent, CMake 3.25 is. Align with our objectives, this change does.
20-20: Correctly marked as system dependency, traccc now is!
Suppress unwanted warnings from this third-party library, this change will. Verify the effect on build warnings, we must.
✅ Verification successful
Correctly implemented, the SYSTEM flag for traccc is, hmmmm
In good company traccc stands, yes. Among other third-party dependencies with SYSTEM flag, it resides. Confirmed through the Force, I have:
- In
thirdparty/traccc/CMakeLists.txt, properly declared with SYSTEM flag, it is - Like its brethren pybind11, nlohmann_json, vecmem, and others, treated as system dependency, it shall be
- Proper configuration options for system installation, present they are
Trust in the CMake configuration, we can. Suppress unwanted warnings from this third-party library, it will.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that traccc is properly marked as system include
# Look for system include paths in CMake build output
cmake --build build 2>&1 | grep -i "traccc.*system"
Length of output: 48
Script:
#!/bin/bash
# Search for FetchContent_Declare usage with SYSTEM keyword in CMake files
fd -e cmake -e txt --exec grep -l "FetchContent_Declare.*SYSTEM" {}
# Check CMake configuration for traccc
fd "CMakeLists.txt" --exec grep -B2 -A2 "traccc" {}
# Look for any include directives related to traccc
fd "CMakeLists.txt" --exec grep -B2 -A2 "include.*traccc" {}
Length of output: 1904
thirdparty/detray/CMakeLists.txt (1)
9-9: Consistent with our path, this change is.
Match the required CMake version across all third-party dependencies, it does.
thirdparty/Annoy/CMakeLists.txt (2)
9-9: Wise choice, the minimum CMake version requirement is, hmm.
Align with the PR objectives, this change does. Version 3.25 or higher, required it is, for the SYSTEM keyword in FetchContent_Declare to function properly.
23-23: The Force is strong with this SYSTEM keyword addition, yes.
Suppress unwanted warnings from third-party code, this change will. A path to cleaner builds, you have chosen.
Verify the CMake version in the build environment, we must:
✅ Verification successful
Consistent CMake versions across the project, I see not. Hmmmm.
Different minimum CMake versions in our codebase exist:
- Main CMakeLists.txt requires 3.14
- Most third-party components require 3.25
- Downstream test projects use 3.11
- FindFilesystem.cmake needs 3.10
Problematic this inconsistency could be. Yet, SYSTEM keyword since CMake 2.8.12 supported is. Safe to use in all cases, this addition remains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check minimum CMake version in other CMake files
# Ensure consistency across the project, we must
# Search for cmake_minimum_required statements
rg "cmake_minimum_required\s*\(\s*(?:VERSION\s*)?[0-9]" --type cmake
Length of output: 1041
|
stop using |
I would love nothing more 🥲 |
stephenswat
force-pushed
the
build/thirdparty_system
branch
from
b8df5f4 to
9cbf1ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
thirdparty/Annoy/CMakeLists.txt (1)
23-23: Strong with the Force, this change is!Added the SYSTEM keyword has been, suppress unwanted warnings it shall. Removed the need for individual warning suppressions, this approach has. More elegant solution, achieved we have!
A step towards better dependency management, this is. When ready to move beyond FetchContent you are, discuss alternative approaches we can.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
thirdparty/Annoy/CMakeLists.txt(2 hunks)thirdparty/FRNN/CMakeLists.txt(1 hunks)thirdparty/OpenDataDetector(1 hunks)thirdparty/actsvg/CMakeLists.txt(1 hunks)thirdparty/algebra-plugins/CMakeLists.txt(1 hunks)thirdparty/covfie/CMakeLists.txt(2 hunks)thirdparty/detray/CMakeLists.txt(2 hunks)thirdparty/eigen3/CMakeLists.txt(2 hunks)thirdparty/nlohmann_json/CMakeLists.txt(1 hunks)thirdparty/pybind11/CMakeLists.txt(1 hunks)thirdparty/traccc/CMakeLists.txt(2 hunks)thirdparty/vecmem/CMakeLists.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- thirdparty/FRNN/CMakeLists.txt
- thirdparty/OpenDataDetector
- thirdparty/nlohmann_json/CMakeLists.txt
- thirdparty/pybind11/CMakeLists.txt
- thirdparty/vecmem/CMakeLists.txt
- thirdparty/traccc/CMakeLists.txt
- thirdparty/actsvg/CMakeLists.txt
- thirdparty/algebra-plugins/CMakeLists.txt
- thirdparty/covfie/CMakeLists.txt
- thirdparty/detray/CMakeLists.txt
- thirdparty/eigen3/CMakeLists.txt
stephenswat
force-pushed
the
build/thirdparty_system
branch
from
9cbf1ae to
c92013c
Compare
|
stephenswat
force-pushed
the
build/thirdparty_system
branch
3 times, most recently
from
2969db2 to
0040d7f
Compare
github-actions
bot
added
the
Infrastructure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
thirdparty/eigen3/CMakeLists.txt (1)
1-11: Approve the path to enlightenment, I do!Wise changes these are! From ExternalProject to FetchContent we move, with SYSTEM flag our ally becomes. Warnings, suppress they will, as intended in the ancient texts of CMake 3.25.
Remember you must, that this path leads to:
- Cleaner build output, with warnings properly managed
- Simplified dependency handling through FetchContent
- Better integration with modern CMake practices
.github/workflows/builds.yml (1)
290-292: Path to system dependencies, carefully consider we must!System dependencies through CMake SYSTEM flag, a powerful tool it is. But remember you must:
- Warning suppression, affect only third-party code it should
- Build reproducibility, maintain we must
- Version conflicts with system packages, watch out for them, you should
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
.github/workflows/builds.yml(2 hunks)thirdparty/Annoy/CMakeLists.txt(2 hunks)thirdparty/FRNN/CMakeLists.txt(1 hunks)thirdparty/actsvg/CMakeLists.txt(1 hunks)thirdparty/algebra-plugins/CMakeLists.txt(1 hunks)thirdparty/covfie/CMakeLists.txt(2 hunks)thirdparty/detray/CMakeLists.txt(2 hunks)thirdparty/eigen3/CMakeLists.txt(1 hunks)thirdparty/nlohmann_json/CMakeLists.txt(1 hunks)thirdparty/pybind11/CMakeLists.txt(1 hunks)thirdparty/traccc/CMakeLists.txt(2 hunks)thirdparty/vecmem/CMakeLists.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- thirdparty/FRNN/CMakeLists.txt
- thirdparty/nlohmann_json/CMakeLists.txt
- thirdparty/traccc/CMakeLists.txt
- thirdparty/vecmem/CMakeLists.txt
- thirdparty/algebra-plugins/CMakeLists.txt
- thirdparty/pybind11/CMakeLists.txt
- thirdparty/Annoy/CMakeLists.txt
- thirdparty/actsvg/CMakeLists.txt
- thirdparty/covfie/CMakeLists.txt
- thirdparty/detray/CMakeLists.txt
🔇 Additional comments (3)
thirdparty/eigen3/CMakeLists.txt (1)
10-10: Verify the source of Eigen, we must!
The variable ACTS_EIGEN3_SOURCE, defined elsewhere it may be. Ensure its existence, we should.
✅ Verification successful
Properly configured, the Eigen source is! Hmmmm.
Found the source we have, in cmake/ActsExternSources.cmake it resides. A specific version from GitLab, with SHA256 verification, it fetches. Secure and well-defined, the configuration is.
- URL: https://gitlab.com/libeigen/eigen
- SHA256 checksum: present it is
- Cache variable: properly marked as advanced, it has been
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ACTS_EIGEN3_SOURCE definition
echo "Searching for ACTS_EIGEN3_SOURCE definition in CMake files..."
rg -t cmake "ACTS_EIGEN3_SOURCE"
# Check if Eigen source is properly configured
echo "Checking Eigen configuration..."
fd -e cmake -e txt CMakeLists --exec grep -l "Eigen"
Length of output: 928
Script:
#!/bin/bash
# Inspect the definition of ACTS_EIGEN3_SOURCE in ActsExternSources.cmake
echo "The source of ACTS_EIGEN3_SOURCE, we shall find..."
cat cmake/ActsExternSources.cmake | grep -A 10 "ACTS_EIGEN3_SOURCE"
# Check if there's a default Eigen URL or source configuration
echo "Default configuration, we shall examine..."
rg "URL.*eigen" -A 2
Length of output: 1338
.github/workflows/builds.yml (2)
251-254: Wise choice, the matrix configuration is!
Balance to the Force, this brings. Both Clang and GCC, tested they will be, hmm. Standard C++20, consistent across all configurations, it remains.
290-292: Verify the path of the Force, we must!
Compiler selection from matrix variables, dynamic it is. But verify we must, that chosen compilers with system dependencies, compatible they are.
Run this command to check compiler compatibility:
✅ Verification successful
The previous attempt failed as compilers weren't available in the sandbox. Let us search the codebase for compiler configurations and requirements, we shall! Hmmmm.
Hmmmm, matrix configurations and compiler requirements, search for we must. Help us understand the compatibility requirements better, this will.
Safe and sound, the compiler configuration is!
Verified the matrix configuration has been, hmmmm. Shows it does that:
- C++20 standard used it is
- Both Clang and GCC supported they are
- Ubuntu 22.04 as base image, modern and stable it is
No compatibility concerns found have I. The Force is strong with this configuration, as modern compilers and standard it uses. System dependencies, compatible they will be.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compiler compatibility with system dependencies
# Check compiler versions and capabilities
echo "Checking compiler versions and capabilities..."
for CXX in clang++ g++; do
if command -v $CXX &> /dev/null; then
echo "=== $CXX version ==="
$CXX --version
echo "=== $CXX C++20 support ==="
echo "#include <concepts>" | $CXX -x c++ -std=c++20 -c -
fi
done
Length of output: 190
Script:
#!/bin/bash
# Search for compiler-related configurations in workflow files
echo "=== Workflow configurations ==="
rg "matrix.(cxx|std)" .github/workflows/ -A 2 -B 2
# Search for compiler requirements in build files
echo -e "\n=== CMake configurations ==="
fd "CMakeLists.txt" --exec cat {}
# Search for any explicit compiler version requirements
echo -e "\n=== Version requirements ==="
rg "CMAKE_(CXX|C)_(STANDARD|COMPILER)" -A 1 -B 1
Length of output: 67645
Script:
#!/bin/bash
# Search for matrix configurations in workflow files
echo "=== Matrix configurations ==="
rg "matrix:" .github/workflows/ -A 10
# Search for compiler requirements in CMake files
echo -e "\n=== CMake compiler requirements ==="
rg "CMAKE_MINIMUM_REQUIRED|CMAKE_CXX_STANDARD" CMakeLists.txt
Length of output: 872
stephenswat
force-pushed
the
build/thirdparty_system
branch
from
0040d7f to
05bb9c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/builds.yml (2)
251-251: Expand the matrix configuration, young padawan must.Hmmmm. Limited the matrix is, with only g++ it contains. Stronger in the Force, it would become, with these additions:
strategy: matrix: include: - image: ubuntu2204 std: 20 cxx: g++ - image: ubuntu2204 std: 20 cxx: clang++
Line range hint
1-24: Document the ways of the Force, we should.Missing, the workflow documentation is. Add comments to explain the purpose of each job and their dependencies, you must. Help future Jedi understand the build process, it will.
Add these comments at the start of the workflow:
# GitHub Actions workflow for building and testing ACTS # # Jobs: # - linux_ubuntu: Main build job for Ubuntu # - linux_ubuntu_extra: Additional builds with different compiler configurations # - linux_examples_test: Runs Python-level tests # - linux_physmon: Performs physics performance monitoring # - macos: Builds and tests on macOS
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
.github/workflows/builds.yml(2 hunks)thirdparty/Annoy/CMakeLists.txt(2 hunks)thirdparty/FRNN/CMakeLists.txt(1 hunks)thirdparty/actsvg/CMakeLists.txt(1 hunks)thirdparty/algebra-plugins/CMakeLists.txt(1 hunks)thirdparty/covfie/CMakeLists.txt(2 hunks)thirdparty/detray/CMakeLists.txt(2 hunks)thirdparty/eigen3/CMakeLists.txt(1 hunks)thirdparty/nlohmann_json/CMakeLists.txt(1 hunks)thirdparty/pybind11/CMakeLists.txt(1 hunks)thirdparty/traccc/CMakeLists.txt(2 hunks)thirdparty/vecmem/CMakeLists.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- thirdparty/Annoy/CMakeLists.txt
- thirdparty/actsvg/CMakeLists.txt
- thirdparty/traccc/CMakeLists.txt
- thirdparty/pybind11/CMakeLists.txt
- thirdparty/nlohmann_json/CMakeLists.txt
- thirdparty/FRNN/CMakeLists.txt
- thirdparty/algebra-plugins/CMakeLists.txt
- thirdparty/detray/CMakeLists.txt
- thirdparty/covfie/CMakeLists.txt
- thirdparty/eigen3/CMakeLists.txt
- thirdparty/vecmem/CMakeLists.txt
🔇 Additional comments (1)
.github/workflows/builds.yml (1)
287-289: Approve these changes, I do. Yet verify we must.
Wise these changes are, aligning with the Force they do. But verify the compiler settings across all builds, we should.
✅ Verification successful
Let us seek deeper understanding of the build matrix and its compiler configurations, hmm.
Consistent and proper, these compiler settings are
Through the Force, seen I have that:
- Matrix configuration properly defines compiler (
g++) and C++ standard (20) - Consistently applied these settings are across the workflow
- All third-party dependencies share compatible CMake minimum version (3.25)
- No conflicting compiler configurations exist in the workflow
Strong with the Force these changes are. No disturbance do I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify compiler settings consistency across jobs
# Search for compiler and standard settings in workflow
rg "CMAKE_CXX_COMPILER|CMAKE_CXX_STANDARD" .github/workflows/builds.yml
# Check CMake minimum version requirements
rg "cmake_minimum_required" -g "CMakeLists.txt"
Length of output: 1475
Script:
#!/bin/bash
# Check matrix configuration and job definitions
rg "matrix:" -A 10 .github/workflows/builds.yml
# Look for other compiler-related configurations
rg "cxx:|compiler:" .github/workflows/builds.yml
# Check if there are any conditional CMake configurations
rg "if.*cmake" .github/workflows/builds.yml
Length of output: 440
stephenswat
force-pushed
the
build/thirdparty_system
branch
from
05bb9c2 to
9ed4d32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.gitlab-ci.yml (1)
286-286: Improve matrix configuration, young padawan must.A more maintainable approach, suggest I do. Instead of separate jobs, use GitLab's matrix syntax to define compiler configurations, I would. Less duplication and easier maintenance, this brings.
Like this, the matrix could be:
.linux_ubuntu_extra: parallel: matrix: - COMPILER: [g++, clang++] CXXSTD: [20] variables: CXX: $COMPILERAlso applies to: 314-314
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
.github/workflows/builds.yml(2 hunks).gitlab-ci.yml(3 hunks)thirdparty/Annoy/CMakeLists.txt(2 hunks)thirdparty/FRNN/CMakeLists.txt(1 hunks)thirdparty/actsvg/CMakeLists.txt(1 hunks)thirdparty/algebra-plugins/CMakeLists.txt(1 hunks)thirdparty/covfie/CMakeLists.txt(2 hunks)thirdparty/detray/CMakeLists.txt(2 hunks)thirdparty/eigen3/CMakeLists.txt(1 hunks)thirdparty/nlohmann_json/CMakeLists.txt(1 hunks)thirdparty/pybind11/CMakeLists.txt(1 hunks)thirdparty/traccc/CMakeLists.txt(2 hunks)thirdparty/vecmem/CMakeLists.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- thirdparty/nlohmann_json/CMakeLists.txt
- thirdparty/vecmem/CMakeLists.txt
- thirdparty/pybind11/CMakeLists.txt
- thirdparty/traccc/CMakeLists.txt
- thirdparty/FRNN/CMakeLists.txt
- thirdparty/actsvg/CMakeLists.txt
- thirdparty/covfie/CMakeLists.txt
- thirdparty/eigen3/CMakeLists.txt
- thirdparty/algebra-plugins/CMakeLists.txt
- thirdparty/Annoy/CMakeLists.txt
- thirdparty/detray/CMakeLists.txt
🔇 Additional comments (2)
.gitlab-ci.yml (1)
Line range hint 364-364: Wise choice, this logging threshold is!
Control log verbosity, it does. Approve this change, I do.
.github/workflows/builds.yml (1)
251-254: Back the clang build is, hmmmm!
Address the past review comment, this change does. Both g++ and clang++ builds, now we have.
github-actions
bot
added
Component - Core
stephenswat
force-pushed
the
build/thirdparty_system
branch
3 times, most recently
from
4c5364f to
54419e4
Compare
This commit adds several new concepts for dealing with Eigen base types. The motivation for this is superficially to simplify some of the requirements in the track state proxy and the multi-trajectory, but it also allows me to more easily deal with some clang compiler warnings seen in acts-project#3949, as clang won't let me compare `A::ColsAtCompileTime` and `B::ColsAtCompileTime` if `A` and `B` are different types; this allows me to hide the conversion to integers inside a concept.
stephenswat
force-pushed
the
build/thirdparty_system
branch
from
440ff35 to
f819d45
Compare
github-actions
bot
removed
Component - Core
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp (1)
622-626: Approve the modernization of template constraints, I do!Replaced static assertions with concepts, you have. Clearer error messages and better compile-time checks, this brings. A wise choice for modern C++, it is.
Consider documenting the requirements in a comment block above the method, help future maintainers it will:
/// @brief Allocate calibrated measurement /// @tparam val_t Value type with fixed size at compile time /// @tparam cov_t Covariance type matching val_t rows and square matrix /// @requires val_t has fixed size and rows <= eBoundSize /// @requires val_t and cov_t have same number of rows /// @requires cov_t is a square matrix
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Podio/include/Acts/Plugins/Podio/PodioTrackStateContainer.hpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
stephenswat
force-pushed
the
build/thirdparty_system
branch
from
d2d4df4 to
dbee1bb
Compare
|
This is now ready for review. |
ACTS currently supports downloading its dependencies via FetchContent which has the problem that they are - by default - considered as files belonging to the project itself. This is undesirable, as this may introduce warnings into the build of ACTS that we cannot do anything about. In CMake 3.25, FetchContent gained the `SYSTEM` flag which allows dependencies fetched through it to be marked as system libraries, for which no warnings are issued. This commit turns all out third-party dependencies into system dependencies.
|
Is this a breaking change? |
Hmm good point, arguably the rise in the CMake version is breaking. Yes I will mark it as such. |
stephenswat
changed the title
|
ACTS currently supports downloading its dependencies via FetchContent which has the problem that they are - by default - considered as files belonging to the project itself. This is undesirable, as this may introduce warnings into the build of ACTS that we cannot do anything about. In CMake 3.25, FetchContent gained the
SYSTEMflag which allows dependencies fetched through it to be marked as system libraries, for which no warnings are issued.This commit turns all out third-party dependencies into system dependencies.
Summary by CodeRabbit
Build Configuration
SYSTEMkeyword toFetchContent_Declarefor various dependenciesCI/CD Improvements