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
Feature/compatible ids #5837
Feature/compatible ids #5837
Conversation
A priori I think this is a nice addition, I will review it after #5836 changes are taken into account (merge develop?). At first sight, just a comment: Would you consider an additional method? def package_compatible_ids(self):
for info in ...:
self.compatible_ids.append(info) This way, this method will only be invoked if the true-ID fails (and probably if there is an opt-in/out activated/deactivated). And probably it results in a cleaner recipe. |
Merged develop, and yes, I think that might be a good idea, I will try it. |
Status update:
|
@danimtb it is necessary to test this feature for a flat Intel compiler model. Please make a PoC of it. |
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 really think this feature adds a lot of value, I only have one request I've commented below.
I'm also thinking about the compatible_packages
member that we could make private. It would require adding the CompatiblePackage
to the conanfile
in the ctor:
class CompatiblePackage(object):
def __init__(self, conanfile):
...
self._conanfile.compatible_packages.append(self)
and then we won't need the append
in the package_id
method:
def package_id(self):
for optimized in range(int(self.options.optimized), 0, -1):
compatible_pkg = CompatiblePackage(self)
compatible_pkg.options.optimized = optimized
# self.compatible_packages.append(compatible_pkg)
But I haven't decided yet if it is better to have or not that last line. wdyt?
Oh, yes, I like it. I prefer that we hide the append, less lines, less interface surface. I am going to add it. |
Linking here the POC with the Intel compiler: https://github.com/memsharded/conan/compare/feature/compatible_ids...danimtb:feature/5590_compatible_ids?expand=1 |
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.
Do the 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.
As we discussed I don't like the interface, a user declaring a CompatiblePackage(self)
is registering something magically, and the user is not accustomed to that, is accustomed to doing CMake(self)
and happening nothing. But anyway, is your call.
On a second (third, fourth?) thought, I think yes it might be better to have |
I agree with the previous comments and I'd go for a more explicit approach. If there is no get-function, I think it is better to add the It requires documenting and exposing more interface (the |
Please check latest changes, I have added checking the final settings/options in the |
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.
Great! Really needed to propagate the actual settings
and options
to be used by the package_info()
function.
Changelog: Feature: New binary compatibility mode. Recipes can define in their
package_id()
an ordered list of binary package variants that would be binary compatible with the default one. These variants will be checked in order if the main package ID is not found (missing), and the first one will be installed and used.Docs: conan-io/docs#1468
Closes #5839
This mechanism can be used:
Maybe this feature can be tested for the Intel-Others compilers issue before releasing it.