-
Notifications
You must be signed in to change notification settings - Fork 34
Add comprehensive GitHub Copilot instructions with header organization and conventional commits requirements for EICrecon development #2028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Copilot
wants to merge
8
commits into
main
Choose a base branch
from
copilot/fix-2027
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f0fc76b
Initial plan
Copilot 5d9033c
Create comprehensive .github/copilot-instructions.md with validated c…
Copilot 0e42868
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 77138f5
Update copilot-instructions with corrections and additions
wdconinc 9ca579b
Fix incomplete text in GitHub Actions snippet description
Copilot 88897cc
Add header alphabetization requirements for IWYU compliance
Copilot a39768c
Add explicit instructions for header include syntax (quotes vs angle …
Copilot fdf02d0
Add conventional commits requirements for breaking change identification
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,235 @@ | ||
| # EICrecon Development Instructions | ||
|
|
||
| EICrecon is a JANA-based reconstruction software for the ePIC detector. This is a complex high-energy physics reconstruction software requiring specialized dependencies and extensive build times. | ||
|
|
||
| **ALWAYS follow these instructions first and fallback to additional search and context gathering only if the information in these instructions is incomplete or found to be in error.** | ||
|
|
||
| ## Working Effectively with EICrecon | ||
|
|
||
| ### Essential Setup: Use eic-shell Environment | ||
|
|
||
| **CRITICAL: This is the ONLY recommended approach for development. Manual dependency installation is complex and strongly discouraged.** | ||
|
|
||
| **Bootstrap the eic-shell environment:** | ||
| ```bash | ||
| mkdir ~/eic | ||
| cd ~/eic | ||
| curl --location https://get.epic-eic.org | bash | ||
| ./eic-shell | ||
| ``` | ||
|
|
||
| **Alternative if /cvmfs is available:** | ||
| ```bash | ||
| # Note: On JLab ifarm, run 'module load singularity/3.9.5' first | ||
| singularity exec /cvmfs/singularity.opensciencegrid.org/eicweb/eic_xl:nightly eic-shell | ||
| ``` | ||
|
|
||
wdconinc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| **Setup geometry and clone EICrecon:** | ||
| ```bash | ||
| source /opt/detector/epic-main/bin/thisepic.sh | ||
| git clone https://github.com/eic/EICrecon | ||
| cd EICrecon | ||
| ``` | ||
|
|
||
| ### Build Process | ||
|
|
||
| **Configure and build (NEVER CANCEL - Build takes 30-60 minutes):** | ||
| ```bash | ||
| cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install | ||
| cmake --build build --target install -- -j8 | ||
| ``` | ||
|
|
||
| **CRITICAL TIMING WARNING: Set timeout to 90+ minutes. Build can take 30-60 minutes even with ccache. NEVER CANCEL long-running builds.** | ||
|
|
||
| **Alternative debug build:** | ||
| ```bash | ||
| cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install -DCMAKE_BUILD_TYPE=Debug | ||
| cmake --build build --target install -- -j8 | ||
| ``` | ||
|
|
||
| **Setup environment to use the build:** | ||
| ```bash | ||
| source install/bin/eicrecon-this.sh | ||
| ``` | ||
|
|
||
| ### Testing | ||
|
|
||
| **Run unit tests (NEVER CANCEL - Tests take 15-30 minutes):** | ||
| ```bash | ||
| export LD_LIBRARY_PATH=$PWD/install/lib:$LD_LIBRARY_PATH | ||
| export JANA_PLUGIN_PATH=$PWD/install/lib/EICrecon/plugins${JANA_PLUGIN_PATH:+:${JANA_PLUGIN_PATH}} | ||
| ctest --test-dir build -V | ||
| ``` | ||
|
|
||
| **Test basic functionality:** | ||
| ```bash | ||
| eicrecon --help | ||
| ``` | ||
|
|
||
| Expected output shows usage information with options like `-h`, `-v`, `-c`, `-Pkey=value`, etc. | ||
|
|
||
| **Test plugin loading (basic validation):** | ||
| ```bash | ||
| jana -PPLUGINS=podio,dd4hep | ||
wdconinc marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ``` | ||
|
|
||
| ### Manual Validation Scenarios | ||
|
|
||
| **ALWAYS perform these validation steps after making changes:** | ||
|
|
||
| 1. **Basic executable test:** | ||
| ```bash | ||
| eicrecon --version | ||
| eicrecon --configs | ||
| ``` | ||
|
|
||
| 2. **Plugin validation:** | ||
| ```bash | ||
| eicrecon -L # List factories | ||
| ``` | ||
|
|
||
| 3. **Environment validation:** | ||
| ```bash | ||
| echo $JANA_PLUGIN_PATH | ||
| echo $LD_LIBRARY_PATH | ||
| ldd install/lib/EICrecon/plugins/*.so | grep -v "not found" || echo "Missing dependencies detected" | ||
| ``` | ||
|
|
||
| ### Build Configurations and Sanitizers | ||
|
|
||
| **Available build options (use in cmake configure step):** | ||
| - `-DCMAKE_BUILD_TYPE=Release` (default, fastest) | ||
| - `-DCMAKE_BUILD_TYPE=Debug` (for debugging) | ||
| - `-DUSE_ASAN=ON` (Address Sanitizer) | ||
| - `-DUSE_TSAN=ON` (Thread Sanitizer - cannot combine with ASAN) | ||
| - `-DUSE_UBSAN=ON` (Undefined Behavior Sanitizer) | ||
|
|
||
| **Example with sanitizers:** | ||
| ```bash | ||
| cmake -B build -S . -DCMAKE_INSTALL_PREFIX=install -DCMAKE_BUILD_TYPE=Debug -DUSE_ASAN=ON -DUSE_UBSAN=ON | ||
| ``` | ||
|
|
||
| ### Development Workflow | ||
|
|
||
| **After making code changes:** | ||
| ```bash | ||
| # Rebuild (incremental build is faster) | ||
| cmake --build build --target install -- -j8 | ||
|
|
||
| # Run relevant tests | ||
| ctest --test-dir build -V -R "specific_test_name" | ||
|
|
||
| # Test functionality | ||
| source install/bin/eicrecon-this.sh | ||
| eicrecon --help | ||
| ``` | ||
|
|
||
| **Before committing changes - ALWAYS run:** | ||
| ```bash | ||
| # These must pass or CI will fail | ||
| cmake --build build --target install -- -j8 | ||
| ctest --test-dir build -V | ||
| ``` | ||
|
|
||
| ## Repository Structure and Navigation | ||
|
|
||
| ### Key Directories | ||
| - `src/algorithms/` - Core physics algorithms | ||
| - `src/factories/` - JANA factory implementations | ||
| - `src/services/` - Service components | ||
| - `src/tests/` - Unit and integration tests | ||
| - `src/benchmarks/` - Performance benchmarks | ||
| - `docs/` - Documentation and tutorials | ||
| - `cmake/` - CMake configuration files | ||
|
|
||
| ### Important Test Suites | ||
| - `src/tests/algorithms_test/` - Algorithm unit tests (uses Catch2) | ||
| - `src/tests/omnifactory_test/` - Factory framework tests | ||
| - `src/tests/tracking_test/` - Tracking algorithm tests | ||
| - `src/tests/geometry_navigation_test/` - Geometry tests | ||
|
|
||
| ### Commonly Modified Files | ||
| When making physics algorithm changes: | ||
| 1. Check `src/algorithms/` for the relevant algorithm | ||
| 2. Look for corresponding factory in `src/factories/` | ||
| 3. Check for tests in `src/tests/algorithms_test/` | ||
| 4. Update documentation in `docs/` if needed | ||
|
|
||
| ## Manual Build (NOT RECOMMENDED) | ||
|
|
||
| **WARNING: Manual dependency installation is extremely complex and time-consuming. Only attempt if eic-shell is absolutely unavailable.** | ||
|
|
||
| Manual build requires installing these dependencies in order: | ||
| - C++20 compiler (GCC 10+ or Clang 10+) | ||
| - CMake 3.24+ | ||
| - Python 3.8+ with pyyaml, jinja2 | ||
| - boost 1.70+ | ||
| - ROOT 6.26+ (with C++17) | ||
| - JANA 2.2.0+ | ||
| - fmt 9.0.0+ | ||
| - spdlog 1.11.0+ | ||
| - PODIO 0.17+ | ||
| - EDM4hep 0.7.1+ | ||
| - DD4hep 1.21+ | ||
| - ACTS 33.0.0+ | ||
| - Eigen 3.3+ | ||
|
|
||
| **Each dependency build can take 15-60 minutes. Total time: 4-8 hours.** | ||
|
|
||
| See `docs/get-started/manual-build.md` for complete manual build instructions, but expect significant complexity and troubleshooting. | ||
|
|
||
| ## Timing Expectations and Critical Warnings | ||
|
|
||
| **NEVER CANCEL these operations - they are expected to take significant time:** | ||
|
|
||
| | Operation | Expected Time | Minimum Timeout | | ||
| |-----------|---------------|-----------------| | ||
| | Initial build | 30-60 minutes | 90 minutes | | ||
| | Incremental build | 5-15 minutes | 30 minutes | | ||
| | Full test suite | 15-30 minutes | 45 minutes | | ||
| | Individual test | 1-5 minutes | 10 minutes | | ||
| | Manual dependency build | 4-8 hours | Not recommended | | ||
|
|
||
| **Use appropriate timeouts for all long-running commands. The CI system uses ccache and parallel builds which can still take significant time.** | ||
|
|
||
| ## Common Issues and Solutions | ||
|
|
||
| **Build fails with missing dependencies:** | ||
| - Ensure you're in eic-shell environment | ||
| - Run `source /opt/detector/epic-main/bin/thisepic.sh` | ||
| - Check CMake configuration output for specific missing packages | ||
|
|
||
| **Tests fail:** | ||
| - Verify environment setup: `source install/bin/eicrecon-this.sh` | ||
| - Check library paths are correct | ||
| - Run individual failing tests for detailed output | ||
|
|
||
| **Performance issues:** | ||
| - Use `-j8` for parallel builds (adjust number based on available cores) | ||
| - Consider using ccache for repeated builds | ||
| - Debug builds are significantly slower than Release builds | ||
|
|
||
| ## CI/CD Integration | ||
|
|
||
| The project uses GitHub Actions with the following key checks: | ||
| - Builds on multiple compilers (GCC, Clang) | ||
| - Multiple build types (Release, Debug) | ||
| - Address, Thread, and UB sanitizers | ||
| - clang-tidy and include-what-you-use checks | ||
| - Extensive physics simulation and reconstruction tests | ||
|
|
||
| **All commits must pass these checks. Run local builds and tests before pushing.** | ||
|
|
||
| ## Data Files and Physics Validation | ||
|
|
||
| **Physics reconstruction requires specific input data formats:** | ||
| - Input: `.edm4hep.root` files (simulated physics events) | ||
| - Output: `.edm4eic.root` files (reconstructed physics data) | ||
|
|
||
| **Sample reconstruction command:** | ||
| ```bash | ||
| export DETECTOR_CONFIG=${DETECTOR}_craterlake | ||
| eicrecon -Ppodio:output_file=output.edm4eic.root input_simulation.edm4hep.root | ||
| ``` | ||
|
|
||
| **For physics validation, you need appropriate simulation files. The CI system generates these automatically.** | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.