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

compatible ids #1468

Merged
merged 23 commits into from Nov 4, 2019

Conversation

@memsharded
Copy link
Member

memsharded commented Oct 29, 2019

@memsharded memsharded added this to the 1.20 milestone Oct 29, 2019
@memsharded memsharded requested a review from danimtb Oct 29, 2019
@memsharded memsharded referenced this pull request Oct 29, 2019
Copy link
Member

danimtb left a comment

Some suggestions and rephrasing. The attribute self.compatible_packages is not documented

creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
reference/conanfile/methods.rst Show resolved Hide resolved
reference/conanfile/methods.rst Outdated Show resolved Hide resolved
reference/conanfile/methods.rst Outdated Show resolved Hide resolved
memsharded and others added 10 commits Oct 29, 2019
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
Co-Authored-By: Daniel <danimanzaneque@gmail.com>
@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Oct 29, 2019

Merged your suggestions.

The self.compatible_packages is not documented on purpose. Besides the documented way to do it, I don't want to define any further detail, if its a list or not. The only thing that can be done with it is the provided code, nothing else.

@danimtb

This comment has been minimized.

Copy link
Member

danimtb commented Oct 29, 2019

IMO we should document the attribute with the intended usage and discourage any other if so. It will be more clear for users and easier to find for those looking in the attributes section. If you really feel it is not needed, I am fine with the current docs

@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Oct 29, 2019

I don't think searching for it as an attribute is a use case. Actually, this is one of the problems, I don't want it to be an attribute. This was one of the reasons we almost did hide it and make the append automatic. If it is an attribute, it seems you can call it and manipulate in other methods too, and this is definitely not the case.

So yes, I think it is better to leave them as-is at the moment.

@danimtb

This comment has been minimized.

Copy link
Member

danimtb commented Oct 29, 2019

We already have similar attributes documented with these restrictions:

cpp_info

Important
This attribute is only defined inside package_info() method being None elsewhere.

user_info

This attribute is only defined inside package_info() method, being None elsewhere, so please use it only inside this method.

info

Object used to control the unique ID for a package. Check the package_id() to see the details of the self.info object.

I think it is better to explain the intended usage than to hide it and let people experiment with it

@jgsogo

This comment has been minimized.

Copy link
Member

jgsogo commented Oct 29, 2019

I would move the motivation to the top of its section in the "Define ABI Compatibility". I think that this sentence is the key one and should be at the very beginning (with a little rewording it should be a good introduction): "this approach is very different from the documented above, because it is possible to have distinct binaries for the different versions, while the above, which is mainly an erasure, maintains 1 binary for all of them."

@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Oct 29, 2019

You hit the point. conan info attribute doesn't document that it cannot be used in other methods, and that is a mistake. Yes, can be fixed in the docs, but that is the issue. If you only document it in the method that can be used there is no space for confusion.

We cannot even say that compatible_packages will be None elsewhere, because it is already initialize to a list to precisely avoid the linter errors that later users complain about. So the docs you are suggesting are: "compatible_packages: don't use it anywhere but in the package_id() method, go and look the package_id() method docs about how to use it"? Should we add a experimental tag to this attribute too then, or rely on the package_id() section one? And if users search for it, and find this instead of the package_id() one, then they will need just another hop? Sure, can be done, but I see little value in it.

@memsharded memsharded requested a review from jgsogo Oct 31, 2019
Copy link
Member

jgsogo left a comment

Minor comments, I think they will help a little bit.

And one more question: shall we add a note saying that the package_id of the compatible one will be different and, hence, those consumers using a package_id_mode that takes into account the package_id of the requirements will get a different package ID?

I mean, the fallback affects downstream the graph to all the consumers (and it is ok)

creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
creating_packages/define_abi_compatibility.rst Outdated Show resolved Hide resolved
reference/conanfile/methods.rst Outdated Show resolved Hide resolved
reference/conanfile/methods.rst Outdated Show resolved Hide resolved
reference/conanfile/methods.rst Outdated Show resolved Hide resolved
reference/conanfile/methods.rst Outdated Show resolved Hide resolved
memsharded and others added 5 commits Oct 31, 2019
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
memsharded and others added 5 commits Oct 31, 2019
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
@lasote

This comment has been minimized.

Copy link
Contributor

lasote commented Oct 31, 2019

fix this: /home/travis/build/conan-io/docs/reference/conanfile/methods.rst:927:undefined label: compatible_package (if the link has no caption the label must precede a section header)

reference/conanfile/methods.rst Outdated Show resolved Hide resolved
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
@jgsogo
jgsogo approved these changes Oct 31, 2019
Copy link
Member

jgsogo left a comment

Ready for 1.20, let's see how people use this feature 🎉

@jgsogo jgsogo assigned danimtb and unassigned jgsogo Nov 4, 2019
@danimtb
danimtb approved these changes Nov 4, 2019
@danimtb danimtb merged commit c3c4d4e into conan-io:develop Nov 4, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.