conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.14k stars 970 forks source link

[feature] Handle library loops (--start-group, --end-group) #6530

Open shellshocked2003 opened 4 years ago

shellshocked2003 commented 4 years ago

It seems that going from v1.21 to v1.22, the library ordering specified in self.cpp_info.libs is no longer maintained. Is this true or am I missing something?

The package I'm creating depends on Intel MKL, which requires a specific linking ordering as well as -Wl,--start-group and -Wl,--end-group surrounding the MKL libraries being linked to. I have the ordering specified in a Bintray package for MKL which was working fine for v1.21. When updating to v1.22, the ordering specified for the libraries in the conanfile.py for MKL is no longer maintained, so linking fails.

Is there a way to force conan to maintain the ordering or what is the best way to deal with this? Seems like this issue is related to #6510.

jgsogo commented 4 years ago

Hi! AFAIK, the order of the libraries in the cpp_info.libs list is not modified until they arrive to the generator, so this issue could be related to changes in the generators.

In v1.22 we modified the CMake generator (those using targets) to fix some issues related to the link order and the dependencies between CMake targets. This is the PR https://github.com/conan-io/conan/pull/6298. It is true that we are not considering the --start-group, --end-group in the list of libraries.

Which generator were you using when you came across this issue?


Issue #6510 is about the build-order between Conan packages computed from graphlock, it sholdn't be related to the link-order of libraries within the same package

shellshocked2003 commented 4 years ago

Hi @jgsogo ! Thanks for the quick response! I'm using cmake as the generator and linking to the conan package as follows:

target_link_libraries(smelt_static CONAN_PKG::ipp-static CONAN_PKG::mkl-static)

This worked before, but now it seems to push the --start-group to just before the --end-group, so it no longer brackets the MKL libraries being linked to.

jgsogo commented 4 years ago

Hi!

This is totally related to the fix applied in #6298, it modified the way we are creating the targets in our CMake function conan_package_library_targets. The problem is that we expect in the cpp_info.libs just the list of libraries, without extra items or flags... we look for the item in the filesystem, if we find it then we add it as an imported target; if not, we consider it to be a system library. For sure, --start-group and --end-group are not found in the filesystem and they are added to the system libraries list which is appended to the end of the linker line.

There is nothing in Conan taking into account those command-line arguments, so we need to approach this issue like a feature request. I'm very sorry it has broken your build, but we can try to work on a workaround before the actual feature is ready.

The reason people use those keywords is that there is a circular dependency between the libraries, those keywords are only a convenient way to avoid repeating the libraries in the linker line, so we can try a few things:

  1. Workaround: does it work if you take care inside the recipe of the link loop? I mean, you can repeat the libraries as needed:
    self.cpp_info.libs = ["mkl_intel_lp64", "mkl_sequential", "mkl_intel_lp64", "mkl_core"]  # I don't know where is the loop, probably it requires more repeated libraries
  2. Feature: provide a way to express the link loop in the cpp_info component of the Conan recipe:
    • Implementation for CMake: we can have a look at the LINK_INTERFACE_MULTIPLICITY variable... and add it to Conan's cpp_info somehow
    • Other build systems, they probably need the --start-group/--end-group in the linker
shellshocked2003 commented 4 years ago

Okay, thanks for the help! The workaround we're using is not pretty, but here it is:

self.cpp_info.exelinkflags = ["-Wl,--start-group {0}/lib/libmkl_intel_lp64.a {0}/lib/libmkl_sequential.a\ {0}/lib/libmkl_core.a -Wl,--end-group".format(self.cpp_info.rootpath)]

You can see the full recipe here. Do I need to submit a formal feature request, or is this something that is already in the works?

jgsogo commented 4 years ago

Yes, that's a workaround for sure.... I'll change the title of this issue and it is enough 😉

I'm thinking about something like this (totally speculative):

def package_info(self):
    self.cpp_info.libs = ['libA', LibGroup('libB', 'libC'), ..., 'libH']

and, under the hood, Conan will do something with that LibGruop object and translate it into the proper flags, properties,... or whatever is needed depending on the generator.

And we also have to consider this in the scope of the components feature...

jgsogo commented 4 years ago

I'm having a look at some CMake modules for MKL and I'm seeing some crazy stuff (i.e. pytorch).

I have a doubt, this inter-dependency can be handled just between libraries, or CMake is able to handle it between targets too?

shellshocked2003 commented 4 years ago

Yeah, it gets pretty crazy. To be honest, I've been doing it somewhat ad-hoc--I'm not using all the functionality within MKL, so not trying to account for all the different inter-dependencies.

One way that I was thinking about dealing with this in a more structured way, was through adding different options to the MKL conanfile.py. This way, CMake will get only the libraries it needs and in the order it needs them without having to write all the gnarly FindMKL stuff.

jsteinhofff commented 3 years ago

Some time ago i came uppon this issue in my company and although i believe this is not a critical issue (there is a workaround using exelinkflags, you shouldnt have cyclic dependencies between your libraries anyway), i still think it is worth to have a look at this because:

So if conan would handle this situation gracefully, i think for the rare cases where it is a problem, it would really be an improvement (it is not the responsibility of conan to make developers aware of weaknesses in their SW architecture, right?).

But after having a first look in a bit detail i found that as usual this is much harder to implement than anticipated. So before actually changing code, i would like to ask for some guidance. (maybe @jgsogo ?)

Findings sofar:

Necessary steps for implementation:

Open points:

bosmacs commented 2 years ago

Is there a better workaround for this when using the CMakeDeps generator? I too am working with MKL, but using the approach laid out by @shellshocked2003 fails with packages that depend on MKL via CMakeDeps generator in 1.45; the link flags always end up on the link line before the libraries that depend on it, resulting in missing symbols.

Nekto89 commented 5 months ago

I've been able to fool the conan and get at least test_package working by creating additional symlinks. But I'm not sure that it won't break in some other place.

    def package(self):
        lib_suffix = "lib" if self.settings.os == "Windows" else "a"
        lib_prefix = "" if self.settings.os == "Windows" else "lib"
        debug_suffix = "d" if self.settings.os == "Windows" and self.settings.build_type == "Debug" else ""
        source_lib_folder = os.path.join(self.build_folder, "lib")
        target_lib_folder = os.path.join(self.package_folder, "lib")
        copy(self, f"{lib_prefix}mkl_intel_ilp64.{lib_suffix}", source_lib_folder, target_lib_folder)
        copy(self, f"{lib_prefix}mkl_core.{lib_suffix}", source_lib_folder, target_lib_folder)
        copy(self, f"{lib_prefix}mkl_tbb_thread{debug_suffix}.{lib_suffix}", source_lib_folder, target_lib_folder)
        source_include_folder = os.path.join(self.build_folder, "include")
        target_include_folder = os.path.join(self.package_folder, "include")
        copy(self, "*", source_include_folder, target_include_folder)
        source_license_folder = os.path.join(self.build_folder, "share", "doc", "mkl", "licensing")
        target_license_folder = os.path.join(self.package_folder, "licenses")
        copy(self, "*", source_license_folder, target_license_folder)

        #ugly workaround. conan doesn't support circular dependencies
        if self.settings.os == "Linux":
            os.symlink("libmkl_core.a", os.path.join(self.package_folder, "lib", "libmkl_core2.a"))
            os.symlink("libmkl_core.a", os.path.join(self.package_folder, "lib", "libmkl_core3.a"))
            os.symlink("libmkl_tbb_thread.a", os.path.join(self.package_folder, "lib", "libmkl_tbb_thread2.a"))

    def package_info(self):
        debug_suffix = "d" if self.settings.os == "Windows" and self.settings.build_type == "Debug" else ""
        self.cpp_info.libs = ["mkl_intel_ilp64", "mkl_core", f"mkl_tbb_thread{debug_suffix}"]
        if self.settings.os == "Linux":
            #ugly workaround. conan doesn't support circular dependencies
            self.cpp_info.libs.extend(["mkl_core2", "mkl_tbb_thread2", "mkl_core3"])
        self.cpp_info.set_property("cmake_file_name", "MKL")
        self.cpp_info.set_property("cmake_target_name", "MKL::MKL")
        self.cpp_info.defines.append("MKL_ILP64")
memsharded commented 5 months ago

Hi all,

I am revisiting this, trying to improve over this issue.

One of the main blockers that I am finding is that CMake doesn't have a native way to specify this. It might allow defining cycles in dependencies, but the cycle has to be known, it is not possible to explicitly define the linking group with -Wl,--start-group and -Wl,--end-group

I have seen some thread using

TARGET_LINK_LIBRARIES(main f -Wl,--start-group g1 g2 -Wl,--end-group h)

But this looks terrible. Someone knows what is the best native way to specify this to CMake?

Nekto89 commented 5 months ago

Hi all,

I am revisiting this, trying to improve over this issue.

One of the main blockers that I am finding is that CMake doesn't have a native way to specify this. It might allow defining cycles in dependencies, but the cycle has to be known, it is not possible to explicitly define the linking group with -Wl,--start-group and -Wl,--end-group

I have seen some thread using

TARGET_LINK_LIBRARIES(main f -Wl,--start-group g1 g2 -Wl,--end-group h)

But this looks terrible. Someone knows what is the best native way to specify this to CMake?

CMake 3.24+ https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:LINK_GROUP

memsharded commented 5 months ago

The disadvantage of LINK_GROUP is that it cannot be set on the libraries targets. It would be necessary to set it on the "consumer" side target_link_libraries?

Maybe we can set it to the target root_target_name, something like:

if("{{ '${' }}{{ pkg_name }}_LINK_GROUP{{ config_suffix }}}")
                target_link_libraries({{root_target_name}} "$<LINK_GROUP:RESCAN,{{ '${' }}{{ pkg_name }}_LINK_GROUP{{ config_suffix }}}>")
            endi()

And we could define it as a cpp_info.set_property("link_group", "lib1,lib2,lib3")?

to talk with @jcar87

jcar87 commented 5 months ago

If I recall correctly, if the target_link_libraries result in a cycle, CMake should handle them just fine by expressing the libraries twice in the link command - https://cmake.org/cmake/help/latest/command/target_link_libraries.html#cyclic-dependencies-of-static-libraries If twice is not enough, one can use a target property: https://cmake.org/cmake/help/latest/prop_tgt/LINK_INTERFACE_MULTIPLICITY.html#prop_tgt:LINK_INTERFACE_MULTIPLICITY

But from what can I see @memsharded, we are still limited by the targets in CMakeDeps:

So this feature wouldn't work.

Out of curiosity, presumably this is only a problem for libraries within a package, where a package has multiple components that are all static libraries which present a dependency cycle? If this is the case, regardless of the implementation details (which we can probably handle some way or the other), this probably calls for a specific syntax in the package_info() method

memsharded commented 5 months ago

If I recall correctly, if the target_link_libraries result in a cycle, CMake should handle them just fine by expressing the libraries twice in the link command -

Problem is that we don't have a mechanism to do cycles of target_link_libraries:

So not only the targets types, also the model would be lacking here. I was thinking in the line of cpp_info.link_group or something like that.

X1aomu commented 1 month ago

Hi, @memsharded

I was thinking in the line of cpp_info.link_group or something like that.

I'm also in favour of that. Image a situation there is a library providing 2 library files, link foo.lib and foo_extra.lib in Release mode, while food.lib and foo_extrad.lib in Debug mode. cpp_info.libs = collect_libs(self) could work well for MSVC. However, it doesn't guaranteed to work in the Unix world, because the linking order of foo and foo_extra is not sure.

Now I have to write all libs' name, specifying order and dealing with the d suffix, manually. I hope a new model to make our life easier. :)