Skip to content

building: extend DLL search paths when searching for binary deps#6925

Merged
rokm merged 1 commit intopyinstaller:developfrom
rokm:extend-win-dll-search-paths
Aug 16, 2022
Merged

building: extend DLL search paths when searching for binary deps#6925
rokm merged 1 commit intopyinstaller:developfrom
rokm:extend-win-dll-search-paths

Conversation

@rokm
Copy link
Copy Markdown
Member

@rokm rokm commented Jul 2, 2022

As part of depdendency analysis, we search for binary dependencies (shared libraries) of the collected binaries, which is done by
running the find_binary_dependencies helper function in an isolated sub-process.

In that helper, we already import all collected top-level packages; the aim was to ensure that library search paths, which may be
modified during packages' initialization, are up to date. In practice, however, just importing the packages has little effect on search paths - at least on Windows, where we still consider only the binary's parent directory.

Therefore, we now try to explicitly extend the search paths on Windows, obtaining extra DLL search directories:

  • from the PATH environment variable once all packages are imported
  • by tracking calls to os.add_dll_directory calls during package imports by monkey-patching that call

Fixes #6924.

Fixes pyinstaller/pyinstaller-hooks-contrib#317.
Fixes #6482.
Fixes #6960.

Copy link
Copy Markdown

@skrynnik skrynnik left a comment

Choose a reason for hiding this comment

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

All works well

@jamesbcd
Copy link
Copy Markdown

jamesbcd commented Aug 7, 2022

Any chance we can get this merged? I've been using it the last month or so and it's working really well.

@rokm
Copy link
Copy Markdown
Member Author

rokm commented Aug 7, 2022

All works well

I've been using it the last month or so and it's working really well.

This change works well for the PySide6 issue, but only because the DLLs would be collected to the top-level application directory anyway.

For other packages (for example av), where DLL directory could not be automatically searched up until now, this will result in duplication of DLLs:

  • once they will be explicitly collected by the hook, while preserving sub-directory structure (which is where they should be collected, because the package also expects to find them there)
  • due to this change, the DLL directory will be discovered (which, by itself, is a good thing), but because our binary dependency analysis collects the discovered DLLs to the top-level application directory, this will result in duplicates

I've been meaning to revise the dependency analysis to preserve the DLL sub-directory structure where possible, but for that to work on all OSes, I need to add support for symlinks on linux and macOS first...

Although , we could try implementing the DLL directory structure preservation for Windows in advance, as we won't have symlinks there anyway. And any regressions stemming from DLLs not being collected into top-level directory will have to be discovered and addressed sooner or later...

@jamesbcd
Copy link
Copy Markdown

jamesbcd commented Aug 8, 2022

Ah, I see. I appreciate the complexity!

@rokm
Copy link
Copy Markdown
Member Author

rokm commented Aug 15, 2022

In addition to solving the PySide6 6.3 problem, this will likely silence the slew of missing DLL warnings that people tend to get on Windows when using torch (e.g., #7024) or packages that use delvewheel and put their DLLs in external library directory.

Furthermore, it should also solve the problem with Anaconda-installed torch where we end up missing uv.dll due to lack of binary analysis (since the horrible brute-force hook collects everything as data) and due to that DLL not being installed in site-packages/torch/libs but rather in conda env's Library/bin. The python extensions do undergo dependency analysis, and thanks to extended search paths, DLLs in site-packages/torch/libs are discovered now (which removes the missing DLL warnings) and in turn undergo dependency analysis as well, which leads to uv.dll being discovered as dependency and collected. Fixes pyinstaller/pyinstaller-hooks-contrib#317, #6482, #6960.

@rokm rokm force-pushed the extend-win-dll-search-paths branch from 51c8819 to ae45b29 Compare August 15, 2022 19:44
@rokm rokm marked this pull request as ready for review August 15, 2022 19:46
@rokm rokm requested a review from bwoodsend August 15, 2022 19:46
As part of depdendency analysis, we search for binary dependencies
(shared libraries) of the collected binaries, which is done by
running the `find_binary_dependencies` helper function in an
isolated sub-process.

In that helper, we already import all collected top-level packages;
the aim was to ensure that library search paths, which may be
modified during packages' initialization, are up to date. In
practice, however, just importing the packages has little effect
on search paths - at least on Windows, where we still consider
only the binary's parent directory.

Therefore, we now try to explicitly extend the search paths on
Windows, obtaining extra DLL search directories:
 - from the PATH environment variable once all packages are imported
 - by tracking calls to `os.add_dll_directory` calls during package
   imports by monkey-patching that call
@rokm rokm force-pushed the extend-win-dll-search-paths branch from ae45b29 to a25d66a Compare August 15, 2022 22:18
@rokm rokm merged commit 33e28c3 into pyinstaller:develop Aug 16, 2022
@rokm rokm deleted the extend-win-dll-search-paths branch August 16, 2022 11:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

4 participants