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

regression: passlib doesnt work with py2exe #144

Closed
urishab opened this issue Aug 23, 2022 · 11 comments
Closed

regression: passlib doesnt work with py2exe #144

urishab opened this issue Aug 23, 2022 · 11 comments
Labels
bug Something isn't working

Comments

@urishab
Copy link
Contributor

urishab commented Aug 23, 2022

Just ran across a bug in py2exe where it doesn't add passlib automatically and even including passlib in the packages option doesn't help.
See the following code:

from passlib.apps import custom_app_context
print(custom_app_context.hash('1234'))

It runs under python 3.9 with the following pip reqs:

py2exe
passlib==1.7.4

If I package it using the following py2exe script it works in py2exe 0.10.2.1 but it fails with the most recent py2exe version (0.11.1.1):

from distutils.core import setup
import py2exe #@UnusedImport

setup(
    console= [ { "script": "test.py",}, ],
    options = {"py2exe": {
        "packages": ['passlib'],
        "includes":['configparser'],
 } },
)

For some reason only a couple of files are added from passlib instead of the whole package

@urishab
Copy link
Contributor Author

urishab commented Aug 23, 2022

If I explicitly add the missing files it works:
includes = ['passlib','passlib.handlers','passlib.handlers.sha2_crypt']

@albertosottile albertosottile added the bug Something isn't working label Aug 24, 2022
@albertosottile
Copy link
Member

albertosottile commented Aug 24, 2022

Thanks for reporting this. To some extent, this is the expected behavior of 0.11, as doing so allows to include just a sub-package, which is extremely useful for large packages (e.g. GUI bindings like wx, pyside2, pyqt5, and so on).

That being said, this appears to be a bug to me, as passlib is not usable without these sub-packages. I will give a look to why py2exe does not realize these sub-packages should also be included.

@urishab
Copy link
Contributor Author

urishab commented Aug 25, 2022

So in 0.11 there is no way to include an entire package? Was not aware of this change in behaviour.
Also I do not see this mentioned in the release notes.
Is it because of the change to mf310?

@albertosottile
Copy link
Member

The change happened because now we use the reference implementation of CPython modulefinder (https://github.com/python/cpython/blob/main/Lib/modulefinder.py) as base for our ModuleFinder. mf310 barely extends it. Hence, in cases like this, we are reproducing the behavior that Python has, which is:

>>> import passlib
>>> passlib
<module 'passlib' from '/Users/alberto/test/venv/lib/python3.9/site-packages/passlib/__init__.py'>
>>> passlib.handlers
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'passlib' has no attribute 'handlers'
>>> from passlib import handlers
>>> handlers
<module 'passlib.handlers' from '/Users/alberto/test/venv/lib/python3.9/site-packages/passlib/handlers/__init__.py'>

CPython modulefinder correctly mimics this behavior and does never import all the sub packages for you.

This change is not explicitly mentioned in the release notes because, to some extent, the previous behavior was incorrect (or not conform to Python) and now we switched to the expected behavior.

@urishab
Copy link
Contributor Author

urishab commented Aug 25, 2022

Problem is there's a lot of ways to import packages in Python
there should be a way to include an entire package folder tree in the executable, otherwise we'll always run into issues, won't we?

What is the difference between packages= and includes= options then?
The behaviour you describe sounds just like what I assumed includes= does.

@albertosottile
Copy link
Member

Problem is there's a lot of ways to import packages in Python

I am well aware of the amount of creative solutions in the Python ecosystem...

there should be a way to include an entire package folder tree in the executable, otherwise we'll always run into issues, won't we?

Not necessarily, see below.

What is the difference between packages= and includes= options then?

First, includes should only be used for modules, so, they are not really the same.
Second, here we come into why I labeled this as "bug" and not "expected behavior". Both the reference implementation of ModuleFinder and the extension made by py2exe (mf310) actually scan the code of each imported module to find imports and then import all the modules imported in the standard way.

Therefore, we see errors only in packages that do not use regular import mechanisms, either because the import is not performed in Python but rather in C, or they use a non-standard import system, or, like the case of passlib.__init__, because the import is not performed at all.

As I wrote in the first comment, I need to investigate why both CPython ModuleFinder and mf310.ModuleFinder fail in identifying these imports in your example (from passlib.apps import custom_app_context), as this might hint a bug in this scanning part. If this investigation finds out that passlib does not use a standard mechanism for importing its parts, then the solution will not be to overhaul the whole ModuleFinder architecture, but rather to simply write a hook for passlib.

@urishab
Copy link
Contributor Author

urishab commented Aug 25, 2022

I'm not following you regarding the functional difference between a module and a package in this context.
Putting a module in includes= option means that specific module gets imported and scanned for further imports.
Putting a package in packages= option means, according to your explanation, that the top level module (or the init.py) gets imported and scanned for further imports.
So wouldn't it be functionally equivalent to do includes=['passlib'] or packages=['passlib']? Since passlib is a top level module anyway?

I'd argue that having a very blunt method for including an entire package without relying on scanning for imports is useful.

We could always run into a package that does the imports in way that cannot be scanned and a "smart" hook cannot be written for it.
In that case you'd probably include a "stupid" hook that just adds every file under that package (or every python file).

The packages= option used to just such a "stupid" option. But configureable.
It's not efficient but it worked every time.

@albertosottile
Copy link
Member

albertosottile commented Aug 25, 2022

So wouldn't it be functionally equivalent to do includes=['passlib'] or packages=['passlib']? Since passlib is a top level module anyway?

This is a fair objections for this case, but not for the case of e.g. PySide2.QtCore and similar.

I'd argue that having a very blunt method for including an entire package without relying on scanning for imports is useful.

I am not saying it is not useful, I am just questioning the balance between our need for it and its implementation and maintenance cost. There is no reference implementation, as modulefinder does not support that (as importlib does not support that). The implementation we had in mf3 and mf34 was in the end causing more bugs than it solved, as both the CI and users were constantly reporting.

Switching our module finder to the reference implementation solved all these problems (and probably more) at once, but of course it reduced our wiggle room for extending it blindly. This is primarily why I am in principle against introducing more features in mf310. In addition, I seem to recall trying to implement this, at some point, but finding some issues as this is a not so easy task in Python (no way to transversely import a package, we would have to resort to something like pkgutil.walk_packages and perform the import manually).
EDIT: depending on how creatively the package is linked together, you might also hit the limits of pkgutil, at some point.

That being said, if you want, you can give it a try into reimplementing this feature, as I'd consider a PR that passes CI and does not tamper excessively with the vendor modulefinder code.

@albertosottile
Copy link
Member

albertosottile commented Aug 25, 2022

Going back to the issue at hand, I'd say a hook in this case is the preferred solution, as the "import chain" of passlib looks something like this:
from passlib.apps import custom_app_context -> from passlib import hash -> https://github.com/glic3rinu/passlib/blob/master/passlib/hash.py -> tamper with sys -> https://github.com/glic3rinu/passlib/blob/master/passlib/registry.py -> tamper with __getattr__ -> actual modules in handlers.
As you can see, no import is performed at all.

Fortunately, the hook can be as simple as:

def hook_passlib(finder, module):
    depth = getattr(finder,"recursion_depth_passlib", 0)
    if depth == 0:
        finder.recursion_depth_passlib = depth + 1
        finder.import_package("passlib.handlers")
        finder.recursion_depth_passlib = depth

Note that mf310 provides a convenience import_package method, but that is not recursive: it just imports all the top level modules in the package, which is exactly what we need in this case.

@urishab
Copy link
Contributor Author

urishab commented Aug 25, 2022

OK thanks for getting into this and let me know if I can help (test the fix?)

In any case I don't know enough about how py2exe works to actually have an opinion about the best implementation. If it works, it works.

albertosottile added a commit that referenced this issue Sep 18, 2022
@albertosottile
Copy link
Member

Fixed in 0.12.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants