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

fixing module.__file__ is None in py3 #4669

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Mar 6, 2019

Changelog: Bugfix: Solve problem with loading recipe python files in Python 3.7 because of module.__file__ = None
Docs: omit

closes #4636
close #4641

@PYVERS: Macos@py27, Windows@py36, Linux@py27, py34

@ghost ghost assigned memsharded Mar 6, 2019
@ghost ghost added the stage: review label Mar 6, 2019
@memsharded memsharded added this to the 1.13 milestone Mar 6, 2019
@memsharded memsharded assigned lasote and unassigned memsharded Mar 6, 2019
@ghost ghost assigned memsharded Mar 6, 2019
try:
folder = os.path.dirname(module.__file__)
except (AttributeError, TypeError): # Namespace packages py3 module.__file__
folder = module.__path__._path[0]
except AttributeError: # some module doesn't have __file__
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this except is for the module.__path__._path[0]? The comment is not very clear.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

This PR solves the problem reported by the user, but it is also introducing a test I do not agree with: test_helpers_python_library.

That test requires a sys.path.append(temp) to work, and it won't be a real use case of any recipe file. That use-case makes sense with a python module installed in the system, and I'm not sure about those global/static variables inside a library, maybe the logging package is the only one library with global state (and in that case, following the philosophy of Conan, we should promote the self.output in the recipe or even the conans.utils.log.logger, which in fact is shared among all recipes).

Nevertheless I agree with this PR, it fixes an issue, but I think we should have a look to some other corner cases proposed in #4664.

@lasote lasote merged commit 376990f into conan-io:develop Mar 6, 2019
@ghost ghost removed the stage: review label Mar 6, 2019
@lasote
Copy link
Contributor

lasote commented Mar 6, 2019

That use-case makes sense with a python module installed in the system, and I'm not sure about those global/static variables inside a library,

That is valid and currently used by companies. Pure python. About the global state, not so important but indeed a performance issue.

@memsharded memsharded deleted the feature/fix_loader_local_imports branch March 6, 2019 16:33
@jgsogo
Copy link
Contributor

jgsogo commented Mar 6, 2019

Yes, it is a valid use case, @lasote, but there are two things that are incompatible when loading dynamically python files:

  1. What it is here: import once and every recipe will use the same. It could make sense with a logger, output,... and that is the reason why Conan is already providing those utilities.

  2. The other perspective: every recipe should behave the same regardless of the rest of the recipes. Think about a constants.py file instead of a logger. This example is about two recipes following the same pattern:

    conanfile.py (this snippet is valid for recipe1 and recipe2):

    from constants import URL, DESC
    
    class MyLib(ConanFile):
       url = URL
       description = DESC
       exports = "constants.py"

    constants.py (accompaning recipe1)

    URL = "http://library1"
    DESC = "This library implements..."

    constants.py (accompaning recipe2)

    URL  = "http://library2
    DESC = "Functionality XXX"

    If this two recipes happen to be in the same Conan instance, the resulting behavior will be wrong, and it will depend on the order they are considered... and they would interact with a hook that could define a constants.py file too!

I think that both use cases are licit, but if I have to choose, I prefer reproducibility and consistency over sharing a global variable (and being afraid of not being using the one I pretended to use).

@memsharded
Copy link
Member Author

The constants.py files accompanying each recipe will be put in the uuid1.constants and uuid2.constants different namespaces, and belonging exclusively to each recipe, not depending in the order, and being reproducible and independent of the order of evaluation.

The problem comes with modules that do not belong to recipes, like globally installed python libraries. These cannot be put under uuidX.something duplicated for every recipe in the graph.

@jgsogo
Copy link
Contributor

jgsogo commented Mar 6, 2019

The constants.py files accompanying each recipe will be put in the uuid1.constants and uuid2.constants different namespaces, and belonging exclusively to each recipe, not depending in the order, and being reproducible and independent of the order of evaluation.

Yes, this PR is implementing that behavior too, you are true, it is doing the same I was proposing in #4664 and #4641 (plus the global thing).

Great! 👍

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] conanfile loader "crashes" because of missing __file__ attribute in Python 3.7(+)
3 participants