-
Notifications
You must be signed in to change notification settings - Fork 81
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
Windows: fix DLL not found error in Python 3.8+ #305
base: main
Are you sure you want to change the base?
Conversation
This upstreams a patch from gvsbuild. In Python 3.8, a new os.add_dll_directory method was added to make loading DLLs in Windows more consistent. The impact of this is that even if I compile GTK successfully with MSVC, and add it to the Path, INCLUDE, and LIB, I am still not able to pip install pycairo in Windows. This finds the first GTK DLL location from the Windows PATH variable.
cairo/__init__.py
Outdated
if sys.platform == "win32" and sys.version_info >= (3, 8): | ||
env_path = os.environ.get("PATH", "").split(os.pathsep) | ||
first_gtk_dll_path = next( | ||
filter( | ||
lambda path: path is not None | ||
and Path.is_file(Path(path) / "cairo.dll"), | ||
env_path, | ||
), | ||
None, | ||
) | ||
if first_gtk_dll_path: | ||
with os.add_dll_directory(first_gtk_dll_path): | ||
from ._cairo import * # noqa: F401,F403 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to do this, how about moving this code into a file called cairo/_distributor_init.py
, following how numpy does it, https://github.com/numpy/numpy/blob/1d5b3397dc7beaf9ca7b2326127e74f55aecd162/numpy/_distributor_init.py? Also, I would prefer that file should by default be empty and only be added by the build system when appropriate, for example I don't want this to happen on the wheels distributed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll have to think about possible options so that if someone gets the pycairo source code and runs pip install .
or does pip install --no-binary :all: pycairo
it gets the DLL code, but it doesn't with the statically built wheel.
Would you be open to the CI deleting some code during build?
Maybe we could detect that the .lib files exist during runtime, and if they don't, search for DLLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also catch a DLL exception using a try-except, and only try to search for the DLLs if the user is about to get an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. Anything should work but having it separated into a separate file like that is better. @lazka may have a better opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up separating the DLL search logic in to a separate windows_init
module. However, we will break the imports need to be at the module level rule if we move all the logic.
I'm fully aware that this is frustrating and packag devs and users have to deal with this somehow without any proper guidance from CPython upstream, so the following is a bit of a rant, not directed at you or this PR...
Like every other package linking against external software
As it is intended and recommended by CPython developers, for security reasons.
Why doesn't it allow setting a dll path in that process? How does it handler other Python packages linking against external libraries? What if we check a PYCAIRO_DLL_DIRECTORY env var instead? to make sure this is only active during build time, and not when run on some random Windows machine?
Yeah, that was one of the reasons why I didn't want to provide wheels initially, but it helped more users that it hurt in the end.
|
No offense taken 😉. I want pycairo to be awesome as well, so I'm here to help us find a good solution.
We don't expect Linux and macOS users to explicitly say where their .so files are located before they run pycairo. I feel like Windows users should have a similar experience.
Correct, although I think the threat is pretty minimal if we are only adding very specific DLLs and not all DLLs found on the Path. They were definitely also protecting for the use case that libraries bundle their own libs instead of only relying on end users to explicitly declare a search path. In the Enable better DLL resolution issue, the solution they picked which zooba called number 1 was specifically to allow for libraries to package their own libs. This is a little different from that I agree though, we aren't talking about packaging our own libs but linking to them.
It does allow setting the DLL path if a dependency library uses add_dll_directory, for example PyInstaller tracks calls to add_dll_directory during package initialization.
This might be a good balance to do something like this. Wouldn't we need it set at runtime to find the DLL though? I can build pycairo fine, it is running pycairo where I get errors. |
I should have said "Like every other package linking against external software, on Windows"
Our extension only looks for cairo.dll, or libcairo-2.dll on the PATH with older CPython, just like your code. So there is no difference from what I see.
By build time I meant the "PyInstaller" part. What does PyInstaller do with the found DLLs via add_dll_directory? Copy them somewhere? |
I didn't understand this, are you saying that Pycairo is already trying to automatically load the cairo DLLs in Windows with the latest Python?
Yes, it packages them with the app. |
Add support for an environmental variable PY_DLL_DIR to explicitly set a DLL directory in Windows. Only search for DLLs in the path if PY_DLL_DIR isn't set, and we get an ImportError.
I made some improvements today to the PR:
I also did a bit of research to make sure that this approach is consistent with other similar libraries. PySide2 has a similar DLL search routine in its |
I did some more research about this today, and another option to solve this may be to bundle the DLL. I ran
I noticed in the setup.py, we already have the cairo.dll as part of the package data, but I am not sure if that is still being used still. Also, maybe this approach overlaps with the cairo static build approach that the current wheels are using? I also saw some projects using the cairo/_distributor_init.py file that @naveen521kk suggested above, and then in the main Any thoughts on either of these two approaches? |
Yeah previously
Could be. If we were to let the build-system/packager add |
@naveen521kk maybe we could define who this build-system/packager is since the packaging ecosystem in Python is really complex, and I'm not sure I understand. I'm also not in any way blaming Pycairo for this, I think this change in Python 3.8 left lots of libraries on the hook to individually find solutions which isn't great.
Right now, the workflow to build pycairo (1) is broken on Windows because I have to patch it manually, and I am not able to pip install the package from PyPI. This impacts the ability for me to package an application (2) using a freezer because it doesn't know where to find the DLLs. So when you say build-system/packager, do you mean operating system packagers (3) should be adding the |
Sorry for not being clear. By "build-system/packager" I meant anyone who builds wheels or packages the application, not just OS packagers. Only they would know the exact path where to find the I know that this doesn't actually fix the issue, where, one can build pycairo from the source and expect it to run on Windows. That's the reason we provide wheels on that Platform. For application developers, I think they could build their own wheels and patch it using I'll elaborate on a use case for
Which logic do you mean? Is it about using |
Hi @naveen521kk, thanks for the response!
In the Python ecosystem, the project that develops Python libraries and tools are the same people who distribute the sdist and wheel on PyPI. This is the 1st use case.
But because these wheels are statically built, they only work for a very small set of use cases that pycairo is used for. They won't work for PyGObject development.
I don't know of any other Python libraries that require developers to patch the library and rebuild it in order to do a pip install. This isn't a good developer experience.
Yup, the logic in this PR.
What do you see as the recommendation by the CPython developers? The release notes for Python 3.8 specifically talk about adding this add dll function for this purpose. Would it be acceptable if we didn't search on the PATH at all and only looked for another environmental variable? |
This was also attempted in #248 and #277.
In Python 3.8, Python changed the behavior to no longer automatically import DLLs from the Path. Unfortunately, Pycairo is effectively broken in Windows when building from source.
As a user, I should be able to follow any Pycairo example in Windows, and it should just work.
Right now, that is not possible for users to run
import cairo
without callingos.add_dll_directory
first.It also isn't possible to package an app using PyInstaller in Windows with Pycairo built from source because the hooks perform their analysis in an isolated subprocess, and there's no way to propagate the DLL search paths. Reference issue: pyinstaller/pyinstaller#7333
Using the wheels built for Pycairo isn't possible because they are statically linked, and I need a dynamic linking to use GTK and Cairo together. This was a clever idea though for a casual user just trying to make a quick app using pycairo standalone.
In #277, I thought this change wasn't needed, but it was only because I was using a patched version of PyGObject that was already adding the DLL directory.