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

Finish physical redesign #4997

Merged
merged 7 commits into from
Jun 20, 2024
Merged

Finish physical redesign #4997

merged 7 commits into from
Jun 20, 2024

Conversation

thejohnfreeman
Copy link
Collaborator

@thejohnfreeman thejohnfreeman commented Apr 19, 2024

This is the final and most disruptive phase of the physical redesign. It makes these changes:

  • CMake modules are moved into their own top-level cmake/ directory.
  • libxrpl headers are moved into include/xrpl/.
  • libxrpl sources are moved to src/libxrpl/.
  • rippled sources are moved to src/xrpld/.
  • The ripple C++ namespace is unchanged.
  • libxrpl headers are installed to both the xrpl/ and ripple/ subdirectories of the header installation prefix. (ripple/ is a symbolic link to xrpl/, even on Windows.) They include each other with path prefix xrpl/, but existing projects can continue to include them with path prefix ripple/ and migrate to prefix xrpl/ at their own pace.
  • rippled is installed as both rippled and xrpld. (xrpld is a symbolic link to rippled, even on Windows.)
  • CMake source lists are computed from globs. Newly added source and header files are automatically built and installed without any intervention from contributors.
  • CMake target names are cleaned up and standardized.

There is a non-trivial (but, I think, easy) process to merge existing branches-in-progress with this one. I will describe it here. Look at the sequence of commits. Notice the commit with subject "Add bin/physical.sh". Call that the checkpoint commit. There are no file movements before the checkpoint commit. The four commits following the checkpoint commit are generated by running the bin/physical.sh script added in the checkpoint commit.

When you want to merge, you should:

  • Merge with the latest ancestor of this branch in develop. Resolve conflicts yourself. Be sure that all of the source files added in your branch (if any) appear in the correct source lists in Builds/CMake/RippledCore.cmake, between the BEGIN/END marker comments (example) . The restructure script moves only the files in these lists, and stops early if any files are leftover (because they were omitted from the lists). Remember the commit ID of this merge. Call it your marker commit.
    git merge c706926
    marker=$(git log -1 --pretty=format:%h)
    
  • Merge with the checkpoint commit. Resolve conflicts yourself.
    git merge f6879da
    
  • Regenerate the next four commits by executing bin/physical.sh, passing it your marker commit from the first step:
    ./bin/physical.sh ${marker}
    
  • Check the sort order in Builds/levelization/results/ordering.txt. Lines beginning with xrpl. must appear before lines beginning with xrpld. Different versions of sort have been inconsistent on this matter in my experience. On my machine, sort puts xrpl. after xrpld, which does not match the output of sort in our CI job that checks the file.
    ${EDITOR} Builds/levelization/results/ordering.txt
    git add --update .
    git commit --amend
    

Then you can git merge -X ours $upstream where upstream points to the head of this branch. It will automatically resolve conflicts in your branch's favor. Then you can git diff $upstream and see your work ported.

This PR requires the same finishing steps as #4966:

  • Confirm it has at least two approvals from qualified reviewers.
  • Rebase the commits on top of develop. The commit sequence that predates the review should be preserved. Fixes made since the review opened should be merged into earlier commits.
  • Rewrite non-substantive (e.g. formatting) commits as authored by "Pretty Printer <cpp@ripple.com>".
  • Add rewritten non-substantive commit IDs to .git-blame-ignore-revs.
  • Confirm the branch still passes all tests after making these changes.

@thejohnfreeman thejohnfreeman force-pushed the restructure branch 2 times, most recently from e604953 to e068be2 Compare April 20, 2024 15:12
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.3%. Comparing base (c706926) to head (714bee1).

Current head 714bee1 differs from pull request most recent head 7cf4611

Please upload reports for the commit 7cf4611 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #4997   +/-   ##
=======================================
  Coverage     71.3%   71.3%           
=======================================
  Files          796     796           
  Lines        66987   66987           
  Branches     10886   10865   -21     
=======================================
+ Hits         47765   47791   +26     
+ Misses       19222   19196   -26     
Files Coverage Δ
include/xrpl/basics/BasicConfig.h 87.7% <ø> (ø)
include/xrpl/basics/Buffer.h 100.0% <ø> (ø)
include/xrpl/basics/ByteUtilities.h 100.0% <ø> (ø)
include/xrpl/basics/CompressionAlgorithms.h 0.0% <ø> (ø)
include/xrpl/basics/CountedObject.h 100.0% <ø> (ø)
include/xrpl/basics/DecayingSample.h 87.9% <ø> (ø)
include/xrpl/basics/Expected.h 100.0% <ø> (ø)
include/xrpl/basics/FeeUnits.h 90.1% <ø> (ø)
include/xrpl/basics/IOUAmount.h 100.0% <ø> (ø)
include/xrpl/basics/LocalValue.h 100.0% <ø> (ø)
... and 158 more

... and 1256 files with indirect coverage changes

Impacted file tree graph

@thejohnfreeman thejohnfreeman force-pushed the restructure branch 2 times, most recently from 4d2f327 to e25ebac Compare April 22, 2024 17:11
@thejohnfreeman thejohnfreeman marked this pull request as ready for review April 22, 2024 17:12
@Bronek Bronek self-requested a review May 3, 2024 10:31
conanfile.py Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@Bronek
Copy link
Collaborator

Bronek commented May 20, 2024

Note, this is moving files from impl and details to their parent directories, which I think is a problem because that expands the libxrpl API surface with the following files (which were designed to be implementation and not API)

json/impl/json_assert.h

protocol/impl/STVar.h  protocol/impl/b58_utils.h  protocol/impl/secp256k1.h  protocol/impl/token_errors.h

resource/impl/Entry.h  resource/impl/Import.h  resource/impl/Key.h  resource/impl/Kind.h  resource/impl/Logic.h  resource/impl/Tuning.h

server/impl/BaseHTTPPeer.h  server/impl/BaseWSPeer.h  server/impl/JSONRPCUtil.h  server/impl/PlainHTTPPeer.h  server/impl/SSLHTTPPeer.h  server/impl/ServerImpl.h
server/impl/BasePeer.h      server/impl/Door.h        server/impl/LowestLayer.h  server/impl/PlainWSPeer.h    server/impl/SSLWSPeer.h    server/impl/io_list.h

@Bronek
Copy link
Collaborator

Bronek commented Jun 7, 2024

Note, this is moving files from impl and details to their parent directories, which I think is a problem because that expands the libxrpl API surface with the following files (which were designed to be implementation and not API)

json/impl/json_assert.h

protocol/impl/STVar.h  protocol/impl/b58_utils.h  protocol/impl/secp256k1.h  protocol/impl/token_errors.h

resource/impl/Entry.h  resource/impl/Import.h  resource/impl/Key.h  resource/impl/Kind.h  resource/impl/Logic.h  resource/impl/Tuning.h

server/impl/BaseHTTPPeer.h  server/impl/BaseWSPeer.h  server/impl/JSONRPCUtil.h  server/impl/PlainHTTPPeer.h  server/impl/SSLHTTPPeer.h  server/impl/ServerImpl.h
server/impl/BasePeer.h      server/impl/Door.h        server/impl/LowestLayer.h  server/impl/PlainWSPeer.h    server/impl/SSLWSPeer.h    server/impl/io_list.h

@thejohnfreeman thanks for addressing this concern.

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.

I like this a lot.

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.

I noticed that the most recent rebase has reverted the change to which I refer here
#4997 (comment)

Is it intentional ?

@Bronek
Copy link
Collaborator

Bronek commented Jun 10, 2024

I noticed that the most recent rebase has reverted the change to which I refer here #4997 (comment)

Thanks @thejohnfreeman , I see that's fixed now.

@scottschurr
Copy link
Collaborator

I'm trying to follow the directions for merging an in-process branch with the physical restructure. Everything was going well until I ran bin/physical.sh. There I had two surprises.

The output from bin/physical.sh was this:

move protocol buffers
move libxrpl headers
move libxrpl sources
check leftovers
leftover files:
beast//utility/src/beast_PropertyStream.cpp crypto//impl/secure_erase.cpp

The leftover files was a surprise, and the instructions did not say what to do if that happened, so I'm reporting it here.

Additionally, I checked the sort order of Builds/levelization/results/ordering.txt as instructed. To my surprise it contained no lines that started with either xrpl. or xrpld. All 229 lines in the file started with either ripple. or test.. So I suspect something went wrong.

Suggestions? Thanks.

@thejohnfreeman
Copy link
Collaborator Author

@scottschurr what branch did you try?

@scottschurr
Copy link
Collaborator

@thejohnfreeman
Copy link
Collaborator Author

I just merged that branch without a hitch: https://github.com/thejohnfreeman/rippled/tree/blocked-book

I'll try to replicate your experience on macOS soon.

@scottschurr
Copy link
Collaborator

Thanks. And if there are any experiments you would like me to try I'm happy to do so. Best of luck.

@thejohnfreeman
Copy link
Collaborator Author

thejohnfreeman commented Jun 12, 2024

The problem is what I expected, the environment. The requirements I found missing on macOS are:

  • GNU sed
  • GNU find
  • Bash >= 4 (for readarray)
  • clang-format-10 must be on the PATH

Can you run the script in a Linux environment? Alternatively, if you can get these programs on your PATH (brew install gnu-sed findutils will install gsed and gfind; Homebrew doesn't have clang-format-10) and invoke the script with Bash (the default shell in macOS is now Zsh), then it should work.

@scottschurr
Copy link
Collaborator

I don't have a readily available Linux environment, but I can install gsed and gfind using homebrew and invoke the script with Bash. I'm in the middle of a different (unrelated) problem right now, but I should be able to get you results in a day or so. Thanks for the help.

@thejohnfreeman thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 20, 2024
thejohnfreeman added a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
- Remove CMake module "MultiConfig".
- Update clang-format configuration, CodeCov configuration,
  levelization script.
- Replace source lists in CMake with globs.
thejohnfreeman added a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
thejohnfreeman pushed a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
thejohnfreeman pushed a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
thejohnfreeman pushed a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
thejohnfreeman pushed a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
thejohnfreeman added a commit to thejohnfreeman/rippled that referenced this pull request Jun 20, 2024
@seelabs seelabs merged commit 7cf4611 into XRPLF:develop Jun 20, 2024
17 checks passed
@seelabs seelabs mentioned this pull request Jun 20, 2024
1 task
@thejohnfreeman thejohnfreeman deleted the restructure branch June 20, 2024 21:05
ximinez added a commit to ximinez/rippled that referenced this pull request Jul 1, 2024
* upstream/develop: (32 commits)
  fixInnerObjTemplate2 amendment (XRPLF#5047)
  Set version to 2.3.0-b1
  Ignore restructuring commits (XRPLF#4997)
  Recompute loops (XRPLF#4997)
  Rewrite includes (XRPLF#4997)
  Rearrange sources (XRPLF#4997)
  Move CMake directory (XRPLF#4997)
  Add bin/physical.sh (XRPLF#4997)
  Prepare to rearrange sources: (XRPLF#4997)
  Change order of checks in amm_info: (XRPLF#4924)
  Add the fixEnforceNFTokenTrustline amendment: (XRPLF#4946)
  Replaces the usage of boost::string_view with std::string_view (XRPLF#4509)
  docs: explain how to find a clang-format patch generated by CI (XRPLF#4521)
  XLS-52d: NFTokenMintOffer (XRPLF#4845)
  chore: remove repeat words (XRPLF#5041)
  Expose all amendments known by libxrpl (XRPLF#5026)
  fixReducedOffersV2: prevent offers from blocking order books: (XRPLF#5032)
  Additional unit tests for testing deletion of trust lines (XRPLF#4886)
  Fix conan typo: (XRPLF#5044)
  Add new command line option to make replaying transactions easier: (XRPLF#5027)
  ...
@ximinez ximinez mentioned this pull request Jul 10, 2024
2 tasks
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
- Remove CMake module "MultiConfig".
- Update clang-format configuration, CodeCov configuration,
  levelization script.
- Replace source lists in CMake with globs.
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants