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

libcap/2.65 + conan v2 compatibility #12241

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

bobrofon
Copy link
Contributor

Specify library name and version: libcap/2.65

libcap was updated to the latest release. And a few warnings from conan-center hooks were fixed.
Also, as I see it, there is ongoing refactoring in the repo. A lot of packages were rewritten to be compatible with conan v2. So I did a little refactoring of the recipe too (have a few concerns though).


  • 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.

WARN: [INVALID TOPICS (KB-H064)]
The topic 'conan' is invalid and should be removed from topics
attribute.
WARN: [TOOLS CROSS BUILDING (KB-H062)]
The 'tools.cross_building(self.settings)' syntax may not work
correctly in some scenarios. Consider using
tools.cross_building(self).
libcap natively supports .pc files. And a minimal test package should
check that at least the pkg_config generator works correctly (because
this is how other packages typically work with libcap).
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.


def package_info(self):
self.cpp_info.components["cap"].set_property("pkg_config_name",
Copy link
Contributor Author

@bobrofon bobrofon Aug 15, 2022

Choose a reason for hiding this comment

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

When I'm trying to build a test package with this line, I get a warning: "(test package): WARN: [libcap/2.65] The PC package name libcap.pc already exists and it matches with another component one. Please, review all the component's pkg_config_name defined. Skipping it!". But I don't know how to figure out what's wrong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's because there's a global one by default for v1 generators.

You need to set a root level one like AFAIK (not an expert just say that in another PR I reviewed recently)

self.cpp_info.set_property("pkg_config_name", "libcap-all-do-not-use")


def generate(self):
env = Environment()
env.prepend_path("PKG_CONFIG_PATH", self.generators_folder)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, pkg_check_modules tries to find .pc files not in the generator_folder, but in the generator_folder/lib/pkgconfig. I thought the PkgConfigDeps generator should resolve that (via env vars maybe), but it doesn't. Not sure if I am doing something wrong or if it is a bug. I didn't find any examples of using CMakeToolchain and PkgConfigDeps generators together.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @franramirez688 @czoido is this the right way to use CMakeToolchain and PkgConfigDeps generators together?

Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, pkg_check_modules tries to find .pc files not in the generator_folder, but in the generator_folder/lib/pkgconfig.

After digging deeper into it, I figured out that CMake is adding the suffix lib/pkgconfig to the CMAKE_PREFIX_PATH (pointing to the package_folder where the *.pc files are created). You can have a look at the issue https://gitlab.kitware.com/cmake/cmake/-/issues/18150

I thought the PkgConfigDeps generator should resolve that (via env vars maybe), but it doesn't

Yes, you're right, but it should not be necessary as far as CMake should use that CMAKE_PREFIX_PATH by default. As I said above, CMake is adding that suffix, so it's getting failed because there are no PC files there.

is this the right way to use CMakeToolchain and PkgConfigDeps generators together?

Given that information, I tend to say that yes, it could be one way to solve that issue, and I think there could be another workaround like:

    def build(self):
        pkg_lib_folder = os.path.join(self.generators_folder, "lib", "pkgconfig")
        copy(self, "*.pc", self.generators_folder, pkg_lib_folder)
        cmake = CMake(self)
        cmake.configure()
        cmake.build()

Anyway, we could discuss if it could be a chance to create a mechanism to add the PKG_CONFIG_PATH via CMake build helper or even CMakeToolchain to avoid those workarounds.


required_conan_version = ">=1.33.0"
required_conan_version = ">=1.37.0"
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
required_conan_version = ">=1.37.0"
required_conan_version = ">=1.47.0"

@@ -26,82 +30,81 @@ class LibcapConan(ConanFile):
"psx_syscals": False,
}
exports_sources = "patches/**"
generators = "VirtualBuildEnv", "VirtualRunEnv"
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I was sure that the migrating guide recommends adding them, because they are enabled by default in v2. But I checked it, and it looks like I just mixed it up.
I will delete it.

Comment on lines 35 to 37
def validate(self):
if self.info.settings.os != "Linux":
raise ConanInvalidConfiguration("Only Linux supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

after configure please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup()
find_package(PkgConfig REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

please add pkgconf to build_requirements of test_package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@conan-center-bot
Copy link
Collaborator

All green in build 4 (e01ccdc9dd35b9e8fb74b6384ec9129318da3ce8):

  • libcap/2.62@:
    All packages built successfully! (All logs)

  • libcap/2.45@:
    All packages built successfully! (All logs)

  • libcap/2.58@:
    All packages built successfully! (All logs)

  • libcap/2.48@:
    All packages built successfully! (All logs)

  • libcap/2.57@:
    All packages built successfully! (All logs)

  • libcap/2.46@:
    All packages built successfully! (All logs)

  • libcap/2.65@:
    All packages built successfully! (All logs)

  • libcap/2.50@:
    All packages built successfully! (All logs)

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.

7 participants