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

Add qdbm/1.8.78 #8676

Merged
merged 7 commits into from
Feb 2, 2022
Merged

Conversation

friendlyanon
Copy link
Contributor

Specify library name and version: qdbm/1.8.78

Issue: #8618


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@@ -0,0 +1,83 @@
cmake_minimum_required(VERSION 3.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain your motivation for writing your own CMake file instead of using the upstream build system?

Copy link
Contributor Author

@friendlyanon friendlyanon Jan 8, 2022

Choose a reason for hiding this comment

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

Sure, it's much easier to hook into the build from Conan if it's a CMake build instead of an Autotools gobbledygook. It also builds trivially across every platform.

Additionally, I rather spend a couple hours writing a CML that I can guarantee to work correctly than spend multiple days trying to untangle some Autotools mess that I hope works correctly. See the net-snmp recipe for an example where I'm not sure things work properly despite the amounts of bandaids I had to apply.

Copy link
Member

Choose a reason for hiding this comment

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

We avoid creating extra files for multiple reasons. First, maintenance, one file more requires more effort. Second, expected behavior, the autotools can be a mess, but it results a file with an expected behavior, if we provide cmake or any other file and forget any flag, it will result in a different artifact (libs, bin, ...).
There are some cases where we use autootools for gcc and clang, but Visual Studio we really need a custom cmake file, but it's an exception.
Said that, please, keep autotools as the first option, if you get stuck, you still can ask for help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd contest that by pointing out that QDBM hasn't seen an update since 2005 and CMake works better on all platforms, so there is nothing to maintain in that regard and the conanfile is much simpler and shorter than it would otherwise be, which makes it easier to maintain. On a second look, I will give you that maybe the .def file (which I took it from the PHP repository) file should be dropped, because there are already export/import attributes in the source code.

@Croydon
Copy link
Contributor

Croydon commented Jan 8, 2022

@jgsogo Same here as in #8668 (comment). Bug in Epochs?

@SSE4 SSE4 added the infrastructure Waiting on tools or services belonging to the infra label Jan 9, 2022
@SSE4
Copy link
Contributor

SSE4 commented Jan 9, 2022

@jgsogo Same here as in #8668 (comment). Bug in Epochs?

yes, not only there, seems like many more PRs are affected

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 10, 2022

@jgsogo Same here as in #8668 (comment). Bug in Epochs?

Indeed, now it should be fixed. We are triggering those PRs that failed.

@SSE4 SSE4 removed the infrastructure Waiting on tools or services belonging to the infra label Jan 10, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

jgsogo
jgsogo previously approved these changes Jan 31, 2022
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

IMHO, the CMakeLists file provided is simple enough and it probably won't require too much maintenance. If the project goes live again we can reevaluate this again and see if their re-bumped build-system better suits our needs. If not, better to add one simple file and have the library available than to have nothing at all.

Comment on lines 7 to 8
add_executable(test_package test_package.c)
target_link_libraries(test_package PRIVATE CONAN_PKG::qdbm)
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use cmake_find_package[_multi] generator, as it is more aligned with the future CMakeDeps in Conan v2.

Suggested change
add_executable(test_package test_package.c)
target_link_libraries(test_package PRIVATE CONAN_PKG::qdbm)
find_package(qdbm REQUIRED)
add_executable(test_package test_package.c)
target_link_libraries(test_package PRIVATE qdbm::qdbm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require adding "unofficial" CMake targets. Feedback from @SpaceIm was that this is not desirable.

Copy link
Contributor

@SpaceIm SpaceIm Jan 31, 2022

Choose a reason for hiding this comment

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

conan fallbacks to Find<recipeName>.cmake/<recipeName>Config.cmake and <recipeName>::<recipeName> (and <recipeName>::<componentName> for components) imported targets in these generators (cmake_find_package, cmake_find_package_multi & CMakeDeps).
What I suggest is to not enforce an unofficial name in package_info(), not even enforce the default values generated by conan (just leave conan generate default names for this recipe).
Indeed, it's very hard and time consuming to keep CCI recipes in good health regarding CMake names (specially because many contributors don't care to define proper names of upstream in the first place, or don't know how to deduce these names from a CMakeLists, which may not be obvious in obfuscated CMakeLists). Therefore I prefer to not see CMake names in package_info() when a library doesn't have official CMake Find/Config file.

But you can use cmake_find_package_multi in test package (there will be no other choice in conan v2 anyway, it will be CMakeToolchain + CMakeDeps, so no more CONAN_LIBS variable or CONAN_PKG::<recipeName> target). IMOO, it's not so important for the moment, but there is a consensus to always use cmake_find_package_multi in test package currently, so let's use it regardless of whether CMake names are explicitly set in package_info().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware. Thank you!

Comment on lines 7 to 8
settings = ("os", "compiler", "build_type", "arch")
generators = ("cmake",)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
settings = ("os", "compiler", "build_type", "arch")
generators = ("cmake",)
settings = ("os", "compiler", "build_type", "arch")
generators = "cmake", "cmake_find_package"

@conan-center-bot
Copy link
Collaborator

All green in build 7 (d94ebff2c8d26edc82087bafb09709d6de2f152f):

  • qdbm/1.8.78@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit a59a124 into conan-io:master Feb 2, 2022
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.

8 participants