-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Model system_libs in cpp_info/deps_cpp_info and generators #5582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is ok, but it needs to be more complete, to actually use that value in the generators. I'd like to see first how this value is going to be used by consumers.
So, this is in the same status @memsharded already commented right? |
Sorry that I didn't comment after the change. Yes, the changes were made. Previously system_deps was only added to the cpp_info model. Now a variable in the cmake generator is created and ready to be consumed |
But ready to be used how... Shouldn't we add them to the targets in right order and into the global CONAN_LIBS or am I missing something? |
And same for other generators, In my opinion, we cannot release this if it is not going to work out of the box. |
The system_deps libraries are appended to CONAN_LIBS in the cmake generator so it should work in the same way. This PR also includes a cmake variable called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing tests for:
- CMake target approach
- CMake find_package approach
- CMake find_package_multi approach
- Other generators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look a the comments, specially the one about a changed test and if it could be breaking.
conans/test/functional/generators/cmake_find_package_multi_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make this backwards compatible, and also consistent with what is done to the global aggregated variables. If CONAN_LIBS is showing package libs + system libs + frameworks, then CONAN_LIBS_MYPKG needs to show the same for MyPkg.
The way to be able to introduce the changes to the package variables is to define new ones as necessary:
- CONAN_PKG_LIBS_MYPKG: Contains only the package defined ones (MyPkg)
- CONAN_SYSTEM_LIBS_MYPKG: Contain only the system ones (of MyPkg)
- CONAN_FRAMEWORKS_MYPKG: Contain the frameworks (of MyPkg)
- CONAN_LIBS_MYPKG: Contains the addition of all (MyPkg). Final result should remain the same as in the past.
Changes are done as requested in the review. I have not added the frameworks variables to avoid making more noise |
Changelog: Feature: Add
system_deps
attribute for cpp_info and deps_cpp_info.Docs: conan-io/docs#1395
cppinfo
improvements: components, exe, system deps... #5090, closes System library dependencies could be specified in the wrong order #2382develop
branch, documenting this one.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.