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

Fix the package recipe for consumers #4631

Closed
wants to merge 11 commits into from

Conversation

thejohnfreeman
Copy link
Collaborator

@thejohnfreeman thejohnfreeman commented Jul 19, 2023

This branch is named clio because this work was driven to support its dependence on libxrpl, but Clio is just an example consumer. These changes should benefit all current and future consumers.

  • "Rename" the type LedgerInfo to LedgerHeader (but leave an alias for LedgerInfo to not yet disturb existing uses). Put it in its own public header, named after itself, so that it is more easily found.
  • Move the type Fees and NFT serialization functions into public (read: installed) headers.
  • Compile the XRPL and gRPC protocol buffers directly into libxrpl and install their headers. Fix the Conan recipe to correctly export these types.

Addresses change (2) in XRPLF/XRPL-Standards#121.

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

I may not know much about the inner workings of rippled but the changes look fine to me.
I'd like to see the one TODO comment getting resolved or maybe an issue number added to it if it is planned as a separate pr.
It would also be great to eventually rename most/all things 'ripple' to something more xrpl-y but i can see how that may be a huge effort and probably is out of scope of this pr.

Thanks for helping Clio out @thejohnfreeman 👍💪

@intelliot
Copy link
Collaborator

note: @thejohnfreeman will be making more changes per requests from @godexsoft

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

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

This PR enables Clio to build successfully on Mac and Linux (tested with AppleClang and gcc11/gcc12) 👍🚀

@intelliot intelliot assigned ximinez and unassigned legleux Aug 2, 2023
@thejohnfreeman thejohnfreeman added Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. and removed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. labels Aug 5, 2023
@thejohnfreeman
Copy link
Collaborator Author

thejohnfreeman commented Aug 6, 2023

I forgot: one reason I wanted @ximinez's review while we have Windows CI disabled is to verify that the common filename is not an issue in the MSVC build. I know that MSVC, while it is building, reports translation units by filename instead of the full path. I don't know if it's dumping all the object files into one directory, like the way Windows handles shared libraries, but if it is, then I might expect to see a link error.

@ximinez
Copy link
Collaborator

ximinez commented Aug 8, 2023

I forgot: one reason I wanted @ximinez's review while we have Windows CI disabled is to verify that the common filename is not an issue in the MSVC build. I know that MSVC, while it is building, reports translation units by filename instead of the full path. I don't know if it's dumping all the object files into one directory, like the way Windows handles shared libraries, but if it is, then I might expect to see a link error.

I don't remember exactly when or how, but the CMake config was updated a while back to do the right thing when multiple files have the same name. On top of that, in the linked example (LedgerHeader.cpp), the two files are in different projects (xrpl_core and rippled).

Amusingly, the unity build outputs a warning about potential conflicts for some existing files, but that warning amounts to nothing:

$ cmake --build build/cmake/msvc19.ON --config Debug -- -m
[...]
  Building Custom Rule C:/Dev/rippled/review1/CMakeLists.txt
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(1091,5): warning MSB8027: Two or more files with the name of Manifest.cpp will produce outputs to the same location. This can lead to an incorrect build result.  The files involved are C:\Dev\rippled\review1\src\ripple\app\misc\impl\Manifest.cpp, C:\Dev\rippled\review1\src\ripple\rpc\handlers\Manifest.cpp. [C:\Dev\rippled\review1\build\cmake\msvc19.ON\rippled.vcxproj]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(1091,5): warning MSB8027: Two or more files with the name of Shard.cpp will produce outputs to the same location. This can lead to an incorrect build result.  The files involved are C:\Dev\rippled\review1\src\ripple\app\rdb\backend\detail\impl\Shard.cpp, C:\Dev\rippled\review1\src\ripple\nodestore\impl\Shard.cpp. [C:\Dev\rippled\review1\build\cmake\msvc19.ON\rippled.vcxproj]
  unity_35_cxx.cxx
  unity_34_cxx.cxx
  unity_33_cxx.cxx
[...]

All of which is to say that this branch builds fine in MSVC.

@thejohnfreeman thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 9, 2023
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Very nice restructuring

@intelliot
Copy link
Collaborator

Suggested commit message for when this PR is squashed and merged:

Fix the package recipe for consumers of libxrpl (#4631)

- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
https:/XRPLF/XRPL-Standards/discussions/121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>

(Feel free to propose an alternative)

manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
This was referenced Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
@manojsdoshi manojsdoshi mentioned this pull request Aug 19, 2023
ximinez pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 19, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 21, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
shawnxie999 pushed a commit to shawnxie999/rippled that referenced this pull request Aug 28, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
- "Rename" the type `LedgerInfo` to `LedgerHeader` (but leave an alias
  for `LedgerInfo` to not yet disturb existing uses). Put it in its own
  public header, named after itself, so that it is more easily found.
- Move the type `Fees` and NFT serialization functions into public
  (installed) headers.
- Compile the XRPL and gRPC protocol buffers directly into `libxrpl` and
  install their headers. Fix the Conan recipe to correctly export these
  types.

Addresses change (2) in
XRPLF/XRPL-Standards#121.

For context: This work supports Clio's dependence on libxrpl. Clio is
just an example consumer. These changes should benefit all current and
future consumers.

---------

Co-authored-by: cyan317 <[email protected]>
Signed-off-by: Manoj Doshi <[email protected]>
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
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants