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 package definition for Conan #4485

Merged
merged 3 commits into from
Jun 27, 2023
Merged

Conversation

thejohnfreeman
Copy link
Collaborator

Our project doesn't quite install itself correctly.

  • It forgets to install (at least) one header. I have not exhaustively searched for more. We can add them as they come up. Longer term I want us to move headers to a separate directory in the project that should mirror the way they are installed so that reviewers don't have to do any mental calculus.
  • It installs secp256k1 headers to a path other than how they are included in our code. I changed the includes because the installation path is correct.
  • I forgot a few requirements in the Conan package info. This bug only reveals itself when indirectly trying to use one of the functions exported by those requirements.

@intelliot
Copy link
Collaborator

For context, can you share example(s) of projects that could (or should) use the project package - and therefore use this fixed package definition?

@@ -6,8 +6,8 @@

#include <string.h>

#include "include/secp256k1.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: paths work when building within our codebase but not the source as it would be installed on the system

@@ -153,6 +153,7 @@ install (
src/ripple/basics/Blob.h
src/ripple/basics/Buffer.h
src/ripple/basics/CountedObject.h
src/ripple/basics/Expected.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: we forgot to add this to the cmake headers - doesn't affect us since we build in our codebase, but others depending on libxrpl would run into this.

this makes it possible for others to depend on libxrpl

@intelliot
Copy link
Collaborator

note: fairly light review - mostly changing include paths. if needed, @thejohnfreeman could walk through what's changing and why

@intelliot intelliot requested review from seelabs and removed request for ximinez April 12, 2023 18:26
@intelliot
Copy link
Collaborator

@seelabs to look

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.

👍

@intelliot
Copy link
Collaborator

@legleux would you be able to review this?

@intelliot intelliot added this to the 1.12 milestone May 30, 2023
@legleux
Copy link
Collaborator

legleux commented May 30, 2023

LGTM 👍

@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 6, 2023
@intelliot intelliot merged commit 6b4437d into XRPLF:develop Jun 27, 2023
@thejohnfreeman thejohnfreeman deleted the libxrpl branch June 27, 2023 12:47
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Jul 10, 2023
Fix the libxrpl library target for consumers using Conan.

* Fix installation issues and update includes.
* Update requirements in the Conan package info.
  * libxrpl requires openssl::crypto.

(Conan is a software package manager for C++.)
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
Fix the libxrpl library target for consumers using Conan.

* Fix installation issues and update includes.
* Update requirements in the Conan package info.
  * libxrpl requires openssl::crypto.

(Conan is a software package manager for C++.)
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
Fix the libxrpl library target for consumers using Conan.

* Fix installation issues and update includes.
* Update requirements in the Conan package info.
  * libxrpl requires openssl::crypto.

(Conan is a software package manager for C++.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build System 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.

4 participants