chore: Remove unity builds - #6300
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6300 +/- ##
=======================================
Coverage 79.9% 79.9%
=======================================
Files 840 840
Lines 65548 65548
Branches 7268 7264 -4
=======================================
+ Hits 52347 52350 +3
+ Misses 13201 13198 -3
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
mathbunnyru
left a comment
mathbunnyru
left a comment
There was a problem hiding this comment.
Let's:
- Wait for ccache to be fixed and properly making sure it works (the current setup let us down a few times already)
- Please, grep for word
unityand make sure there are no more places needed to be fixed
mathbunnyru
left a comment
mathbunnyru
left a comment
There was a problem hiding this comment.
Let's:
- Wait for ccache to be fixed and properly making sure it works (the current setup let us down a few times already)
- Please, grep for word
unityand make sure there are no more places needed to be fixed
This has now been fixed. The instance used an ephemeral NVME disk, and after AWS performed an unasked reboot of the machine the disk got wiped including configuration settings. The machine type has now been changed and a persistent EBS volume added. For future redeployments I also fixed the private IP so the private DNS will not have to be updated. Repeated CI runs show no more connection errors, with subsequent runs using cached build objects.
There were some all-caps references that escaped. These have now been fixed. |
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
My only qualm about merging this is that, while it's tangential to removing unity, I don't want to lose the knowledge about needing to downgrade ccache on Windows builds.
Would it be worth adding something to the "Troubleshooting" section in BUILD.md, or even just a comment in CCache.cmake as part of this PR?
Other than that, this looks good to go.
There was a problem hiding this comment.
Pull request overview
This PR removes unity build support from the XRPL codebase, as unity builds are no longer needed now that ccache is available on all platforms for build caching. The change simplifies the build configuration by removing unity-related options and conditionals.
Changes:
- Removed unity build option from Conan and CMake configuration files
- Updated CI/CD strategy matrix to remove unity/no-unity build variations
- Added ccache setup documentation to BUILD.md
- Updated header guard in rocksdb.h to reflect correct path convention
- Updated comments explaining legacy workarounds
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| conanfile.py | Removed unity option from options, default_options, and generate method |
| cmake/XrplSettings.cmake | Removed unity build option and related CMAKE_UNITY_BUILD settings |
| cmake/XrplCore.cmake | Removed unity build comments and exclusions for protobuf and test files |
| cmake/XrplAddTest.cmake | Removed unity build batch size settings for test targets |
| BUILD.md | Added ccache installation instructions and removed unity build documentation |
| .github/scripts/strategy-matrix/*.json | Removed unity cmake_args variations from all platform configurations |
| .github/scripts/strategy-matrix/generate.py | Removed unity build conditions from configuration filtering logic |
| src/libxrpl/net/RegisterSSLCerts.cpp | Updated comment to note unity builds are no longer used |
| include/xrpl/basics/rocksdb.h | Fixed header guard to follow XRPL_BASICS_ convention instead of XRPL_UNITY_ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I added a new section to |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Unity builds were intended to speed up builds, by bundling multiple files into compilation units. However, now that ccache is available on all platforms, there is no need for unity builds anymore, as ccache stores compiled individual build objects for reuse. This change therefore removes the ability to make unity builds.
Unity builds were intended to speed up builds, by bundling multiple files into compilation units. However, now that ccache is available on all platforms, there is no need for unity builds anymore, as ccache stores compiled individual build objects for reuse. This change therefore removes the ability to make unity builds.
Unity builds were intended to speed up builds, by bundling multiple files into compilation units. However, now that ccache is available on all platforms, there is no need for unity builds anymore, as ccache stores compiled individual build objects for reuse. This change therefore removes the ability to make unity builds.
High Level Overview of Change
This change removes the ability to make unity builds.
Context of Change
Unity builds were intended to speed up builds, by bundling multiple files into compilation units. However, now that
ccacheis available on all platforms, there is no need for unity builds anymore, asccachestores compiled individual build objects for reuse.For more information on
ccacheplease visit https://ccache.dev. You can install it via Homebrew, Chocolatey, and Apt.Type of Change
.gitignore, formatting, dropping support for older tooling)