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

Use platform-agnostic library names in Conan recipe #4831

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

thejohnfreeman
Copy link
Collaborator

Dependents like validator-keys-tool are not correctly linking to libxrpl on Windows because the library filenames are different. I believe that using the platform-agnostic names (as the linker knows them) will work. I kindly ask that @ximinez please test for me with these steps:

  1. Clone this branch.
  2. conan export .
  3. Try to build validator-keys-tool against dependency xrpl/2.0.0-b4 (which should be the version that was exported in step 2).

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.

With the change to the library name below, and two changes to your branch in validator-keys-tool (ripple/validator-keys-tool#44), everything seems to work.

Changes:

  1. Obviously, change the dependency in conanfile.txt to match the rippled version in this branch (2.0.0-b4).
  2. Remove the beast::uint_test::dstream debugger output class. I don't know when it stopped working, so it's easier to just take it out for now.

Those changes are in commit ximinez/validator-keys-tool@7a70e66

conanfile.py Show resolved Hide resolved
@intelliot
Copy link
Collaborator

intelliot commented Jan 22, 2024

note: the following are blockers to merging #4831

With the change to the library name below, and two changes to your branch in validator-keys-tool (ripple/validator-keys-tool#44), everything seems to work.

Changes:

  1. Obviously, change the dependency in conanfile.txt to match the rippled version in this branch (2.0.0-b4).
  2. Remove the beast::uint_test::dstream debugger output class. I don't know when it stopped working, so it's easier to just take it out for now.

Those changes are in commit ximinez/validator-keys-tool@7a70e66

Update: I assume these blockers have been resolved and are no longer blocking.

@intelliot
Copy link
Collaborator

notes:

  • the secp256k1 dependency is named secp256k1 on linux, but named libsecp256k1 on Windows
  • need to figure out what names are actually being created on each platform
  • figure out why they are not libname.a on Linux and name.lib on Windows
  • fix CMake to produce those names and not any other names (with different names for shared libraries)

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f9e3658) 61.49% compared to head (0b3908b) 61.48%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4831      +/-   ##
===========================================
- Coverage    61.49%   61.48%   -0.01%     
===========================================
  Files          797      797              
  Lines        70121    70121              
  Branches     36238    36238              
===========================================
- Hits         43119    43114       -5     
- Misses       19759    19760       +1     
- Partials      7243     7247       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

With the latest changes, I'm able to build ripple/validator-keys-tool#44 on both Windows and Linux

@intelliot intelliot added Build System Libraries Dependencies Issues associated with 3rd party dependencies (RocksDB, SQLite, etc) Will Need Documentation Perf impact not expected Change is not expected to improve nor harm performance. labels Jan 26, 2024
@intelliot
Copy link
Collaborator

Will Need Documentation:

  1. make it easy for developers to learn how to depend on libxrpl;
  2. prospective validator operators should be able to build validator-keys-tool

@intelliot
Copy link
Collaborator

note: this should be merged prior to ripple/validator-keys-tool#44 (the version number used in that PR will need to be updated)

@thejohnfreeman at your convenience, please:

  1. Bring this branch up-to-date with the base branch (develop)
  2. Indicate when, from your perspective, this PR is ready to merge. (When it's ready, we'll put the Passed label on it.)

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

Suggested commit message:

fix(libxrpl): change library names in Conan recipe (#4831)

Use consistent platform-agnostic library names on all platforms.

Fix an issue that prevents dependents like validator-keys-tool from
linking to libxrpl on Windows.

It is bad practice to change the binary base name depending on the
platform. CMake already manipulates the base name into a final name that
fits the conventions of the platform. Linkers accept base names on the
command line and then look for conventional names on disk.

@intelliot intelliot merged commit 6f00d32 into XRPLF:develop Feb 2, 2024
17 checks passed
@thejohnfreeman thejohnfreeman deleted the conan branch February 28, 2024 20:23
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
Use consistent platform-agnostic library names on all platforms.

Fix an issue that prevents dependents like validator-keys-tool from
linking to libxrpl on Windows.

It is bad practice to change the binary base name depending on the
platform. CMake already manipulates the base name into a final name that
fits the conventions of the platform. Linkers accept base names on the
command line and then look for conventional names on disk.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build System Dependencies Issues associated with 3rd party dependencies (RocksDB, SQLite, etc) Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Perf impact not expected Change is not expected to improve nor harm performance. Will Need Documentation
Projects
Status: 🚢 Released in 2.2.0
Development

Successfully merging this pull request may close these issues.

5 participants