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

Feature/new py requires #5804

Merged
merged 73 commits into from Dec 5, 2019
Merged

Conversation

@memsharded
Copy link
Member

memsharded commented Sep 24, 2019

Changelog: Feature: New python_requires declared as Conanfile class attributes. Includes extension of base class, they affect the binary packageID with minor_mode default mode. They are also locked in lockfiles.
Docs: conan-io/docs#1495

Intended to recover control from current python_requires, which resolution is executed by interpreter.

Things to take into account:

  • Used minor_mode as the default for package-ID definition, as it allows easy user config without forcing revisions
  • I am not adding python_requires information to conaninfo.txt. It was not there before, now I am adding it to ConanInfo for the package-ID, but not sure about the value of conaninfo.txt for this.

Needs to address: #5861

Will probably close #6034 (need to add tests)
#tags: slow
#revisions: 1

@memsharded memsharded requested review from jgsogo and lasote Sep 24, 2019
Copy link
Member

jgsogo left a comment

Comments:

  • The way we are extending the recipe implies that it won't be possible to add a python_requirements() function or to declare different python_requires depending on options or settings of the consumer recipe. This could be a limitation of python_requires

  • The syntax self.python_requires.Tool I think it is not needed (we don't have it for requirements, right?) and it won't work for libraries with some allowed chars like xz-utils.

  • I need one test with the canonical way to use python_requires: a recipe for the package itself and the ConanFile to inherit from returned from a get_conanfile function. Would it work something like:

    python_requires_extend = "base.get_conanfile_base()"
    
  • I need one test with a requirement and a python_requirements, both to the same package but different versions, is it supported by the graphlock?

I understand the need of taking control over the python_requires although I haven't investigated the problem myself and I don't have all the information about it and the limitations we have with the current implementation.

I like to have the python_requires member attribute to consume methods from the declared packages, it's aligned with the current implementation of requires or build_requires. Nice. This could even be dependent on settings or options, and have a python_requirements() function.

I don't like the python_requires_extend member attribute to inject a parent class. I really like the feature of inheriting from a python_requires but I feel like this implementation is strange to the language. I would like to look for more natural alternatives from the Python perspective (although they might be stranger for the C++ Conan user):

  • metaclasses
  • some magic: class MyRecipe(ConanFile, "pyrequire/...BaseConanFile")
@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Sep 25, 2019

The way we are extending the recipe implies that it won't be possible to add a python_requirements() function or to declare different python_requires depending on options or settings of the consumer recipe. This could be a limitation of python_requires

It is not possible now either, as they are evaluated much sooner than the settings or options are there. This doesn't change.

The syntax self.python_requires.Tool I think it is not needed (we don't have it for requirements, right?) and it won't work for libraries with some allowed chars like xz-utils.

We need a syntax to access them from within the recipe, any other suggestion? It is ok about xz-utils as you will not include a regular package as python requires. I could enable the self.python_requires["Tool"] syntax.

I need one test with the canonical way to use python_requires: a recipe for the package itself and the ConanFile to inherit from returned from a get_conanfile function. Would it work something like:

No longer needed. The nice and clean way to have it is:

class SomeClass(object):
    ....
class Pkg(ConanFile):
    ....

And then:

class Recipe(ConanFile):
     python_requires = "tool/..."
     python_requires_extend = "tool.SomeClass"

I think it reads much nicer than a wrapper method.

I need one test with a requirement and a python_requirements, both to the same package but different versions, is it supported by the graphlock?

Sure, can be added, it shouldnt be a problem for lockfiles, I will add the test.

I like to have the python_requires member attribute to consume methods from the declared packages, it's aligned with the current implementation of requires or build_requires. Nice. This could even be dependent on settings or options, and have a python_requirements() function.

As said before, it is not possible to delay that far the evaluation of python_requires, specially if we want to extend the class from them, we need them much before having settings and options defined. Furthermore, it has not been requested, I'd vote to keep this one as simple as possible.

I don't like the python_requires_extend member attribute to inject a parent class. I really like the feature of inheriting from a python_requires but I feel like this implementation is strange to the language. I would like to look for more natural alternatives from the Python perspective (although they might be stranger for the C++ Conan user):

Feel free to suggest real alternatives, but please take into account that most of this effort is to reduce complexity on our side, yes, even at the expense of a not very pythonic approach. The code to inject the base class is around 4-5 lines, please try to keep the alternatives simple.

Thanks for the review!

@lasote lasote self-assigned this Sep 30, 2019
@lasote lasote added this to the 1.20 milestone Oct 2, 2019
@memsharded memsharded marked this pull request as ready for review Nov 20, 2019
@lasote
lasote approved these changes Nov 22, 2019
Copy link
Contributor

lasote left a comment

Remember the docs. Mentioning no exports_sources anymore and the alternatives. Also any corner case of things that cannot/shouldn't be inherited etc. I would say it shouldn't break anything from the old approach, so we should release it and receive issues.

@memsharded memsharded mentioned this pull request Nov 25, 2019
3 of 3 tasks complete
@lasote lasote removed their assignment Dec 3, 2019
@jgsogo

This comment has been minimized.

Copy link
Member

jgsogo commented Dec 4, 2019

Trying the recipes with Pylint, we need to add some magic to the pylint_plugin: memsharded#27

memsharded added 2 commits Dec 4, 2019
Add python_requires to pylint_plugin
@lasote lasote merged commit 272e342 into conan-io:develop Dec 5, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@memsharded memsharded deleted the memsharded:feature/new_py_requires branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
[refactor] python_requires
  
Awaiting triage
3 participants
You can’t perform that action at this time.