chore: Restore unity builds - #6328
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restores unity build support to the project while maintaining lower computational overhead by enabling it for only a single CI configuration (Ubuntu Jammy with GCC 12 Debug on linux/amd64). It also fixes a bug where coverage was being enabled for both Debian Bookworm and Debian Trixie instead of just Debian Bookworm.
Changes:
- Added
unityoption to Conan and CMake configuration systems - Configured specific files and targets to be excluded from unity builds (protobuf generated files, specific test files)
- Fixed coverage configuration to only apply to Debian Bookworm with GCC 15
- Updated documentation to describe the unity build option
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libxrpl/net/RegisterSSLCerts.cpp | Updated comment to reflect that unity builds are now being used again |
| conanfile.py | Added unity option with default value of False and passed it to CMake |
| cmake/XrplSettings.cmake | Implemented unity build configuration with conditional batch size setting |
| cmake/XrplCore.cmake | Excluded protobuf library from unity builds and marked specific test files for exclusion |
| cmake/XrplAddTest.cmake | Configured test targets to disable unity builds |
| BUILD.md | Added documentation for the unity build option with explanation of tradeoffs |
| .github/scripts/strategy-matrix/generate.py | Fixed coverage to only apply to Debian Bookworm and added unity build for Ubuntu Jammy configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| isolate_headers(${target} "${CMAKE_SOURCE_DIR}" "${CMAKE_SOURCE_DIR}/tests/${name}" PRIVATE) | ||
|
|
||
| # Make sure the test isn't optimized away in unity builds |
There was a problem hiding this comment.
The comment "Make sure the test isn't optimized away in unity builds" is misleading. Setting UNITY_BUILD_BATCH_SIZE to 0 effectively disables unity builds for the test target by placing each source file in its own unity group. A more accurate comment would be "Disable unity build for test targets to avoid potential conflicts between test sources" or "Prevent test source files from being combined in unity builds".
| # Make sure the test isn't optimized away in unity builds | |
| # Disable unity build for test targets to avoid potential conflicts between test sources |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6328 +/- ##
=========================================
- Coverage 79.9% 79.9% -0.0%
=========================================
Files 840 840
Lines 65549 65549
Branches 7267 7264 -3
=========================================
- Hits 52355 52353 -2
- Misses 13194 13196 +2
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thank you so much, Bart! This will make rebuilding faster. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ximinez The unity builds succeeded without the exceptions for the RPC and Invariant test files using configurations on all three platforms. While this is no guarantee it will work for all configurations - we will find out during a nightly run after merging - it is a good indicator that it may be that way. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
In certain cases, such as when modifying headers used by many compilation units, performing a unity build is slower than when performing a regular build with `ccache` enabled. There is also a benefit to a unity build in that it can detect things such as macro redefinitions within the group of files that are compiled together as a unit. This change therefore restores the ability to perform unity builds. However, instead of running every configuration with and without unity enabled, it is now only enabled for a single configuration to maintain lower computational use. As part of restoring the code, it became clear that currently two configurations have coverage enabled, since the check doesn't focus specifically on Debian Bookworm so it also applies to Debian Trixie. This has been fixed too in this change.
High Level Overview of Change
This change restores the ability to do unity builds, while only having a single configuration use it. It also fixes an issue with the coverage target.
Context of Change
In certain cases, such as when modifying headers used by many compilation units, performing a unity build is slower than when performing a regular build with
ccacheenabled. There is also a benefit to a unity build in that it can detect things such as macro redefinitions within the group of files that are compiled together as a unit. This PR therefore restores the ability to perform unity builds. However, instead of running every configuration with and without unity enabled, it is now only enabled for a single configuration to maintain lower computational use.As part of restoring the code, it became clear that currently two configurations have coverage enabled, since the check doesn't focus specifically on Debian Bookworm so it also applies to Debian Trixie. This has been fixed too in this PR.
Type of Change
.gitignore, formatting, dropping support for older tooling)