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

Hooks: hook-Crypto: support for PyCryptodome #3424

Merged
merged 4 commits into from Apr 8, 2018

Conversation

Projects
None yet
2 participants
@Legrandin
Contributor

Legrandin commented Mar 24, 2018

PyCryptodome is an almost drop-in support for the now unmaintained PyCrypto library (Crypto namespace).

Important: this PR adds support for applications that use PyCryptodome, but it does not allow encryption of the python script (--key option). The reason is that PyInstaller assumes that either Crypto.Cipher.AES or Crypto.Cipher._AES are native Python extensions, but they are not in PyCryptodome.

Hooks: hook-Crypto: support for PyCryptodome
PyCryptodome is an almost drop-in support for the now
unmaintained PyCrypto library.
@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 25, 2018

Thanks for this pull-request. Two questions:

  1. Why not simply using collect_dynamic_libs()?
  2. Can't is_module_satisfies by used to verify PyCryptodome is the one package installed?

@htgoebel htgoebel added the hooks label Mar 25, 2018

@Legrandin

This comment has been minimized.

Contributor

Legrandin commented Mar 26, 2018

  1. This hook for the pycryptodome package is just a small variation of the existing one for pycryptodomex (#2226), so I simply assumed that design was fine. Is collect_dynamic_libs() guaranteed to look for all suffixes a compiled Python C extension can have (EXTENSION_SUFFIXES)? If so, I can also simplify this code and use that method instead.
  2. Good point. Then the hook becomes a no-op for PyCrypto. I will add that check.
@Legrandin

This comment has been minimized.

Contributor

Legrandin commented Mar 26, 2018

An update:

  1. collect_dynamic_patterns() does not find PyCryptodome's extensions, so the current approach is necessary.
  2. is_module_satisfies() returns True if you pass a package that does not exist. I didn't investigate the reason why exactly, but I modified slightly the code to do nothing when PyCrypto is installed, which is indeed the goal here.
@htgoebel

This comment has been minimized.

Member

htgoebel commented Mar 26, 2018

Thanks for the update :-)

  1. This hook for the pycryptodome package is just a small variation of the existing one for pycryptodomex (#2226), so I simply assumed that design was fine.

Well, hooks are of quite different age and quality :-)

  1. collect_dynamic_patterns() does not find PyCryptodome's extensions, so the current approach is necessary.

Okay, so maybe we should go back a step and look at the PyCryptodome again. :-) We need to include extension modules, not dynamic libraries. So there are two possibilities:

  1. Use PyInstaller.utils.hooks.collect_submodules() to collect all sub-module, both pure-python and extensions. One could restrict search to only extension modules by passing a filter-function. IMHO this is 2 not so nice solution since this would include a lot of code even if the application does not need it.
  2. Create a hook for each sub-package (Cipher, Hash, etc.) to reduce this effect.
  3. Create a hook for all python packages requiring an extension module. This would be the most efficient variant, but leads to a lot of hooks to be maintained.
  4. Add a list of these (python-package -> extension) relations to implies in PyInstaller/lib/modulegraph/find_modules.py - elegant but unfortunately this has be relation to PyCraptome anymore.
  5. Try to implement a hack for achieving the same as 5. would do.
  6. Extend the hook-api (see code in PyInstaller/building/imphook.py) for achieving the same as 5. would do.

Obviously I'd prefer solution (7) :-)

  1. is_module_satisfies() returns True if you pass a package that does not exist.

This is a bug. May I ask you to file a bug report for this, since we should also add test-cases? Thanks.

@Legrandin

This comment has been minimized.

Contributor

Legrandin commented Mar 27, 2018

PyCryptodome distributes dynamic libraries and builds them as if they were Python C extensions (even though they are not extensions - as they can't be imported by Python). It might sound a bit weird, but this decision is rooted in PyPy and its partial and slow support for C extensions. However, this also invalidates several of the existing methods used by PyInstaller to decide the right files to pull in.

I would like to avoid to submit more pull requests in the future as new modules are added, so I prefer to keep the current proposal as it is - despite being slightly inefficient, it gets the job done, it is simple enough and maintenance-free.

htgoebel added some commits Mar 30, 2018

@htgoebel htgoebel merged commit ba9d1b5 into pyinstaller:develop Apr 8, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@htgoebel htgoebel added this to the PyInstaller 3.4 milestone Apr 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment