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 cmakedeps systemlibs visibility #13364

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Mar 7, 2023

Changelog: Bugfix: Do not omit system_libs from dependencies even if they are header-only.
Docs: Omit

Close #13358

@@ -322,7 +322,6 @@ def join_defines(values, prefix=""):
# self.lib_paths = "" IMPORTANT! LINKERS IN LINUX FOR SHARED MIGHT NEED IT EVEN IF
# NOT REALLY LINKING LIB
self.libs = ""
self.system_libs = ""
self.frameworks = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Are frameworks omitted as well? Because they shouldn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with frameworks is that there is no distinction between frameworks provided by the package and system frameworks. We cannot propagated and link package frameworks if the trait tell us we shouldn't, otherwise we would have incorrect linking. How many system frameworks are defined in conan-center recipes?

Copy link
Contributor

@SpaceIm SpaceIm Mar 7, 2023

Choose a reason for hiding this comment

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

There are only system frameworks (I mean in conancenter). Many recipes have frameworks.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the meantime, it's quite obvious that cpp_info.frameworks are system frameworks in case of header-library.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add traits for frameworks as well (e.g. frameworks and transitive_frameworks)?
CCI at the moment only uses system frameworks, but private projects actually package frameworks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary, the logic to propagate frameworks is the same as for libraries.

We are trapped in a bad situation, CC recipes have been using frameworks for system frameworks, while private projects have been using it also for package frameworks. There is no possible logic that will make this work without breaking something, so what we need to think now is a path forward, how to unlock this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a system_frameworks attribute in cpp_info? I thought CCI recipes (and probably many private recipes) were using frameworks as "system_frameworks" because there was no alternative.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we're trapped, we can add new attributes (e.g. system_frameworks and custom_frameworks) and slowly deprecate old ones (frameworks).
btw, frameworks are not exactly as libraries - they are like libraries + headers packaged together into directory. so their traits policy ideally should be a combination of headers/transitive_headers and libraries/transitive_libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are trapped, because in the current propagation we cannot differentiate. We cannot always propagate frameworks, because that will do incorrect linking with package frameworks, and we cannot not always propagate frameworks, because system frameworks will not be there and fail to link. We would need to introduce new system_frameworks and migrate all ConanCenter recipes to make this work, but this will also be challenging and even breaking in 1.X, it will be difficult to backport this without breaking.

Copy link
Contributor

@SpaceIm SpaceIm Mar 8, 2023

Choose a reason for hiding this comment

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

Assuming system_frameworks is not backported to 1.X, recipes can still rely on conan.conan_version for these cases. A quick grep in conan-center shows 84 unique recipes with system frameworks in their package_info().

@memsharded memsharded merged commit 3fce209 into conan-io:release/2.0 Mar 13, 2023
@memsharded memsharded deleted the fix/cmakedeps_systemlibs_visibility branch March 13, 2023 15:45
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.

[bug] conan v2: cpp_info.system_libs ignored if package_type is header-library
4 participants