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

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

Open
shellshocked2003 opened this issue Feb 12, 2020 · 16 comments
Open

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

shellshocked2003 opened this issue Feb 12, 2020 · 16 comments

Comments

@shellshocked2003
Copy link

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 jgsogo self-assigned this Feb 12, 2020
@jgsogo
Copy link
Contributor

jgsogo commented Feb 12, 2020

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 #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
Copy link
Author

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
Copy link
Contributor

jgsogo commented Feb 13, 2020

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
Copy link
Author

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
Copy link
Contributor

jgsogo commented Feb 14, 2020

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 jgsogo changed the title [question] v1.22 no longer respects library ordering? [feature] Handle library loops (--start-group, --end-group) Feb 14, 2020
@jgsogo
Copy link
Contributor

jgsogo commented Feb 14, 2020

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
Copy link
Author

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
Copy link

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:

  • cyclic dependencies do exist in the wild
  • the issue with the order/grouping/repetition when linking static libraries is an inconvinience caused by the way the linker works, i dont like it show through in conan
  • the workaround breaks the great abstraction that conan provides, especially since components have been introduced, conan could know everthing required to solve the problem

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 ?)

  • My use-case is only Linux + CMake, so i cannot take care of other sytems like eg. Windows + Visual Studio, does it still make sense to try to contribute for Linux?
  • As far as i can tell from first investigation, local changes cannot solve this issue, instead bigger changes are required (see below), does it still make sense to try it?
  • What if architectural discussions are required?
  • Is "handling library loops gracefully" a breaking change? Is it plausible that conan users somehow rely on loops beeing detected and reported as exception?

Findings sofar:

  • libraries are handled only as list of strings, loop is detected in _get_sorted_components and reported as exception
    raise ConanException("There is a dependency loop in "

    => currently no way to 1. group things, 2. indicate that loops exist
  • in CMake, groups of libraries are not supported, rather LINK_INTERFACE_MULTIPLICITY is the "solution" (i would call it workaround, but thats CMake for you). To apply LINK_INTERFACE_MULTIPLICITY some preconditions have to be fulfilled ( see comments by Brad King in https://gitlab.kitware.com/cmake/cmake/-/issues/17964 ):
    • LINK_INTERFACE_MULTIPLICITY is not allowed on interface libaries
      (as which conan packages are added as CMake targets)
      (otherwise error: INTERFACE_LIBRARY targets may only have whitelisted properties. The property "LINK_INTERFACE_MULTIPLICITY" is not allowed.)
    • instead, LINK_INTERFACE_MULTIPLICITY has to be set directly on the STATIC libraries
    • but then again, conan does create UNKNOWN libraries, not STATIC libraries for all components
    • also the dependency between those STATIC libraries have to be modeled in CMake such that LINK_INTERFACE_MULTIPLICITY gets consulated at all

Necessary steps for implementation:

  • have a test for it (sofar only a unit test for the loop-detection exists afaik)
  • if component-libraries are static, create the CMake targets as STATIC correspondingly
    => need more than just a list of strings to transport this information to the CMake generator
  • model dependencies between components in CMake
    => also not feasible with the flat list currently used
  • set LINK_INTERFACE_MULTIPLICITY on those component targets as required
  • instead of providing component libraries just as a flat list, provide a more rich interface for generators
    • function to check library type static/dynamic
    • function to check if a loop exists
    • if not, flat list can be generated
    • if loop exists, provide worst-case LINK_INTERFACE_MULTIPLICITY
    • for each component, provide dependencies

Open points:

  • is effort required worth it? Are other topics related and should be considered at the same time?
  • can this be approached with intermediate (meaningful) steps?
  • what about other generators?
  • is using worst case LINK_INTERFACE_MULTIPLICITY acceptable? (requiring the conen recipe developer to provide this information feels like a leaky abstraction again to me)
  • (general question) why is the CMake generator doing so much work at CMake configuration time? (and not in conan cmake_generator python code?)

@bosmacs
Copy link

bosmacs commented Mar 3, 2022

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
Copy link
Contributor

Nekto89 commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Contributor

Nekto89 commented Apr 5, 2024

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
Copy link
Member

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

@memsharded memsharded added this to the 2.3.0 milestone Apr 7, 2024
@jcar87
Copy link
Contributor

jcar87 commented Apr 9, 2024

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:

  • the library targets are UNKNOWN, not STATIC
  • the visible targets are INTERFACE rather than library ones

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
Copy link
Member

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:

  • If cpp_info.libs is used, it is just a list of libs, but there is no info of relationships among them, so it is impossible to define target_link_libraries among the internal libraries of the package
  • If cpp_info.components are defined, this is a DAG, as detecting and managing cycles is very challenging, and it would introduce some further modeling problems. Many dependency managers assume dependencies are a DAG, without cycles. So it is not possible to define a loop of libraries via components.

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
Copy link

X1aomu commented Aug 16, 2024

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants