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

[package] util-linux-libuuid uses wrong cmake target #18554

Closed
samuel-emrys opened this issue Jul 15, 2023 · 3 comments · Fixed by #18559
Closed

[package] util-linux-libuuid uses wrong cmake target #18554

samuel-emrys opened this issue Jul 15, 2023 · 3 comments · Fixed by #18559
Labels
bug Something isn't working

Comments

@samuel-emrys
Copy link
Contributor

Description

In the following lines, the util-linux-libuuid recipe sets the cmake target to be LibUUID::LibUUID with a filename of LibUUID-config.cmake:

self.cpp_info.set_property("cmake_target_name", "LibUUID::LibUUID")
self.cpp_info.set_property("cmake_file_name", "LibUUID")

This was based on the internal practice that Kitware has for their internal libuuid cmake module, however this is not public and a number of packages (czmq, cppcommon) seem to assume a libuuid::libuuid target. These change should be reverted such that these packages can utilise util-linux-libuuid without a requirement to be patched.

Package and Environment Details

N/A

Conan profile

N/A

Steps to reproduce

N/A

Logs

Click to expand log
Put your log output here
@samuel-emrys samuel-emrys added the bug Something isn't working label Jul 15, 2023
samuel-emrys added a commit to samuel-emrys/conan-center-index that referenced this issue Jul 15, 2023
…uuid::libuuid

* This is in line with what conan recipes seem to expect, which will
  minimise the number of patches required. LibUUID was chosen as the
  target name given this is what's used internally within cmake, but
  this isn't a public interface and so there is no common usage to take
  guidance from.

Closes conan-io#18554
@jcar87
Copy link
Contributor

jcar87 commented Aug 7, 2023

I've made a comment on the PR: #18559

I think there is an argument that libuuid::libuuid (all lower case) is internal to Conan Center and unlikely to be referenced outside of Conan Center - while LibUUID::LibUUID is part of a public CMake find module, and thus overwhelmingly more likely to be found out in the wild.
You can see for instance: https://github.com/search?q=libuuid%3A%3Alibuuid+language%3Acmake&type=code , where LibUUID::LibUUID is more common, and libuuid::libuuid is only used by project-private find modules.

Given that there are only 10 recipes that need to be updated to use util-linux-libuuid (#19084), I would probably ensure those use the LibUUID Cmake find_package / target names instead - in some cases this means we can remove patches and workarounds altogether.

@samuel-emrys
Copy link
Contributor Author

Good point. That's a good way to identify commonality. In contrast, looks like uuid::uuid or UUID::UUID is more common than that again: https://github.com/search?q=uuid%3A%3Auuid+language%3Acmake&type=code

@jcar87
Copy link
Contributor

jcar87 commented Aug 7, 2023

looks like uuid::uuid or UUID::UUID is more common than that again: https://github.com/search?q=uuid%3A%3Auuid+language%3Acmake&type=code

This may indeed be the case, however, I can see that in those instances, project-private find modules are used. The only find modules that we consider authoritative are the CMake built-in. Each may have different conventions. For specific cases where a project uses a custom name, we have the ability to tweak the target name in the generate() method.

conan-center-bot pushed a commit that referenced this issue Aug 8, 2023
…ID to libuuid::libuuid

* [util-linux-libuuid] Change CMake target from LibUUID::LibUUID to libuuid::libuuid

* This is in line with what conan recipes seem to expect, which will
  minimise the number of patches required. LibUUID was chosen as the
  target name given this is what's used internally within cmake, but
  this isn't a public interface and so there is no common usage to take
  guidance from.

Closes #18554

* add alias for LibUUID::LibUUID

---------

Co-authored-by: Luis Caro Campos <3535649+jcar87@users.noreply.github.com>
ericLemanissier pushed a commit to ericLemanissier/conan-center-index that referenced this issue Sep 15, 2023
…D::LibUUID to libuuid::libuuid

* [util-linux-libuuid] Change CMake target from LibUUID::LibUUID to libuuid::libuuid

* This is in line with what conan recipes seem to expect, which will
  minimise the number of patches required. LibUUID was chosen as the
  target name given this is what's used internally within cmake, but
  this isn't a public interface and so there is no common usage to take
  guidance from.

Closes conan-io#18554

* add alias for LibUUID::LibUUID

---------

Co-authored-by: Luis Caro Campos <3535649+jcar87@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants