-
Notifications
You must be signed in to change notification settings - Fork 606
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
allow loading of dlls from PATH on Windows Python 3.8+ #3470
allow loading of dlls from PATH on Windows Python 3.8+ #3470
Conversation
src/python/__init__.py
Outdated
# This works around python 3.8 change to stop loading DLLs from PATH on Windows. | ||
# We reproduce the old behaviour by manually tokenizing PATH, checking that the directories exist and are not ".", | ||
# then add them to the DLL load path. | ||
# This behviour is only enabled if the environment variable "OIIO_LOAD_DLLS_FROM_PATH" is set to "1" so it is opt-in. |
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'm confused about this part. Is this just for testing? Or should it stay that you have to set this env variable in order for it to find the dll in the path? How will users know to do that?
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.
Or put another way, when would you not want to find it in the path? Was there a reason for the behavior change for python?
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.
https://bugs.python.org/issue36085 is the thread I believe. I’ve only skimmed but the line of thought appears to be “PATH is messy, let’s burn the world down”.
If your environment is carefully managed, like in a VFX facility, you probably want to stick to the old behaviour. If you’re distributing a package that bundles OpenImageIO’s Python module, maybe you’d want the new behaviour? I don’t know, personally in that case id prefer to link everything statically to be sure.
But either way what this PR is doing is explicitly reversing the standard behaviour of Python, so a way to opt out seemed prudent (something it would have been nice for the Python developers to provide tbh).
As far as discoverability goes, I could add something to the docs and release notes perhaps? We could also potentially invert the meaning of the flag so it’s opt out rather than opt in.
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.
Either way, I'll make sure the release notes mention it.
When you say "old behavior", do you mean "the behavior when using Python 3.8, immediately prior to this patch", or do you mean "the behavior with Python <= 3.7?"
Mainly what I was getting at was ensuring we were carefully choosing which way the default should be. I'm not a native Python speaker, and Windows is a foreign land I haven't visited for decades. (Seriously, I think the last version I had on a machine I used was Windows 2000.) So I have no idea what behavior Windows+Python users will expect or desire. We should choose a default behavior that will be least astonishing to people who operate in that environment -- with some attention to not having things break when people upgrade from py37 to py38, nor when (for either version of python) they upgrade from OIIO last to OIIO next.
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.
So I’m a reluctant user of both Windows and Python as well. The default behaviour in Windows is that PATH acts as a combination of what we’d expect from PATH and LD_LIBRARY_PATH on Linux. So on the face of it, the Python 3.8 change goes against the default OS behaviour, and in fact a working shared library build of OIIO (and OCIO and USD<21) will simply break when upgrading to Python 3.8+, even if all dependencies were linked statically, because it won’t find OpenImageIO.dll from OpenImageIO.pyd.
That being said, a couple of people in that thread mention that there was a specific Windows SDK version that allowed them to do this (or dropping support for an older one) so there’s some additional context there that we should probably understand more clearly before merging. I will try and do some research this week.
I do note though that USD decided just to YOLO it and and read PATH regardless (this PR is basically a copy/paste of what they did but with the addition of the feature flag).
OCIO on the other hand explicitly builds itself statically when building a wheel for PyPI distribution, but ignores the problem when doing a “normal” build. I haven’t checked but was told OTIO does the same thing. I would assume if you’re distributing for Python3.8+ on PyPI you’d want to follow the Python3.8 behaviour by default, while if you’re building for yourself you’d want the 3.7/Windows-default behaviour.
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'm inclined to recommend changing the default to "behave like people expect from python 3.7" and merge. We can always flip the default in later releases as the Windows/python population gets used to the new way.
But I'm thinking about people like us, who aren't paying attention to the "inside baseball" python conversations, who had 3.7 cobbled together and working, but then after what should have been a straightforward upgrade to 3.8, things are broken and they don't know why or what to change to fix it. Let's make it work for them.
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.
Now that I'm looking at this again... I don't see an opt in/out control at all. Maybe that's fine. We can always add a control to opt out later, if people request it.
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.
n/m, I see now that it's controlled by a runtime environment variable. I was clearly high and thought I remembered it as a cmake build-time switch.
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.
:D I've just pushed a commit to invert it so that the python-3.7 behaviour is the default and users can disable loading DLLs from path by setting OIIO_LOAD_DLLS_FROM_PATH=0
…reFoundation#3470) Python 3.8 stopped loading DLLs from PATH on Windows, meaning anything that relied on that environment for shared library resolution breaks. This PR works around that behviour by manually adding PATH back to DLL resolution using os.add_dll_directory() in a new __init__.py which just re-exports everything in the current .pyd. The new behaviour is enabled by default, but is easily disabled at runtime if the environment variable OIIO_LOAD_DLLS_FROM_PATH is equal to "1".
Description
Python 3.8 stopped loading DLLs from PATH on Windows, meaning anything that relied on that environment for shared library resolution breaks. This PR works around that behviour by manually adding PATH back to DLL resolution using
os.add_dll_directory()
in a new__init__.py
which just re-exports everything in the current .pyd.The new behaviour is only enabled if the environment variable
OIIO_LOAD_DLLS_FROM_PATH
is equal to "1".Tests
Checklist:
have previously submitted a Contributor License Agreement
(individual, and if there is any way my
employers might think my programming belongs to them, then also
corporate).
(adding new test cases if necessary).