Enforce levelization in libxrpl with CMake

[thejohnfreeman]

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

[codecov[bot]]

Codecov Report

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

Project coverage is 77.9%. Comparing base (6d58065) to head (d7a030f). Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
include/xrpl/basics/SHAMapHash.h	0.0%	2 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5111   +/-   ##
=======================================
  Coverage     77.9%   77.9%           
=======================================
  Files          784     783    -1     
  Lines        66680   66677    -3     
  Branches      8118    8106   -12     
=======================================
  Hits         51922   51922           
+ Misses       14758   14755    -3     

Files with missing lines	Coverage Δ
include/xrpl/basics/Number.h	100.0% <ø> (ø)
include/xrpl/basics/base_uint.h	96.8% <100.0%> (+0.1%)	:arrow_up:
include/xrpl/basics/partitioned_unordered_map.h	99.2% <100.0%> (+<0.1%)	:arrow_up:
include/xrpl/protocol/AmountConversions.h	87.7% <ø> (ø)
include/xrpl/protocol/FeeUnits.h	90.4% <ø> (ø)
include/xrpl/protocol/Fees.h	100.0% <ø> (ø)
include/xrpl/protocol/IOUAmount.h	100.0% <ø> (ø)
include/xrpl/protocol/LedgerHeader.h	100.0% <ø> (ø)
include/xrpl/protocol/MPTAmount.h	100.0% <100.0%> (ø)
include/xrpl/protocol/PayChan.h	100.0% <ø> (ø)
... and 35 more

... and 5 files with indirect coverage changes

[ximinez]

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

[Bronek]

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]

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 ?

[thejohnfreeman]

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

[Bronek]

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.

[ximinez]

Bad news right out the gate on Windows builds:

$ cmake build/cmake/msvc19.ON
-- Using Conan toolchain: C:/Dev/rippled/review1/build/conan.msvc19/build/generators/conan_toolchain.cmake
-- Conan toolchain: C++ Standard 20 with extensions OFF
-- Conan toolchain: Setting BUILD_SHARED_LIBS = OFF
-- Selecting Windows SDK version 10.0.22621.0 to target Windows 10.0.19045.
[...]
-- Conan: Component target declared 'xxHash::xxhash'
-- Conan: Including build module from 'C:/.conan/4b4fe1f3/1/lib/cmake/protobuf/protobuf-generate.cmake'
-- Conan: Including build module from 'C:/.conan/4b4fe1f3/1/lib/cmake/protobuf/protobuf-module.cmake'
-- Conan: Including build module from 'C:/.conan/4b4fe1f3/1/lib/cmake/protobuf/protobuf-options.cmake'
CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.beast/xrpl/beast':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:69 (add_module)
  CMakeLists.txt:123 (include)


CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.basics/xrpl/basics':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:76 (add_module)
  CMakeLists.txt:123 (include)


CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.json/xrpl/json':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:80 (add_module)
  CMakeLists.txt:123 (include)


CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.crypto/xrpl/crypto':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:83 (add_module)
  CMakeLists.txt:123 (include)


CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.protocol/xrpl/protocol':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:87 (add_module)
  CMakeLists.txt:123 (include)


CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.resource/xrpl/resource':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:94 (add_module)
  CMakeLists.txt:123 (include)


CMake Error at cmake/isolate_headers.cmake:44 (file):
  file failed to create symbolic link
  'C:/Dev/rippled/review1/build/cmake/msvc19.ON/modules/xrpl.libxrpl.server/xrpl/server':
  A required privilege is not held by the client.

Call Stack (most recent call first):
  cmake/add_module.cmake:25 (isolate_headers)
  cmake/RippledCore.cmake:97 (add_module)
  CMakeLists.txt:123 (include)


-- xrpl.libxrpl.basics with 14 sources took 14 sources from xrpl.libxrpl
-- xrpl.libxrpl.beast with 15 sources took 15 sources from xrpl.libxrpl
-- xrpl.libxrpl.crypto with 3 sources took 3 sources from xrpl.libxrpl
-- xrpl.libxrpl.json with 9 sources took 9 sources from xrpl.libxrpl
-- xrpl.libxrpl.protocol with 51 sources took 51 sources from xrpl.libxrpl
-- xrpl.libxrpl.resource with 4 sources took 4 sources from xrpl.libxrpl
-- xrpl.libxrpl.server with 2 sources took 2 sources from xrpl.libxrpl
-- Configuring incomplete, errors occurred!

[thejohnfreeman]

@ximinez ok, try again.

[ximinez]

It's building successfully now. Neat workaround. Super annoying that this is an issue, though.

I remember there were some file(CREATE_LINK calls elsewhere that were simply disabled under Windows. Would it be worth coming back to those to use this same technique?

Otherwise, I'll come back to the rest of this review as soon as I get some other priorities knocked out.

[ximinez]

@thejohnfreeman I see this has two approvals. Since I've verified that it builds under Windows, it's up to you if you want to wait for mine, too, or go ahead and add the "Passed" label and provide a commit message. I'll preemptively mark it as "Blocked" now to be sure we wait until after the 2.3.0 release before merging.

[thejohnfreeman]

@ximinez I would still appreciate your review.

[gregtatcam]

This commit breaks CLion IDE. Refactoring, break-points in include files, file links to compile errors, intellisense stopped working completely or partially depending on the edited files because the include files copied to modules directory and CLion treats those copied include files as the project's includes as opposed to the actual includes. This degrades CLion usability.