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

Fix link order issues for CMake generators (using targets) #6298

Merged
merged 52 commits into from Feb 3, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Jan 3, 2020

Changelog: Fix: Add all the system_libs and requirements to the CMake targets constructed by the generators. It will impact header-only libraries that are consumed using targets (previously they were missing some information).
Docs: omit

On top of #6291
Fixes #6280

[.... pending]

Supersedes #6281

#TAGS: slow

jgsogo added 30 commits Jan 2, 2020
test for link order

check link order for all generators

add tests

share in class
test for link order

check link order for all generators

add tests

share in class
@jgsogo jgsogo changed the title [WIP] Fix link order issues for CMake generators (using targets) Fix link order issues for CMake generators (using targets) Jan 7, 2020
@jgsogo jgsogo requested a review from czoido Jan 7, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Jan 8, 2020

This is the graph of libraries used in the tests (header-only libs are represented with a box):

image

czoido
czoido approved these changes Jan 9, 2020
@jgsogo jgsogo requested a review from memsharded Jan 9, 2020
@jgsogo jgsogo marked this pull request as ready for review Jan 9, 2020
Copy link
Member

@memsharded memsharded left a comment

Please rebase develop and solve conflicts.

Copy link
Member

@memsharded memsharded left a comment

I have finally understood this. It was challenging, I appreciate the good job here.
Besides the minor comments to check, I'd say to try to attach to one cmake style (lists or strings). So far it seems that the code is using strings, and converting to lists when necessary, so probably better to stick to that style (while you are doing the opposite).

However, that could be done later, this is important and should be in 1.22.

list(APPEND {name}_LIBRARIES_TARGETS{build_type_suffix} ${{_SYSTEM_LIB}})
list(APPEND {name}_LIBRARIES{build_type_suffix} ${{_SYSTEM_LIB}})
endforeach()
# We need to add our requirements too
set({name}_LIBRARIES_TARGETS{build_type_suffix} "${{{name}_LIBRARIES_TARGETS{build_type_suffix}}};{deps_names}")
Copy link
Member

@memsharded memsharded Feb 3, 2020

Choose a reason for hiding this comment

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

Not sure why dependencies are being added to this variable, it seems defined for self package things, not for the transitive dependencies values.

Copy link
Member Author

@jgsogo jgsogo Feb 3, 2020

Choose a reason for hiding this comment

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

Maybe this is a misunderstanding on my side. I'm adding the dependencies because we are already adding the system_libs two lines above... and the consumer (outside Conan, in the user CMakeLists.txt) will need these libraries too in order to link with all the things needed.

Copy link
Member

@memsharded memsharded Feb 3, 2020

Choose a reason for hiding this comment

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

I thought the purpose of that variable is to have the current package things, not the transitive ones, so if the user wants to independently use somehow different things will be using BOOST_LIBRARIES_TARGETS and ZLIB_LIBRARIES_TARGETS? This variable is really not documented in https://docs.conan.io/en/latest/reference/generators/cmake_find_package.html, so I guess not a big deal. The docs say that the targets are transitive, but doesn't say much about other variables.

@@ -430,7 +433,9 @@ def build(self):
client.run("build .")
self.assertIn('Found MYHELLO2: 1.0 (found version "1.0")', client.out)
self.assertIn('Found MYHELLO: 1.0 (found version "1.0")', client.out)
self.assertIn("Target libs: CONAN_LIB::MYHELLO2_hello;;;CONAN_LIB::MYHELLO_hello",
self.assertIn("Target libs (hello2): CONAN_LIB::MYHELLO2_hello;MYHELLO::MYHELLO;MYHELLO::MYHELLO;;",
Copy link
Member

@memsharded memsharded Feb 3, 2020

Choose a reason for hiding this comment

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

Repeated MYHELLO::MYHELLO, is this OK?

Copy link
Member

@memsharded memsharded Feb 3, 2020

Choose a reason for hiding this comment

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

Agree with both things. More logic should be done in the python side. Also, the above duplication, I am not concerned about the duplication itself in the output, but understanding where it comes from and that it is not an underlying bug

from conans.test.utils.tools import TestClient


class LinkOrderTest(unittest.TestCase):
Copy link
Member

@memsharded memsharded Feb 3, 2020

Choose a reason for hiding this comment

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

This test is challenging to understand, possibly hard to maintain.
But I understand it, probably the best is to leave it as-is

@memsharded memsharded added this to the 1.22 milestone Feb 3, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Feb 3, 2020

Ok, let me recap all the information related to your last comments: why the duplication and why appending the dependencies to {name}_LIBRARIES_TARGETS and {name}_LIBRARIES variables.

Why appending to those variables?

  • The variables {name}_LIBRARIES and {name}_LIBRARIES_TARGETS can be used from the consumer CMakeLists.txt, so it should contain the dependencies.
  • The dependencies are added to the targets that are created in the conan_package_library_targets, so the dependencies are added to each one of the targets defined in {name}_LIBRARIES_TARGETS, but
  • If it is a header-only library, we aren't generating any to target so the dependencies won't be added to the {name}_LIBRARIES_TARGETS variable. The only way to go is to add them unconditionally (they will be duplicated in the CMake graph) or to check and add if needed.

About the duplication:

It is an unfortunate consequence, but I think it can ve avoided. It is introduced due to changes in cmake_find_package.py (here):

set_property(TARGET {name}::{name} PROPERTY INTERFACE_LINK_LIBRARIES "${{{name}_LIBRARIES_TARGETS}};{deps_names};${{{name}_LINKER_FLAGS_LIST}}")

but, after adding the dependencies to the {name}_LIBRARIES_TARGETS it clearly makes no sense to add them again here. I'm pushing a commit, let's wait for the CI

@jgsogo jgsogo assigned memsharded and unassigned jgsogo Feb 3, 2020
@memsharded memsharded merged commit 9d990a6 into conan-io:develop Feb 3, 2020
2 checks passed
@jgsogo jgsogo deleted the fix/cmake-transitive-imported branch Mar 27, 2020
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.

3 participants