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

rocksdb: ignoring thirdparty.inc. Modernized. #26009

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

franramirez688
Copy link
Contributor

ORIGINAL PR: #23090 ( I just added a commit to modernize the recipe but can not push directly to the original branch. Keeping the original author's commits anyway)

Summary

Changes to recipe: rocksdb/x.x.x

  • Modernized recipe.
  • Ignoring the thirdparty.inc file. rocksdb/6.29.5 needs a patch but the newer ones have a global CMake option ROCKSDB_SKIP_THIRDPARTY to handle that.

@thejohnfreeman
Copy link
Contributor

Thank you for working on this! I see you changed required_conan_version to ">=2.0". Was that necessary or just recommended? My team is still largely using Conan 1.x. I think we are ready to switch to 2.x, but would love to leave the 1.x option open if possible. For now, we keep a recipe for RocksDB, with the patch, in our own project, but I would love to get rid of that in favor of the version in Conan Center.

@RazielXYZ
Copy link
Contributor

RazielXYZ commented Nov 22, 2024

@thejohnfreeman Conan Center is no longer accepting packages/recipe updates/anything for Conan 1.x, so even if the 1.x stuff is left in the recipe and the version required is left alone, it won't propagate to the 1.x CC

@RazielXYZ
Copy link
Contributor

RazielXYZ commented Nov 22, 2024

@thejohnfreeman @franramirez688 if I'm reading rocksdb's (latest) CMakeLists right, setting ROCKSDB_SKIP_THIRDPARTY makes it so some of the third party libs (jemalloc, gflags, snappy, zlib, bz2, lz4 and zstd specifically) are not handled or included/linked on MSVC, only on the else() branch of the if(MSVC) check. The MSVC branch only handles those libs through thirdparty.inc. So I think the patch (or, I suppose, a slightly different patch) is actually necessary for versions that have ROCKSDB_SKIP_THIRDPARTY as well, if we want those to work with MSVC from conan.

Copy link
Contributor

@RazielXYZ RazielXYZ left a comment

Choose a reason for hiding this comment

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

Patch still needed even with ROCKSDB_SKIP_THIRDPARTY

@thejohnfreeman
Copy link
Contributor

thejohnfreeman commented Nov 22, 2024

That's correct. If ROCKSDB_SKIP_THIRDPARTY is passed, then no third-party dependencies are searched when compiling with MSVC. We don't want that. We don't want them skipped entirely, we just want them searched with find_package(), with the additional handling that non-MSVC compilers enjoy.

@franramirez688
Copy link
Contributor Author

@thejohnfreeman @RazielXYZ Thanks a lot for your comments! I did not realize about that, my bad 🙏
I'll be working on the new patch 😉 I'll keep you posted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants