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

User-defined generators in local cache #7527

Merged
merged 17 commits into from Sep 29, 2020

Conversation

kralicky
Copy link
Contributor

@kralicky kralicky commented Aug 10, 2020

Changelog: Feature: Allow user-defined generators to be installed and used from the Conan cache.
Docs: conan-io/docs#1811

Fixes #7432

This change allows user-defined generators to be installed into the Conan local cache. Generators installed in the cache can be used in the same ways as the built-in generators, i.e. they can be used from the command line with conan install -g, and they can be used from a recipe without specifying a package requirement. Generators can be installed using conan config install.

While working on this, I noticed what I believe to be a very minor bug in loader.py, in which a generator in a module could be registered globally before determining that the module is ill-formed. The generator would be left around after an exception is thrown. In practice, I don't think this scenario would ever cause a problem seeing as the global generator list goes away when the program exits but this could theoretically impact unit tests and cause incorrect results. Let me know if you want me to create a separate issue for this.

Additionally, I added a function in the ConanFileLoader class to allow loading only Generator classes found in a module. This was done so that generator classes in the local cache would not need a recipe in the same module. It wouldn't make sense to package a generator used in this way, so the need for a recipe would be confusing. It would also allow defining multiple generators in the same file (if so desired).

Let me know if I should add any additional test cases as well.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the 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.

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

I like the idea of generators that can be installed and function as "built-in" with conan config install, but I am trying also to think about possible rough edges.

The main one is that this seems to go in the direction of "global state", while there is some evidence that moving towards locally versioned could be more powerful and flexible:

  • A generator that is imported by recipes with a python_require, for example, can be versioned.
  • Each package could use its own version of generator (not implemented yet)
  • The package_id_mode effect of python_requires could be a mechanism to control what needs to be built, bumping version of the generator could fire re-build of packages or not, based on the versioning approach.

Our experience with built-in generators is that it is not that simple to evolve them without breaking. Generators in the cache will suffer the same, it will be basically impossible to know how the generator changes, when and which packages need to be rebuilt, and it will force whole project to move generator at once.

As always, it is a trade-off, and both approaches are not incompatible, both could be implemented, this is why I wouldn't oppose to this PR.

Please have a look at the comments, it might require more work and some kind of lazy loading, to ensure that generators are loaded only once (you will probably hit this when addressing the conan create issue). Do not hesitate to ask for help or if you have any question. Thanks for contributing this!

client.save({"conanfile.py": conanfile_py})

# Test the install using a conanfile
client.run("install . --build")
Copy link
Member

@memsharded memsharded Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will fail if you do conan create. These generators can only work at the end consumer, but will fail for package creation in the cache.

Copy link
Contributor Author

@kralicky kralicky Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added another test case for this, let me know if it looks correct.

def content(self):
return "user_defined contents"

class UserDefinedConan(ConanFile):
Copy link
Member

@memsharded memsharded Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When generators are in the conan cache, this is totally irrelevant and unused. But if removed, parsing will fail, so there is no very elegant solution.

Copy link
Contributor Author

@kralicky kralicky Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I forgot to take this out. The ConanFile class is not actually required here, it will parse only the generators.

@kralicky kralicky force-pushed the feature/local-cache-generators branch from 2486a75 to 70ab8d6 Compare Aug 27, 2020
@kralicky kralicky force-pushed the feature/local-cache-generators branch from 82c7ad2 to ef1a893 Compare Sep 12, 2020
@memsharded
Copy link
Member

memsharded commented Sep 25, 2020

Just a follow up. I am reviewing this, and it has a minor detail that makes it pass the test, but will fail in real command.
The reason is that the registered_generators is an ugly global variable, that remains in memory while python interpreter is running, extending its life among tests. I am trying to remove in #7758 this global variable, and then I will come back here to make sure things work correctly. Thanks!

@kralicky
Copy link
Contributor Author

kralicky commented Sep 25, 2020

Good catch and #7758 looks like a good change. I will update my branch this weekend.

memsharded and others added 6 commits Sep 25, 2020
This adds a generators folder in the local client cache where user
generators can be installed. When installing a package using
the `conan install` command, generators in the cache will be loaded
and available to use with the -g flag.
@kralicky kralicky force-pushed the feature/local-cache-generators branch from ef1a893 to 393af3e Compare Sep 26, 2020
@memsharded
Copy link
Member

memsharded commented Sep 29, 2020

Hi @cobalt77

I have merged in this branch the develop one after the refactor of the generators making them non-global.
I have also done some minor refactors, and added one scenario in the test that causes it to fail. Basically, the loading of the generators in the cache has to be done earlier, so it cover all cases. I will have a look to check what is the best place to put it.

@memsharded
Copy link
Member

memsharded commented Sep 29, 2020

I propose to move them to the BinaryInstaller constructor

@kralicky
Copy link
Contributor Author

kralicky commented Sep 29, 2020

Looks good to me.

@memsharded memsharded merged commit 3f84d14 into conan-io:develop Sep 29, 2020
1 of 2 checks passed
@memsharded memsharded added this to the 1.30 milestone Sep 29, 2020
@memsharded
Copy link
Member

memsharded commented Sep 29, 2020

Merged! Thanks very much for your contribution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Allow user-defined generators to be used with conan install -g
3 participants