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] Add util-linux-libuuid package #17664

Merged

Conversation

samuel-emrys
Copy link
Contributor

@samuel-emrys samuel-emrys commented May 23, 2023

  • This is the most actively maintained fork of libuuid
  • This package should supersede the existing libuuid recipe, which should be deprecated as an inactive fork of the original libuuid. The existing libuuid recipe does not match what gets installed on linux systems, which causes a symbol conflict when a system package is used with libuuid in the dependency tree, such as xorg/system. See [tk] Migrate tk to conan 2.0 #17427 (comment) for a concrete example of this.

Closes #17337

Specify library name and version: libuuid/2.39


* This is the most actively maintained fork of libuuid
* This package should supersede the existing libuuid recipe, which
  should be deprecated as an inactive fork of the original libuuid.

Closes conan-io#17337
@conan-center-bot

This comment has been minimized.

@samuel-emrys samuel-emrys force-pushed the recipe/17337-util-linux-libuuid branch from 5a8cf99 to bea16f8 Compare May 24, 2023 15:06
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

…libintl

* libintl ships as part of glibc on linux and so this isn't required in
  that context. On Macos, this isn't the case so this has to be linked
  against explicitly
@conan-center-bot

This comment has been minimized.

@samuel-emrys
Copy link
Contributor Author

I haven't been able to replicate the error message on my mac locally:

ERROR: You must have libtool-2 installed to generate the util-linux build system.

If anybody has any suggestions on how to replicate this it would be appreciated.

@conan-center-bot

This comment has been minimized.

* Invalidate Macos builds as they are failing in a way that hasn't been
  able to be reproduced for debugging on the CI machine.
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericLemanissier ericLemanissier mentioned this pull request Jun 6, 2023
3 tasks
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 14 (c4a0fc851ee433c5a51879e4e2976152029444e4):

  • 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 14 (c4a0fc851ee433c5a51879e4e2976152029444e4):

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

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.

Thanks @samuel-emrys!

@ericLemanissier
Copy link
Contributor

@RubenRBS @memsharded @czoido @uilianries can one of you review this PR ? thanks!

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit f18fb90 into conan-io:master Jun 21, 2023
@jwillikers
Copy link
Contributor

@samuel-emrys Thanks for adding this!

ericLemanissier added a commit to ericLemanissier/conan-center-index that referenced this pull request Jun 30, 2023
to avoid conflict with system libuuid

see conan-io#17664 and conan-io#17427 (comment) for motivation and rationale
Comment on lines +131 to +132
self.cpp_info.set_property("cmake_target_name", "LibUUID::LibUUID")
self.cpp_info.set_property("cmake_file_name", "LibUUID")
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does it come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@SpaceIm SpaceIm Jul 1, 2023

Choose a reason for hiding this comment

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

It's an internal file of CMake source code, not some official FindLIBUUID.cmake file shipped in CMake program, so it shouldn't be used as a reference. This recipe shouldn't override cmake names since there is no official Find file (someone asked for it, but it will likely be rejected by CMake team: https://gitlab.kitware.com/cmake/cmake/-/issues/24607), and util-linux project doesn't provide any config 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.

Ah, I'd missed that detail. Knowing that a convention has been used by CMake though (even though it's internal), wouldn't that still be the sensible choice for Conan? Since util-linux provides no config file, I would imagine people will take what already exists. Seems like this might be a sensible convention to standardise on..

Comment on lines +85 to +90
self.tool_requires("libtool/2.4.7")
self.tool_requires("m4/1.4.19")
self.tool_requires("pkgconf/1.9.3")
self.tool_requires("bison/3.8.2")
self.tool_requires("autoconf/2.71")
self.tool_requires("automake/1.16.5")
Copy link
Contributor

Choose a reason for hiding this comment

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

Very bad idea to explicitly reference automake, autoconf and m4, it doesn't scale in build requirements, it's well known. Why this recipe doesn't follow other autoconf recipes guidelines?

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'm not experienced with automake. I used a few other recipes as guidelines but they obviously haven't incorporated the practices you're talking about and it didn't come up in review. It would be useful if those were documented somewhere easily discoverable. Feel free to submit a new PR with the necessary changes if that's a better way of managing these.

Copy link
Contributor

Choose a reason for hiding this comment

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

A good reference is autotools recipes template.

(sorry, I won't fix this, I don't have the time. It will break when someone will bump automake version in libtool recipe)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, but it wouldn't have addressed this - it has an explicit example requiring automake, and no discussion of when it shouldn't be required.

Copy link
Contributor

Choose a reason for hiding this comment

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

it has an explicit example requiring automake, and no discussion of when it shouldn't be required.

There is a comment explaining when it's required: https://github.com/conan-io/conan-center-index/blob/60fa77f44eafacf99b7f1d43650f13dd8db5d971/docs/package_templates/autotools_package/all/conanfile.py#L93C1-L96

pezy pushed a commit to pezy/conan-center-index that referenced this pull request Jul 15, 2023
* [util-linux-libuuid] Add util-linux-libuuid package

* This is the most actively maintained fork of libuuid
* This package should supersede the existing libuuid recipe, which
  should be deprecated as an inactive fork of the original libuuid.

Closes conan-io#17337

* [util-linux-libuuid] Add minimum compatible compiler versions

* [util-linux-libuuid] Add apple-clang to minimum compiler list

* [util-linux-libuuid] Add failover for min_version call to return 0 if compiler or build_type don't exist

* [util-linux-libuuid] Add dependency on gettext for Macos to bring in libintl

* libintl ships as part of glibc on linux and so this isn't required in
  that context. On Macos, this isn't the case so this has to be linked
  against explicitly

* [util-linux-libuuid] Use libgettext instead of gettext for libintl

* [util-linux-libuuid] Invalidate macos builds

* Invalidate Macos builds as they are failing in a way that hasn't been
  able to be reproduced for debugging on the CI machine.

* [util-linux-libuuid] Modify version to use util-linux version rather than libuuid

* Update CMake targets to match upstream

libuuid -> LibUUID

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>

* [util-linux-libuuid] Fix consumer find_package to match new target name

---------

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
@ericLemanissier ericLemanissier mentioned this pull request Sep 2, 2023
3 tasks
@samuel-emrys samuel-emrys mentioned this pull request Sep 3, 2023
3 tasks
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.

[request] util-linux-libuuid/2.38.1
9 participants