-
Notifications
You must be signed in to change notification settings - Fork 981
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
Alternative way of setting properties that depend on generator specific features. #8727
Alternative way of setting properties that depend on generator specific features. #8727
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 like the idea of generalizing the information for generators, keep going
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.
This is looking very good, I like it.
…ge_cpp_info_syntax
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 points to consider and to discuss, I still need to understand a bit more the new migration proposal, now that it seems there is no longer an internal translation to properties.
import textwrap | ||
import os | ||
|
||
class custom_generator(GeneratorComponentsMixin, Generator): |
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 get rid of this inheritance too. New generators do not need to extend anything, just implement generate()
method. Components helpers should go elsewhere.
Not now, not in this PR, but in the future.
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.
A couple of minor stylistic things, but overall good job, I think this is ready to be merged
if result: | ||
return result | ||
return self.get_name(generator) | ||
|
||
# TODO: Deprecate for 2.0. Use get_property for 2.0 | ||
def get_build_modules(self): |
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 is a bit undesired that this is called so many times, it could be cached, but the above is redefining self.build_modules = BuildModulesDict.from_list(self.get_build_modules())
, which is used in the method, so that might break such caching.
Recall that things in CppInfo might be called mamy many times while propagating graph, so this is one of the bottlenecks in graph resolution, we should be careful with adding here inefficient things.
conans/test/integration/generators/cpp_info_set_generator_properties_test.py
Outdated
Show resolved
Hide resolved
conans/test/integration/generators/cpp_info_set_generator_properties_test.py
Outdated
Show resolved
Hide resolved
conans/test/integration/generators/cpp_info_set_generator_properties_test.py
Outdated
Show resolved
Hide resolved
conans/test/integration/generators/cpp_info_set_generator_properties_test.py
Outdated
Show resolved
Hide resolved
conans/test/integration/generators/cpp_info_set_generator_properties_test.py
Show resolved
Hide resolved
client.run("create mypkg.py") | ||
client.run("install consumer.py -s build_type=Release") | ||
|
||
old_approach_contents = get_files_contents(client.current_folder, files_to_compare) |
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 like very much these tests comparing the previous and new results, good job
Changelog: Feature: Add
set_property
andget_property
to set properties and access them in generators. Can be set only for a specific generator or as a default value for all of them.Changelog: Feature: Use
set_property
andget_property
to support custom defined content inpkg_config
generator.Changelog: Feature: Add new property names:
cmake_target_name
,cmake_file_name
,pkg_config_name
andcmake_build_modules
that can be used for multiple generators of the same type allowing also an easier migration ofnames
,filenames
andbuild_modules
properties to this model.Docs: conan-io/docs#2082
Closes: #7661 , #8600
Set properties of the cpp_info that are related to generators (things like names, filenames, build_modules...) with
set_property
andget_property
. Tests withfilenames
,names
andbuild_modules
properties.Using new properties names
cmake_target_name
,cmake_file_name
,pkg_config_name
andcmake_build_modules
will make the migration a little bit less risky. The new property name will be given preferences over the old one.This:
would be expressed as this:
It's also possible to make it specific for generator but that most of cases it won't be necessary:
Also, not specifying the generator will set this value as a default for every generator. So if you set:
A new custom generator can access that information with:
Related to: #8600, #8568 and #7661
#tags: slow