-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 compatibility with Conan 2.x #5001
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
BUILD.md
Outdated
@@ -36,7 +36,7 @@ See [System Requirements](https://xrpl.org/system-requirements.html). | |||
Building rippled generally requires git, Python, Conan, CMake, and a C++ compiler. Some guidance on setting up such a [C++ development environment can be found here](./docs/build/environment.md). | |||
|
|||
- [Python 3.7](https://www.python.org/downloads/) | |||
- [Conan 1.55](https://conan.io/downloads.html) | |||
- [Conan 1.60](https://conan.io/downloads.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're making this change, but still not recommending >=2.0, maybe add a note to that effect here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5001 +/- ##
=========================================
- Coverage 71.1% 71.1% -0.0%
=========================================
Files 796 796
Lines 66997 66997
Branches 11005 10980 -25
=========================================
- Hits 47639 47633 -6
- Misses 19358 19364 +6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it on
- MacOS with apple-clang-15 (and with
-DBOOST_ASIO_DISABLE_CONCEPTS
workaround, moved to bothbuildenv
andrunenv
) - Linux with clang-17 (also with
-DBOOST_ASIO_DISABLE_CONCEPTS
workaround) - Linux with gcc-12
... and it worked on all three without a hitch. I tried only Debug
, but I've also run unit tests just to be sure.
One comment for possible documentation improvement, this aside this is brilliant - thank you !
which is why we are not recommending it yet. | ||
Notably, the `conan profile update` command is removed in 2.x. | ||
Profiles must be edited by hand. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a good place to also explain whether runenv
or buildenv
(or both) should be used instead of env
, for the workarounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be encouraging the conf.tools
settings. Environment variables are frowned upon, but sometimes a necessary backup depending on (a) the Conan version and (b) the Conan generator. As Conan matures, environment variables will stop working and everything will use conf.tools
. That said, I'll add a comment about environments in Conan 2.x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial approval: Thank you for adding the footnote. I'm just adding this to remove the pending change request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building rippled
is fine but building the package has issues which I'm sure are expected at the moment
This PR makes a few small changes to
conanfile.py
that make it work with Conan 2.x in my basic testing. I don't think we should recommend 2.x yet. It requires a small change to theconan export
instructions, and theconan profile update
instructions are completely invalid. Builders will have to manually edit their profiles to make changes. 1.x support is unaffected.Closes #4926, #4990