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

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

Conversation

samuel-emrys
Copy link
Contributor

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.

See discussion in original ticket adding util-linux-libuuid after merge: #17664 (comment)

The following tickets are blocked until this change is merged:

Closes #18554


…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
@conan-center-bot

This comment has been minimized.

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

@SpaceIm SpaceIm Jul 15, 2023

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

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

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:

self.cpp_info.libs = ["uuid"]

so perhaps the target name should be uuid::uuid

@samuel-emrys
Copy link
Contributor Author

For traceability, there is also relevant conversation about this in https://cpplang.slack.com/archives/C41CWV9HA/p1689414965702109

Copy link
Contributor

@gegles gegles left a comment

Choose a reason for hiding this comment

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

Hopefully we can merge this in quick so we can adjust our names internally and stabilize on this. thx!

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@gegles
Copy link
Contributor

gegles commented Jul 27, 2023

@samuel-emrys @SpaceIm could we make progress on this so as to finalize the target names so that every depending recipe can unify and stabilize around the final name. thx!

@samuel-emrys
Copy link
Contributor Author

@gegles this is waiting on input from a team member. @jcar87 has been assigned and @danimtb is pending review

@jcar87
Copy link
Contributor

jcar87 commented Aug 7, 2023

Hi @samuel-emrys - thank you for your contribution, and for the detailed analysis in #17427 (comment).

You may have been this already, but indeed it does appear to be the case that the libuuid implementation on most Linux distros is the one from this recipe (util-linux-libuuid) and not the one from Conan Center's libuuid recipe. I think aligning the implementation of libuuid across all of Conan Center with what the distros use is sensible, especially as it appears that https://sourceforge.net/projects/libuuid/ is no longer well maintained.

The conflicts with a system installed version are unfortunate, as it means that there are dependencies in potentially two places due to reliance on xorg/system (not that we can do much about it) - which makes me wonder whether a libuuid-system may be a better solution.

As for the CMake target name, this is rather unfortunate and caused by a defect in the libuuid recipe. When there is no known upstream name from a CMake-config, we fall back to well-known names from CMake built-in modules, and if none is available, we use the name of the library. So the libuuid recipe should have used LibUUID::LibUUID, much as this one currently does.

I can see that because of the Conan-Center-only libuuid::libuuid target, there are patches in a few places, for example:
https://github.com/conan-io/conan-center-index/blob/master/recipes/cppcommon/all/patches/0001-update-cmakelists-1-0-0-0.patch#L29-L30

I think I would be inclined to keep util-linux-libuuid having the case-sensitive LibUUID cmake package name, and LibUUID::LibUUID targets (as per CMake's upstream find module), but add a provides as you have mention.

As we update recipes to depend on util-linux-libuuid instead of of libuuid, we should be able to remove the workarounds that existed to support the names that were specific to the libuuid recipe, does this make sense?

@conan-center-bot

This comment has been minimized.

@jwillikers
Copy link
Contributor

@jcar87 Is it possible for us to have both a system and non-system recipe? I have some code that uses libuuid directly and I'd prefer to have it tied to a specific version of this library as opposed to the system one if possible, even if it would potentially conflict with the libuuid used by the xorg system package.

@samuel-emrys
Copy link
Contributor Author

samuel-emrys commented Aug 7, 2023

As we update recipes to depend on util-linux-libuuid instead of of libuuid, we should be able to remove the workarounds that existed to support the names that were specific to the libuuid recipe, does this make sense?

Yep, that makes sense. Before we move on, could you just confirm the following for me?

The conflicts with a system installed version are unfortunate, as it means that there are dependencies in potentially two places due to reliance on xorg/system (not that we can do much about it) - which makes me wonder whether a libuuid-system may be a better solution.

This feels a bit like giving up as a package manager - I think the solution that @prince-chrismc proposed here is a better one - introduce a dependency in the xorg recipe on the conan-center-index uuid package (this one). That way xorg won't conflict and will raise an error if someone tries to use another package in that graph.

As for the CMake target name, this is rather unfortunate and caused by a defect in the libuuid recipe. When there is no known upstream name from a CMake-config, we fall back to well-known names from CMake built-in modules, and if none is available, we use the name of the library. So the libuuid recipe should have used LibUUID::LibUUID, much as this one currently does.

Can you confirm that you're aware that LibUUID::LibUUID is an internal target that isn't propagated downstream to users, and is unlikely to become a public target? It's not clear to me that the same inheritence rules should apply, as discussed in the thread above. If you think this is the way to go regardless, that's cool, just want to make sure you're aware that this isn't a public target.

I think I would be inclined to keep util-linux-libuuid having the case-sensitive LibUUID cmake package name, and LibUUID::LibUUID targets (as per CMake's upstream find module), but add a provides as you have mention.

So, just to make sure I understand, all of the uuid recipes should have

class LibUUID(ConanFile):
    provides = "uuid"

and a cmake target of LibUUID::LibUUID? You don't think a general rule of defaulting to <provides>::<provides> in the case of packages that provide an implementation of the <provides> interface is the preferable way to go? So in this case it would be uuid::uuid. This is consistent with the linking rules (i.e. -luuid links to libuuid.so), and also the current naming convention of <packagename>::<packagename>, if you consider that any standalone package implicitly provides = <packagename>, so it seems like a natural choice for a default in the absence of an obvious, predefined public target imo.

@jcar87
Copy link
Contributor

jcar87 commented Aug 7, 2023

This feels a bit like giving up as a package manager - I think the solution that @prince-chrismc proposed here is a better one - introduce a dependency in the xorg recipe on the conan-center-index uuid package (this one). That way xorg won't conflict and will raise an error if someone tries to use another package in that graph.

To an extent, this could be valid as well. However this would be an artificial dependency - libuuid is not a direct dependency of xorg/system - and the dependency on the specific "new" libuuid may be distro dependent as well - so recording a dependency on util-linux-libuuid may be OK for some distros or some versions, but not others, and may not be valid in the future of the transitive dependency ceases to be in the future. We would also be recording a dependency on a Conan package that is not used by that package itself in any way, shape, or form - we still we a libuuid.so from the system. So this is not without risks - I can see how adding that dependency in xorg/system would have resulted in an error that would have failed fast instead of letting this get to the linking stage - but I would only advocate recording the dependency if we could have stricter guarantees on a version-tagged version of libuuid, as well as specific version - but we cannot even guarantee (over time), that it is a dependency at all. The most robust solution here would be to make sure that there exists a libuuid/system or similar, and have the ability to ensure that the entire dependency graph can depend on that. This ensures only 1 is used, and for a given system, that it is always the same version.

Can you confirm that you're aware that LibUUID::LibUUID is an internal target that isn't propagated downstream to users, and is unlikely to become a public target? It's not clear to me that the same inheritence rules should apply, as discussed in the thread above. If you think this is the way to go regardless, that's cool, just want to make sure you're aware that this isn't a public target.

I'm unsure what you are referring to as "public" target. If it is a target defined in a package_info as the way to consume the library, it is a public target name, in the sense that is the name of the target that consumers need to express in their CMakeLists.txt. If we make this change, then consumers need to change the target name they use themselves (if they do so in CMakeLists). I think that captures the public-ness.

When users consume any of the 10 recipes that depend on libuuid (I've posted the list here: #19084 (comment)) - then the name of the target is less relevant to them unless they depend on libuuid themselves. In this case, however, having noticed that some libraries had patches to use all-lowercase libuuid::libuuid (that existed only in Conan Center), instead of LibUUID::LibUUID (which uses the public name of the cmake find module) - we may as well take the opportunity to correct that situation, and follow the policy that we have been following in Conan Center more recently, which is the following:
A) If the library authors provide a CMake config file - use the name of the package and target to match this, else:
B) If there exists a CMake find module that is widely used - use these names, else:
C) use recipename::recipename

I think there is a strong case here that this is the (B) situation and that the libuuid should have used that convention as well. This should hopefully result in less things to patch and reduce the complexity of consumer libraries overall - assuming that they are more likely to rely on the built-in CMake module anyway.

@samuel-emrys
Copy link
Contributor Author

samuel-emrys commented Aug 7, 2023

The most robust solution here would be to make sure that there exists a libuuid/system or similar, and have the ability to ensure that the entire dependency graph can depend on that. This ensures only 1 is used, and for a given system, that it is always the same version.

I'm not sure this is the most robust solution. I see other problems with it. What if both a source version of uuid exists in the dependency tree along with the system package? This would create material issues for anybody who actually cares to link to uuid directly - it seems like this is @jwillikers use case.

The most robust solution imo would be to package xorg in conan. A big task, for sure, and I'm not necessarily proposing it - I don't have a good understanding of the limitations in doing so. I agree that the faux dependency solution has issues. I think practically though, as long as all linux distro's are using util-linux-libuuid, most of the potential issues are likely to be unrealised.

I think there is a strong case here that this is the (B) situation and that the libuuid should have used that convention as well. This should hopefully result in less things to patch and reduce the complexity of consumer libraries overall - assuming that they are more likely to rely on the built-in CMake module anyway.

I see where there's been miscommunication. The discussion above was related to this statement:

which uses the public name of the cmake find module

LibUUID::LibUUID is not the public name, propagated by CMake. This is an internal find module that CMake itself uses, but does not make available publicly - so it's not really (B).

This Find module is unofficial - not a module for public consumption. See @SpaceIm's review and the kitware discussion

I think the options you have are good:

A) If the library authors provide a CMake config file - use the name of the package and target to match this, else:
B) If there exists a CMake find module that is widely used - use these names, else:
C) use recipename::recipename

But I think (C) can be abstracted to provides::provides instead of recipename::recipename. lets use libjpeg and jpeg-turbo to illustrate - they both provide jpeg, that is, they both contain provides = "jpeg". I'm not sure what the specific circumstances surrounding this interface is - if theres a public CMake find module that already exists for it, then the target name for that should be used (i.e., (B) above). but, if it doesn't exist, then using your definition of (C) would lead to different targets depending on the library, i.e.:

  1. libjpeg::libjpeg for libjpeg
  2. jpeg-turbo::jpeg-turbo for jpeg-turbo

What i'm proposing is that both of these should use jpeg::jpeg, which would make each library completely interchangeable for the other, from a build system perspective. To switch between the two, the only thing an author would have to do is change the self.requires("...") statement to point at the library of choice.

The way this applies to libuuid is that if libuuid, util-linux-libuuid and a hypothetical uuid (if a package gets created) all provide the uuid interface, i.e. have provides = "uuid", then the cmake target they should present is uuid::uuid (again, notwithstanding the existence of (A) or (B) above). This would mean that from a build perspective, you could make these libraries completely substitutible by just changing the implementation you refer to in self.requires().

To be clear, all would share the same CMake code:

find_package(uuid REQUIRED)
target_link_libraries(mylib uuid::uuid)

Does that make sense? Again, no problems if you don't like this approach - just want to make sure I'm getting my point across before moving on.

@jcar87
Copy link
Contributor

jcar87 commented Aug 7, 2023

i see what you mean re: FindLibUUID.cmake - if this is internal to CMake then yes, any name is just as good or as bad as any. In that case, indeed I'd much rather it had the same name as the other libuuid and be libuuid::libuuid as this PR proposes.

Happy to proceed with this PR, but out of abundance of caution I would also keep LibUUID::LibUUID as an alias in the interim, to avoid breaking well-intentioned users that are already relying on that name (assuming they have used it directly) - otherwise they will experience disruption when they update the recipe revision.

I agree that it should be sensible for "provides" to share the same target name for transparent drop-in replacement. I'm not sure libjpeg and jpeg-turbo are the most perfect example of this, since jpeg-turbo is not a drop-in replacement in all cases (v7 and v8 emulation is opt-in, jpeg-turbo is not compatible with libjpeg v9 features), and jpeg-turbo also has its own API that is not compatible with libjpeg (should one choose to use that).

I'm not sure this is the most robust solution. I see other problems with it. What if both a source version of uuid exists in the dependency tree along with the system package? This would create material issues for anybody who actually cares to link to uuid directly - it seems like this is @jwillikers use case.

A system libuuid would also need a provides, and would also need to be incompatible with the other versions. A consumer can link against the system uuid coming from a libuuid conan packages that acts as proxy for the system one. The incompatibility would be there on purpose to avoid having different ones.

The most robust solution imo would be to package xorg in conan. A big task, for sure, and I'm not necessarily proposing it - I don't have a good understanding of the limitations in doing so. I agree that the faux dependency solution has issues. I think practically though, as long as all linux distro's are using util-linux-libuuid, most of the potential issues are likely to be unrealised.

I was not involved when the xorg/system approach was considered - but I can see why such an approach would be applicable. For build purposes (compiling and linking), it would be perfectly fine to have a dedicated xorg package that has all the needed components. It's an runtime on desktop Linux where an X server is running, where we could perhaps have issues if the instance of the X server cannot interface correctly with the runtime of the application (for example, an X11 runtime that is newer than the one running on the system). Those problems don't disappear with the current solution, but the Docker images used by Conan Center are kept "old enough" on purpose such that on newer systems things work due to backwards compatibility - but I assume that if we were relying on xorg/system on a too recent Linux distro, libraries that link against it may not work properly on distros with older versions. It's worth revisiting this if this is not the case - but I think building the x11 runtime or statically linking against it is not something that is usually done.

@samuel-emrys
Copy link
Contributor Author

I agree that it should be sensible for "provides" to share the same target name for transparent drop-in replacement. I'm not sure libjpeg and jpeg-turbo are the most perfect example of this, since jpeg-turbo is not a drop-in replacement in all cases (v7 and v8 emulation is opt-in, jpeg-turbo is not compatible with libjpeg v9 features), and jpeg-turbo also has its own API that is not compatible with libjpeg (should one choose to use that).

fair comment - I'm not particularly familiar with either. Glad to see this is moving forward.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 3 (be051acb74b3d2a90233ba732cacbe62bc45ab28):

  • util-linux-libuuid/2.39@:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 3 (be051acb74b3d2a90233ba732cacbe62bc45ab28):

  • util-linux-libuuid/2.39@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 4a0d5f0 into conan-io:master Aug 8, 2023
5 checks passed
ericLemanissier pushed a commit to ericLemanissier/conan-center-index that referenced this pull request 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[package] util-linux-libuuid uses wrong cmake target
8 participants