{"action":"created","issue":{"url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1553","labels_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1553/labels{/name}","comments_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1553/comments","events_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1553/events","html_url":"https://github.com/pyinstaller/pyinstaller/issues/1553","id":108831091,"number":1553,"title":"Make the existing (old) \"ImportHook\" handler submit a API","user":{"login":"htgoebel","id":784161,"avatar_url":"https://avatars.githubusercontent.com/u/784161?v=3","gravatar_id":"","url":"https://api.github.com/users/htgoebel","html_url":"https://github.com/htgoebel","followers_url":"https://api.github.com/users/htgoebel/followers","following_url":"https://api.github.com/users/htgoebel/following{/other_user}","gists_url":"https://api.github.com/users/htgoebel/gists{/gist_id}","starred_url":"https://api.github.com/users/htgoebel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/htgoebel/subscriptions","organizations_url":"https://api.github.com/users/htgoebel/orgs","repos_url":"https://api.github.com/users/htgoebel/repos","events_url":"https://api.github.com/users/htgoebel/events{/privacy}","received_events_url":"https://api.github.com/users/htgoebel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":9,"created_at":"2015-09-29T09:30:13Z","updated_at":"2015-10-02T14:25:56Z","closed_at":null,"body":"Originally from @leycec https://github.com/pyinstaller/pyinstaller/pull/1393#issuecomment-143632631:\r\n@htgoebel Excellent questions! You always keep me on my gangrenous toes. :footprints: \r\n\r\n> One thing came to my mind: Shouldn't we make the existing (old) `ImportHook` handler submit a API, too?\r\n\r\n**Absolutely.**\r\n\r\nThe new `building.imphookapi.PostGraphAPI` class has been designed to be a drop-in replacement for the old `building.imphook.FakeModule` class. Despite bundling `PostGraphAPI` with this commit, I decided at the last minute _not_ to make the switch-over in this pull request. I was concerned about merge conflicts and test failures. There don't appear to be any, so I probably should have just done it. _Annyway._ (When laziness and risk aversion unite!)\r\n\r\nMaking the switch-over is trivial:\r\n\r\n1. Remove `building.imphook.FakeModule`.\r\n1. Refactor the `building.imphook.ImportHook._process_hook_function()` method to use `building.imphookapi.PostGraphAPI` instead. In theory, this just means renaming `FakeModule` to `PostGraphAPI` in that method.\r\n\r\nI've made the switch-over locally. So far, so good. I'll probably submit a new pull request finalizing this change over the coming week.\r\n\r\n> And should we merge the old hook mechanisms into the new ones?\r\n\r\n**Absolutely.**\r\n\r\ntallforasmurf and codewarrior0 were completely right on this point; I was completely wrong. Thankfully, I've seen the light. (_It burns!_)\r\n\r\n## What's the Plan, Man?\r\n\r\nHere's how I see this going down:\r\n\r\n1. The `building.imphook.HooksCache` should be completely refactored from the ground up to support **lazy hook loading.** Currently, `HooksCache` only caches the _filenames_ of all hooks on initialization. That's it. Instead, `HooksCache` should:\r\n  1. Probably still cache the filenames of all hooks on initialization.\r\n  1. Lazily cache the _in-memory module object_ returned by the `importlib_load_source()` utility function for each hook in a \"just-in-time\" manner. The contents of unused hooks will _not_ be cached. The contents of each used hook will be cached on the first attempt to access those contents, which will then be efficiently accessible by different **hook points** (e.g., `PyiModuleGraph._find_module_path()`, `PyiModuleGraph._safe_import_module()`) throughout the PyInstaller lifecycle.\r\n1. The contents of each hook in the new `hooks/pre_find_module_path` and `hooks/pre_safe_import_module` subdirectories should be moved back into the corresponding hook in the top-level `hooks` directory. For example:\r\n  1. The `pre_find_module_path()` function defined by the new `hooks/pre_find_module_path/hook-distutils.py` hook should be moved back into the old `hooks/hook-distutils.py` hook.\r\n1. The new `hooks/pre_find_module_path` and `hooks/pre_safe_import_module` subdirectories should go away.\r\n\r\n# What Could Possibly Go Wrong?\r\n\r\nThere are a few things to consider when performing this refactoring, however.\r\n\r\nFor each hooked module, `HooksCache` needs to cache the contents of `n` possible hook files – _not_ just `1`. These are:\r\n\r\n* The official PyInstaller hook file for that module (if any).\r\n* The `n-1` unofficial user-defined hook files for that module (if any).\r\n\r\nTo ensure each hook function is called at most once, `HooksCache` will also need some means of tracking which hook functions for each cached hook file have already been called and which haven't. I'm fairly sure we can do this by just **deleting** each in-memory hook function from its in-memory hook object _after_ that function has been called. (There may be hidden dragons here, however.)\r\n\r\nIt's not exactly trivial. It's not exactly hard, either. I estimate a week's worth of design, implementation, testing, and documentation.\r\n\r\n## What About Them Hook Functions?\r\n\r\n_Yeah._ We'll still need separate hook functions, of course. These are:\r\n\r\n* `hook()` for post-graph hooks.\r\n* `pre_find_module_path()` for pre-find module path hooks.\r\n* `pre_safe_import_module()` for pre-safe import module hooks.\r\n\r\nAfter implementing lazy hook loading, we won't need to separate those functions into separate hook files. They'll all be unified into top-level `hook/` files. If nothing else, that should simplify both the documentation and maintenance of hooks.\r\n\r\nA big win for everybody! (Mostly, tallforasmurf. Best of luck on that documentation. _I don't envy you._)\r\n"},"comment":{"url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/comments/145036777","html_url":"https://github.com/pyinstaller/pyinstaller/issues/1553#issuecomment-145036777","issue_url":"https://api.github.com/repos/pyinstaller/pyinstaller/issues/1553","id":145036777,"user":{"login":"matysek","id":527280,"avatar_url":"https://avatars.githubusercontent.com/u/527280?v=3","gravatar_id":"","url":"https://api.github.com/users/matysek","html_url":"https://github.com/matysek","followers_url":"https://api.github.com/users/matysek/followers","following_url":"https://api.github.com/users/matysek/following{/other_user}","gists_url":"https://api.github.com/users/matysek/gists{/gist_id}","starred_url":"https://api.github.com/users/matysek/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/matysek/subscriptions","organizations_url":"https://api.github.com/users/matysek/orgs","repos_url":"https://api.github.com/users/matysek/repos","events_url":"https://api.github.com/users/matysek/events{/privacy}","received_events_url":"https://api.github.com/users/matysek/received_events","type":"User","site_admin":false},"created_at":"2015-10-02T14:25:56Z","updated_at":"2015-10-02T14:25:56Z","body":"> Regarding the excludedimports, I'm with you. But I would delay this change until after 3.0 release - for the sole reason to get 3.0 out of the door.\r\n\r\nI'm with you too."}}