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

- [kmod] add 29 #9488

Merged
merged 7 commits into from
Mar 14, 2022
Merged

- [kmod] add 29 #9488

merged 7 commits into from
Mar 14, 2022

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Feb 23, 2022

for: #8555
closes: #8557

Specify library name and version: kmod/29

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!


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

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Signed-off-by: SSE4 <[email protected]>
@conan-center-bot

This comment has been minimized.

tc = AutotoolsDeps(self)
tc.environment.define("PKG_CONFIG_PATH", self.source_folder) # is it really needed?
tc.environment.define("LIBS", "-lpthread")
tc.environment.define("libzstd_LIBS", "-lzstd")
Copy link
Contributor Author

@SSE4 SSE4 Feb 23, 2022

Choose a reason for hiding this comment

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

some workarounds for conan-io/conan#10640 & conan-io/conan#10341
in 1.44, libzstd.pc recursively references itself:

$ cat /home/conan/.conan/data/kmod/29/_/_/build/e7842461bbcd29441fdb340a5299c019eae366ea/libzstd.pc
Name: libzstd
Description: Conan package: libzstd
Version: 1.5.2
Requires: libzstd

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 a TODO or something similar in the recipe source itself, so we know that these lines should be removed after some bug is fixed in Conan.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I have added, just in case

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

recipes/kmod/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Comment on lines 62 to 66
tc = AutotoolsDeps(self)
tc.environment.define("PKG_CONFIG_PATH", self.source_folder) # is it really needed?
tc.environment.define("LIBS", "-lpthread") # FIXME: https://github.com/conan-io/conan/issues/10640 & https://github.com/conan-io/conan/issues/10341
tc.environment.define("libzstd_LIBS", "-lzstd")
tc.generate()
Copy link
Contributor

@SpaceIm SpaceIm Feb 23, 2022

Choose a reason for hiding this comment

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

So except for zstd issue in its pc file, is AutotoolsDeps needed since all dependencies are discovered by PKG_CHECK_MODULES?

https://github.com/kmod-project/kmod/blob/v29/configure.ac#L86-L128

Why do we need to manually inject PKG_CONFIG_PATH, while CMakeToolchain handles CMAKE_MODULE_PATH/CMAKE_PREFIX_PATH out of the box? Seems to be a lack of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see conan-io/conan#10639, we need AutotoolsDeps to set PKG_CONFIG_PATH, as well as other variables to workaround conan bugs (in particular, conan-io/conan#10640 & conan-io/conan#10341)
at the moment of writing a recipe, it wasn't decided who and how (automatically or manually) will set PKG_CONFIG_PATH.

tools.get(**self.conan_data["sources"][self.version], destination=self._source_subfolder, strip_root=True)

def build(self):
self.run(os.path.join(self._source_subfolder, "autogen.sh"))
Copy link
Contributor

@SpaceIm SpaceIm Feb 23, 2022

Choose a reason for hiding this comment

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

Suggested change
self.run(os.path.join(self._source_subfolder, "autogen.sh"))
self.run(os.path.join(self._source_subfolder, "autogen.sh"))

libtool & pkgconf should be added to build requirements. autogen.sh call autoreconf... but also gtkdocize which may not be available.

https://github.com/kmod-project/kmod/blob/v29/autogen.sh#L9-L10

Why not just call autoreconf manually to have full control of what happen, like we do usually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in their readme, they say to launch autogen.sh, I just followed their build instructions.
gtkdocize seems to be fully optional (I don't build any docs).
libtool/pkgconf seems to be already available in docker images, but we can add explicit build_requirements for the completeness, as it's harmless (and will benefit users who will build on pure environment).

Copy link
Contributor

@SpaceIm SpaceIm Feb 23, 2022

Choose a reason for hiding this comment

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

If you run autogen.sh, gtkdocize is not optional anymore. My advice is to never run any autogen.sh script, regardless of upstream readme, but ensure to run our own autoreconf. libtool & pkgconf must be added to build requirements because you don't know whether folks have libtool or pkg-config installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I can see, it's fully optional, e.g. autogen.sh prints:

source_subfolder/autogen.sh: 9: gtkdocize: not found

and then build continues and finishes successfully.
never mind, I've added explicit call to autoreconf

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've also added libtool/pkgconf for completeness. it's the old good discussion on what to add to the build_requires, and what to assume as a ground base (or add to the profile, but not to the recipe). e.g. we assume that cmake should be already available on the developer's system, as well as binutils and coreutils, and the compiler itself, of course. it really only depends on where do you want to mark a border, and I am personally fine with libtool/pkgconf as build requirements.

@SSE4 SSE4 requested a review from jgsogo February 23, 2022 18:13
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

there are too many FIXMEs in my opinion, but we need to start using these new generators... so let's see how it goes as we switch to new Conan versions

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 2 (9fd5592a8322ef8774d83d9dfda59fa2887680d9):

  • kmod/29@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit fc53995 into conan-io:master Mar 14, 2022
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.

6 participants