Fix conan.tools.files.collect_libs() not handling files with multiple file extensions.

[JulianGro]

Libraries on Linux often have a version number behind their file extension. E.g. libnode.so.108. In our case, we are building libnode downstream and it automatically gets its version appended by its build system. I felt like changing the behaviour of collect_libs() to include files like that can save someone else some headache, since people might expect collect_libs() to just find the libraries regardless of them having weird names. In our case, it took us an entire week to figure this out, since it only caused visible errors at the end of our entire build process.

Changelog: (Fix): Fix conan.tools.files.collect_libs() not handling .so files with multiple file extensions, such as libnode.so.108.
Docs: https://github.com/conan-io/docs/pull/XXXX Todo

• Refer to the issue that supports this Pull Request.
• If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
• I've read the Contributing guide.
• I've followed the PEP8 style guides for Python code.
• I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

[CLAassistant]

All committers have signed the CLA.

[memsharded]

Hi @JulianGro

Thanks for your contribution!

Some quick feedback:

• In general it is much better to start first with a failing test that shows the use case and issue. From there then a solution can be investigated, reproducing the issue with a test is being half there
• In general, the collect_libs is not the most recommended approach. In fact, conan is moving more to explicit definition of package contents according the CPS specification effort (that we are participating it), and things like the .location of the produced artifact must be explicitly defined, as a result of the build, or at least explicitly defined in the Conan recipe, but not automatically deduced with something like collect_libs(), because that can only be a rough "guess" of the real package contents and it will always have issues and corner cases, it is impossible to have it. Not sure if the libnode package contains too many libraries, but defining self.cpp_info.libs = ["libnode"] would be preferred over collect_libs().

[JulianGro]

Thank you for your feedback @memsharded. I am a newbie when it comes to Conan, so it is highly appreciated.

(…) defining `self.cpp_info.libs = ["libnode"]` would be preferred over `collect_libs()`.

Problem with that approach is that cpp_info.libs apparently needs the full name of the library including the extension. At least it throws CMake Error at build/generators/cmakedeps_macros.cmake:66 (message): Library 'libnode' not found in package. (…) when consuming libnode::libnode without the file extension in cpp_info.libs.
In case of our downstream libnode, there is only libnode itself, but it has a different file extension based on the OS and version being built. So we would need three different cases for the three different OSes we support in addition to updating that part of the conanfile.py for every major version of libnode. This isn't a dealbreaker, but we would essentially be reimplementing collect_libs() in our conanfile.py.

Is self.cpp_info.libs = ["libnode"] supposed to map to libnode.so.108 and suffers from the same issue as collect_libs() or was that just an example?

Personally, I feel like finding libnode.so.108 automatically, would be preferable to hard-coding the file name, or trying to get libnode's build system to output libnode on every platform and version instead of outputting files like libnode.so.108.

[memsharded]

Problem with that approach is that cpp_info.libs apparently needs the full name of the library including the extension. At least it throws CMake Error at build/generators/cmakedeps_macros.cmake:66 (message): Library 'libnode' not found in package. (…) when consuming libnode::libnode without the file extension in cpp_info.libs.

That is unexpected. You can check in conan-center-index recipes, all of them use the barename without much issues. It is important that the self.cpp_info.libdirs/bindirs are correctly defined, then, internally in the CMakeDeps generator, the full name discovery is done by a CMake find_library() call. (Note: this has changed in the new incubating "CMakeDeps", which does the job in Python and allows the full location including relative path and full filename in the .location field, to comply with the CPS specification)

Is self.cpp_info.libs = ["libnode"] supposed to map to libnode.so.108 and suffers from the same issue as collect_libs() or was that just an example?

I am not fully sure, I think CMake find_library() might be able to do that. But maybe it relies on having the typical symlink libnode.so -> libnode.so.108, is this not the case for libnode? Isn't that libnode.so produced too?

[JulianGro]

It is important that the self.cpp_info.libdirs/bindirs are correctly defined (…)

As far as I can tell, both directories are correctly defined. self.cpp_info.libdirs is ['lib'] and libnode.so.108 is in lib/libnode.so.108 in the resulting Conan package.

Isn't that libnode.so produced too?

Not by libnode's build system at least. I looked at how Debian packages it, and they create the symlink in their own code later; Not sure if that matters though.

[memsharded]

Not by libnode's build system at least. I looked at how Debian packages it, and they create the symlink in their own code later; Not sure if that matters though.

That makes sense, as it conforms more to standard practices, it looks like the build system is lacking the creation of such symlinks, which are expected by other parts of the ecosystem.

I think it would make sense that the Conan conanfile.py recipe does the same in the libnode recipe, creating the expected symlinks in the package() method, so things look more standard to the consumers. In other words, this potential fix for collect_libs() is just a workaround for the Conan package_info() modeling, but there might be other scenarios, like for example deploying the shared libs for production outside of Conan that will find again the same issues, because package_info() will no longer be there to model it.

[JulianGro]

Sounds reasonable to me.
Main reason for me proposing this change is how long it took for us to debug this.
@memsharded do you think it would be reasonable for Conan to throw an error if collect_libs() is called, but no library could be found.
I think it should definitely throw a warning. It can be easy to miss such warnings, especially when other packages get build afterwards and stuff gets printed after it.

[memsharded]

Hi @JulianGro

Sorry for not following up earlier

do you think it would be reasonable for Conan to throw an error if collect_libs() is called, but no library could be found.

I think this might be breaking behavior, there might be users using collect_libs() in a base-python requires that applies to all packages, including header-only libraries that would fail otherwise. Or even calling it directly in their recipes for the common header_only option that allows users to select between a header-only or a compiled binary package.

I think it should definitely throw a warning. It can be easy to miss such warnings, especially when other packages get build afterwards and stuff gets printed after it.

I am fine with adding a warning, would you like to re-purpose this same PR to print that warning instead?

Thanks again for your contribution.