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

fix inherit system_requirements #7721

Merged
merged 2 commits into from Sep 22, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Sep 17, 2020

Changelog: Bugfix: Correctly inherit and use system_requirements when using python_requires.
Docs: Omit

Fix #7718

Copy link
Contributor

@jgsogo jgsogo left a comment

I would use a different strategy here: I think that the base ConanFile class should list all the methods (it will prevent IDEs from showing errors), and we are already prepending the baseclass from python_requires_extend to the ConanFile itself, so the method from the python_erquires takes precedence.

In fact, the only change needed is the one in conans/client/installer.py, I would remove the if (rare btw) at the beginning, it is safe to run system_requirements() always.

wdyt?

@memsharded
Copy link
Member Author

memsharded commented Sep 18, 2020

I would use a different strategy here: I think that the base ConanFile class should list all the methods (it will prevent IDEs from showing errors), and we are already prepending the baseclass from python_requires_extend to the ConanFile itself, so the method from the python_erquires takes precedence.

I agree 100% with this

In fact, the only change needed is the one in conans/client/installer.py, I would remove the if (rare btw) at the beginning, it is safe to run system_requirements() always.

The problem is there is a nasty legacy here. The system_requirements() expects to return a string as a result, and store that result of being called in the cache, so it is not called again every "conan install". Using the base Conanfile class system_requirements() will start adding those files for every package in the cache, as if they actually installed system requirements. So it seems a bit risky to me, we probably cannot change that without breaking.

@jgsogo
Copy link
Contributor

jgsogo commented Sep 18, 2020

Uff, the only alternative I can think of is to iterate the base classes (type(conan_file).__bases__) looking for the system_requirements function in any of them but the ConanFile. Almost what it is being done right now.

@memsharded
Copy link
Member Author

memsharded commented Sep 18, 2020

I have found an elegant solution that doesn't require iterating:

if type(conan_file).system_requirements == ConanFile.system_requirements:
      return  #Do nothing

czoido
czoido approved these changes Sep 22, 2020
@czoido czoido merged commit 9070c68 into conan-io:develop Sep 22, 2020
2 checks passed
@memsharded memsharded deleted the fix/inherit_system_requirements branch Sep 22, 2020
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.

[bug] system_requirements is ignored when using python_requires
3 participants