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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Aug 8, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
266a1f1
[util-linux-libuuid] Change CMake target from LibUUID::LibUUID to lib…
samuel-emrys Jul 15, 2023
be051ac
add alias for LibUUID::LibUUID
jcar87 Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions recipes/util-linux-libuuid/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ def package(self):

def package_info(self):
self.cpp_info.set_property("pkg_config_name", "uuid")
self.cpp_info.set_property("cmake_target_name", "LibUUID::LibUUID")
self.cpp_info.set_property("cmake_file_name", "LibUUID")
self.cpp_info.set_property("cmake_target_name", "libuuid::libuuid")
self.cpp_info.set_property("cmake_file_name", "libuuid")
Comment on lines +112 to +113
Copy link
Contributor

@SpaceIm SpaceIm Jul 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
self.cpp_info.set_property("cmake_target_name", "libuuid::libuuid")
self.cpp_info.set_property("cmake_file_name", "libuuid")

Where does it come from? Why not just keep recipename::recipename? It's conan-center convention when a library doesn't have any official config file or Find module file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This recipe provides the libuuid interface and replaces/deprecates the libuuid recipe. The failing libraries for this migration have already been patched for libuuid::libuuid, so this is the smallest change that would get everything on CCI functional.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not because it's the smallest change that it the good one. You can also use set_property() method of CMakeDeps in these recipes to override these names in order to satisfy these patches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Example if in downstream library there is a custom Findlibuuid.cmake and its CMakeLists.txt uses find_package(libuuid) and libuuid::libuuid target:

    def generate(self):
        deps = CMakeDeps(self)
        deps.set_property("util-linux-libuuid", "cmake_find_mode", "both")
        deps.set_property("util-linux-libuuid", "cmake_file_name", "libuuid")
        deps.set_property("util-linux-libuuid", "cmake_target_name", "libuuid::libuuid")
        deps.generate()

Copy link
Contributor Author

@samuel-emrys samuel-emrys Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't disagree. I put this up to push the issue forward but I'm not particularly invested in an individual solution. I'm not sure your suggestion for recipename::recipename is a good one here - we're talking about the target for an interface for which there is no standard. Ideally these three libuuid recipes would be completely substitutable (independently of how advisable that would be). recipename::recipename doesn't satisfy that. If anything along those lines, I think it should be provides::provides. Whether we change what's provided is another question I guess.

At the moment the recipe has provides = "libuuid", so I think libuuid::libuuid makes the most sense. I think there's probably also an argument for provides = "uuid" and uuid::uuid

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 clarity, the three libraries that I see implementing this interface are:

See #17427 (comment) for discussion of each. The original implementation is uuid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example if in downstream library there is a custom Findlibuuid.cmake and its CMakeLists.txt uses find_package(libuuid) and libuuid::libuuid target:

    def generate(self):
        deps = CMakeDeps(self)
        deps.set_property("util-linux-libuuid", "cmake_find_mode", "both")
        deps.set_property("util-linux-libuuid", "cmake_file_name", "libuuid")
        deps.set_property("util-linux-libuuid", "cmake_target_name", "libuuid::libuuid")
        deps.generate()

I like this way of patching/modifying the conan defaults to what individual recipes require - i think it would be good to preference this over patching the build system files where possible. Having said that, given that we're talking about an implementation interface, as above, I think that the provides interface should be used instead of recipename, i.e.

-        deps.set_property("util-linux-libuuid", "cmake_find_mode", "both")
+        deps.set_property("uuid", "cmake_find_mode", "both") # or libuuid/LibUUID, whatever value `provides` has

Copy link
Contributor Author

@samuel-emrys samuel-emrys Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm coming around to using uuid as the provides value. It's the name of the lib that's built by both recipes, which i guess is what we're trying to capture in an interface name:

conan-center-index/recipes/util-linux-libuuid/all/conanfile.py

Line 114 in 60fa77f

self.cpp_info.libs = ["uuid"]

conan-center-index/recipes/libuuid/all/conanfile.py

Line 88 in 60fa77f

self.cpp_info.libs = ["uuid"]

so perhaps the target name should be uuid::uuid

# Maintain alias to `LibUUID::LibUUID` for previous version of the recipe
self.cpp_info.set_property("cmake_target_aliases", ["LibUUID::LibUUID"])

self.cpp_info.libs = ["uuid"]
self.cpp_info.includedirs.append(os.path.join("include", "uuid"))
4 changes: 2 additions & 2 deletions recipes/util-linux-libuuid/all/test_package/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
cmake_minimum_required(VERSION 3.8)
project(test_package LANGUAGES C)

find_package(LibUUID REQUIRED CONFIG)
find_package(libuuid REQUIRED CONFIG)

add_executable(${PROJECT_NAME} test_package.c)
target_link_libraries(${PROJECT_NAME} PRIVATE LibUUID::LibUUID)
target_link_libraries(${PROJECT_NAME} PRIVATE libuuid::libuuid)
target_compile_features(${PROJECT_NAME} PRIVATE c_std_99)