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
CppInfo Components internal requirements #6871
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.
Some comments over the tests, I've been having a look mostly to the functionality, not the implementation
if not all([dep in components for dep in comp.requires]): | ||
raise ConanException("Component '%s' declares a missing dependency" % comp_name) | ||
# check if component is not required and can be added to ordered | ||
if comp_name not in [require for dep in components.values() for require in |
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.
Probably way simpler and faster to sort in the opposite order: check that the current components requires are None or all in ordered
.
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 implemented that logic before and it was a mess calculating the link order later. I'd like to keep it as is for now and improve the speed in the future if needed
Sort Components from most dependent one first to the less dependent one last | ||
:return: List of sorted components | ||
""" | ||
if not self._sorted_components: |
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.
When lazy/caching, this is better checked against None if self._sorted_components is None
, to avoid firing again computations when result is an empty dict, list or set.
conans/model/build_info.py
Outdated
@@ -206,6 +218,12 @@ def _raise_if_mixing_components(self): | |||
raise ConanException("self.cpp_info.components cannot be used with self.cpp_info configs" | |||
" (release/debug/...) at the same time") | |||
|
|||
# Raise on component name | |||
for comp_name in self.components: |
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.
Also as a oneliner: if any(comp_name == name for comp_name in self.components)
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'm missing a test here, but I don't know how to make it. Maybe you have an idea:
I need a package llvm
using components that require another package openssl
. I want to see that components from llvm
need to require openssl
explicitly in order to work (because it has to be explicit, right?):
OpenSSL recipe: any recipe not using components
LLVM recipe
class LLVM(ConanFile):
name = 'llvm'
requires = "openssl/1.1.1z"
def package_info(self):
self.cpp_info.components["tooling"].requires["openssl::openssl"]
# What happens with this line in this PR? By now it should work the same as the previous one? raise? ignore?
self.cpp_info.components["unwind"].requires["openssl::crypto"]
People will start to use components and we want them to add the .requires["openssl::openssl"]
so it will work in the next Conan iteration with the "enhanced-generators". Otherwise people will need to change their recipes again with the next version.
I really don't know if it is worth to check it in the sources (at least for CMake I imagine a weird if clause in the CMake file) or just write it big and clear in the docs.
You mean to force the definition of at least one It should be clearly documented that when defining components, it is not assumed a dependency to the required packages, and it should be explicit. |
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.
Easier than expected. Thanks for this extra effort \o/
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.
It seems that you were hit by your own check in another test: package_info_components_test()
Added docs! |
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.
🎁 Ready to go!
Changelog: Feature: Implements
cpp_info.components
dependencies.Docs: conan-io/docs#1682
#tags: slow
develop
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.