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

Prioritize hook discovery / overwrites #4806

Closed
htgoebel opened this issue Apr 12, 2020 · 6 comments · Fixed by #4867
Closed

Prioritize hook discovery / overwrites #4806

htgoebel opened this issue Apr 12, 2020 · 6 comments · Fixed by #4867
Labels
feature Feature request

Comments

@htgoebel
Copy link
Member

After implementing #4582 we need to prioritize discovered hooks. Agreed order is:

  1. (highest) hooks added via command-line parameters
  2. hooks from packages
  3. (lowest) hooks included with PyInstaller
@htgoebel
Copy link
Member Author

htgoebel commented Apr 12, 2020

@bjones1 @Legorooj

  1. How shall this "overwrite" work in detail in respect to pre-find-module-path-, pre-safe-import-module- and runtime-hooks? I would expect, if there is a higher-prio hook of any hook-type, this would overwrite all hooks for that module.

    This comes down to collect a list of all modules where we have any type of hook for first.

  2. Related: Shall we change the directory structure of hooks having pre-find-module-path-, pre-safe-import-module- or runtime-hooks into this:

     + hooks/
        + hook-simple.py
        + hook-gi.repository.Atk/
           + hook.py
           + pre_safe_import_module.py
           + pre_find_module_pathook.py
           + pyi_gi_repo_Atk1.py
           + pyi_gi_repo_Atk2.py
           + rthooks.dat
    
  3. Shall hooks for packages overwrite those for sub-packages, too? E.g. shall ~/myhooks/hook-foo.py overwrite …/PyInstaller/hooks/foo.bar.py?

@Legorooj
Copy link
Member

RE your points:

  1. See end of comment.

  2. Interesting idea - how about both structures are allowed? The second (one-dir) for more complex hooks.

  3. No no. As far as the hook API and resulting enviroment is concerned, subpackages might as well be different packages altogether, and should be treated as such.

Point 1 - Overwrite

The default should be that (like with python imports) the first hook we come accross is the "only" hook. If it doesn't specify runtime and search hooks, then there are "None".

However, a way for the user to say "use the builtin search/runtime etc hooks" would be good.

@bjones1
Copy link
Contributor

bjones1 commented Apr 14, 2020

  1. I agree. I think, assuming the hook paths are correctly ordered, this is the behavior of the existing code.

  2. From what I see, run-time and pre-safe/pre-find hooks are rare. For this reason, I don't have a strong opinion. I would suggest keeping existing code, rather than adding a new approach.

  3. Good question. I would say no. What's the current behavior? I would expect no as well.

@Legorooj
Copy link
Member

Can I implement the following:

  • Update hook discovery
    • Hook folder order:
      1. Command Line
      2. Packages
      3. PyInstaller
  • If a second hook for a package is found, because of this order, we want to ignore the second hook. This is only for standard hooks (eg hook-jinja2.py), and this is also a temporary fix that would need updating later. Currently an AssertionError is raised upon a second hook having been found.

Both of these would be just for the 4.0 release, and would need fine-tuning - or redoing - in 4.1.

Slight implementation detail on the second point:

This would be a messier monkeypatch style fix. I'd simply add checkers in depend.imphook.ModuleHook which didn't load the module hook if a hook had already been loaded, and would set datas, binaries, hiddenimports, and excludedimports to empty lists/sets. This way it would work, just not as cleanly as it could.

@bjones1
Copy link
Contributor

bjones1 commented May 11, 2020

I'm happy if you work on this. Feel free to ask for help

Re point 2: issue a warning, at least, not just ignore it. On the messy implementation, I'll need to think about it and look at the code more closely.

@Legorooj
Copy link
Member

Legorooj commented May 11, 2020

@bjones1 I'll do it in a PR. Warnings are a good idea.

Legorooj added a commit to Legorooj/pyinstaller that referenced this issue May 11, 2020
Legorooj added a commit to Legorooj/pyinstaller that referenced this issue May 22, 2020
Legorooj added a commit to Legorooj/pyinstaller that referenced this issue Jun 4, 2020
Legorooj added a commit to Legorooj/pyinstaller that referenced this issue Jun 9, 2020
Legorooj added a commit to Legorooj/pyinstaller that referenced this issue Jun 16, 2020
Legorooj added a commit that referenced this issue Jul 2, 2020
…4867)

* Modulegraph: Reorder hook-dir search pattern. See #4806
* imphook: ModuleHook will now skip hooks for packages that already have a loaded hook
* Hooks: Only allow one source to define runtime hooks for any one package
* imphook: AdditionalFilesCache now extends dependencies instead of overwriting them
* tests: update the pyimodulegraph rthooks tests for the right number of runtime hooks
* tests: add tests for AdditionalFileCache and the hook-collection
* Add changelog entry
jeremyd2019 pushed a commit to jeremyd2019/pyinstaller that referenced this issue Jul 8, 2020
…yinstaller#4867)

* Modulegraph: Reorder hook-dir search pattern. See pyinstaller#4806
* imphook: ModuleHook will now skip hooks for packages that already have a loaded hook
* Hooks: Only allow one source to define runtime hooks for any one package
* imphook: AdditionalFilesCache now extends dependencies instead of overwriting them
* tests: update the pyimodulegraph rthooks tests for the right number of runtime hooks
* tests: add tests for AdditionalFileCache and the hook-collection
* Add changelog entry
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants