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

Make it easy for projects to depend on libxrpl #4449

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

thejohnfreeman
Copy link
Collaborator

This changeset does a few things:

  • Adds an ALIAS target named xrpl::libxrpl for projects to link.
    • I chose this name for a few reasons. The current library target is named xrpl_core. There is no other "non-core" library target against which we need to distinguish the "core" library. We only export one library target, and it should just be named after the project to keep things simple and predictable. (I think underscores in target or library names are generally discouraged anyway.) Every target exported in CMake should be prefixed with the project name. The project should not be named after the company Ripple. I think most of us prefer to just name it after the ledger.
    • By adding an ALIAS target, existing consumers who use the xrpl_core target will not be affected. In the future, we can start a migration plan to make xrpl_core the ALIAS target (and libxrpl the "real" target, which will affect the filename of the compiled binary), and eventually remove it entirely.
  • Fixes the Conan recipe so that consumers using Conan import a target named xrpl::libxrpl. This way, every consumer can use the same instructions.
  • Documents the two easiest methods to depend on libxrpl. I have tested both. I'll work on adding CI tests for both methods.

See #4443.

@intelliot intelliot requested a review from seelabs March 9, 2023 03:30
@intelliot intelliot added this to the 1.11.0 milestone Mar 9, 2023
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

Both the build system changes look good and I liked the doc as well. Nice job on this!

Note that I did not create a project to exercise the instructions in the doc. I'll try to find time to do so, but this is a low risk change. I think it's ok to merge it. 👍 from me.

Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

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

I don't know CMake well enough to review changes so I'm not going to formally approve this, but I'm glad this is happening and this will be really useful.

@intelliot intelliot requested a review from drlongle March 21, 2023 15:19
@intelliot
Copy link
Collaborator

@thejohnfreeman if you agree this is ready to merge, you can put the Passed label on it, or just comment saying that it's good to go

@thejohnfreeman thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 22, 2023
@intelliot intelliot merged commit d772583 into XRPLF:develop Mar 23, 2023
@thejohnfreeman thejohnfreeman deleted the libxrpl branch March 23, 2023 14:28
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.

5 participants