Skip to content
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

Enforce levelization in libxrpl with CMake #5111

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

thejohnfreeman
Copy link
Collaborator

@thejohnfreeman thejohnfreeman commented Aug 28, 2024

Adds two CMake functions:

  • add_module(library subdirectory): Declares an OBJECT "library" (a CMake abstraction for a collection of object files) with sources from the given subdirectory of the given library, representing a module. Isolates the module's headers by creating a subdirectory in the build directory, e.g. .build/tmp123, that contains just a symlink, e.g. .build/tmp123/basics, to the module's header directory, e.g. include/xrpl/basics, in the source directory, and putting .build/tmp123 (but not include/xrpl) on the include path of the module sources. This prevents the module sources from including headers not explicitly linked to the module in CMake with target_link_libraries.
  • target_link_modules(library scope modules...): Links the library target to each of the module targets, and removes their sources from its source list (so they are not compiled and linked twice).

Uses these functions to separate and explicitly link modules in libxrpl:

  • Level 01: beast
  • Level 02: basics
  • Level 03: json, crypto
  • Level 04: protocol
  • Level 05: resource, server

Define each "module" of libxrpl as a separate OBJECT library in CMake.
Link each one only to modules of a lower level.
Then link all of them into libxrpl itself.
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.4%. Comparing base (63209c2) to head (0929033).

Files with missing lines Patch % Lines
include/xrpl/basics/SHAMapHash.h 0.0% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5111     +/-   ##
=========================================
+ Coverage     76.1%   76.4%   +0.2%     
=========================================
  Files          762     761      -1     
  Lines        61469   62023    +554     
  Branches      8121    8111     -10     
=========================================
+ Hits         46807   47364    +557     
+ Misses       14662   14659      -3     
Files with missing lines Coverage Δ
include/xrpl/basics/Number.h 100.0% <ø> (ø)
include/xrpl/basics/base_uint.h 96.8% <100.0%> (+0.2%) ⬆️
include/xrpl/basics/partitioned_unordered_map.h 99.2% <100.0%> (+0.1%) ⬆️
include/xrpl/protocol/AmountConversions.h 87.7% <ø> (+0.2%) ⬆️
include/xrpl/protocol/FeeUnits.h 90.4% <ø> (ø)
include/xrpl/protocol/Fees.h 100.0% <ø> (ø)
include/xrpl/protocol/LedgerHeader.h 100.0% <ø> (ø)
include/xrpl/protocol/PayChan.h 100.0% <ø> (ø)
include/xrpl/protocol/Quality.h 98.6% <ø> (+0.1%) ⬆️
include/xrpl/protocol/STAmount.h 91.7% <ø> (+0.8%) ⬆️
... and 27 more

... and 107 files with indirect coverage changes

Impacted file tree graph

@ximinez
Copy link
Collaborator

ximinez commented Aug 29, 2024

Not a review yet, but this is such a cool idea.

cmake/isolate_headers.cmake Outdated Show resolved Hide resolved
Comment on lines 125 to 126
PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably not needed ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, upon some testing, this whole target_include_directories seem to be not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed now that all of the subdirectories in libxrpl are modules. It was necessary as I migrated them one-by-one, and it could be necessary in the future if someone adds a non-module subdirectory. Could be nice if it "just works" without tampering with the CMake. What do you think?

Copy link
Collaborator

@Bronek Bronek Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps comment out and put a comment above, to explain why this is not necessary and in what condition it might become necessary again ?

@Bronek
Copy link
Collaborator

Bronek commented Sep 20, 2024

I like how you moved extract to appropriate locations, but it seems that extract is function-wise the same as hash.

Given we already specialize both boost::hash (e.g. in IPEndpoint.h) and std::hash (e.g. in Book.h) I think that cleaning this up belongs in a different PR (also converting extract to hash), so maybe just take a note for future refactoring.

@Bronek
Copy link
Collaborator

Bronek commented Sep 23, 2024

I do wonder whether Builds/levelization is becoming superfluous with this change. Why should we keep it, if the ordering of modules (and header isolation) enforce it for us ?

Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall very nice change. Some comments in the middle, unsure if they require changes in this PR or not.

@thejohnfreeman
Copy link
Collaborator Author

Builds/levelization is still used to diagnose cycles in xrpld. I want to remove it after we've eliminated all of the cycles.

@Bronek
Copy link
Collaborator

Bronek commented Sep 24, 2024

Builds/levelization is still used to diagnose cycles in xrpld. I want to remove it after we've eliminated all of the cycles.

Makes sense. All the more reason to make an effort moving parts of xrpld into libxrpl, but that's separate discussion and more than one PR.

@Bronek Bronek self-requested a review September 24, 2024 16:23
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement, thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants