[FEATURE REQUEST] Turn off OIIO_LOAD_DLLS_FROM_PATH by default

[zachlewis]

This regards behavior introduced by @anderslanglands in #3470 pertaining to Windows / Python 3.8+ DLL loading, and mirrors an identical issue with the OpenColorIO Python API:

This variable was introduced to work around a change in Python 3.8 around its DLL search path on Windows, following what was done in other projects such as OIIO. However, not only is it bad practice (the change in Python's behavior was intended to fix a security issue), but it was also breaking our Platform Latest CI. Based on the discussion at the TSC meeting today, we should turn this off by default and document the Windows installation instructions to make people aware of the issue.

(Anecdotally, in the OCIO #helpdesk Slack channel, we've recently encountered Windows users experiencing segfaults when trying to import pip-installed PyOpenColorIO unless they set OCIO_PYTHON_LOAD_DLLS_FROM_PATH=1.)

Currently, users can "opt out" of this behavior by setting OIIO_LOAD_DLLS_FROM_PATH=0.

Instead, I would rather have users "opt in", and only load DLLs from $PATH if os.getenv("OIIO_LOAD_DLLS_FROM_PATH", "0") == "1".

Looking back in the commentary, it looks like #3470 originally intended for opt-in-style behavior, but was later changed to opt-out-style, to inflict the least amount of surprise on Windows Python-3.8+ users expecting Python-3.7 behavior:

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.

Originally posted by @lgritz in #3470 (comment)

I propose that we flip the behavior for OIIO-3.0, and/or find a more discerning method of adding paths to the DLL search path.

Describe the solution you'd like

This is what src/python/__init __.py looks like now:

import os, sys, platform

# This works around the 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 can be disabled by setting the environment variable "OIIO_LOAD_DLLS_FROM_PATH" to "0"
if sys.version_info >= (3, 8) and platform.system() == "Windows" and os.getenv("OIIO_LOAD_DLLS_FROM_PATH", "1") == "1":
    for path in os.getenv("PATH", "").split(os.pathsep):
        if os.path.exists(path) and path != ".":
            os.add_dll_directory(path)

I'm suggesting that we change the first if-statement to:

if sys.version_info >= (3, 8) and platform.system() == "Windows" and os.getenv("OIIO_LOAD_DLLS_FROM_PATH", "0") == "1":
    ...

Describe alternatives you've considered

One thing we could do is wrap this behavior in a try / except block, only loading DLLs from PATH if import OpenImageIO raises a ModuleNotFound exception.

We could also find a way to be a bit more selective with the paths we add -- perhaps by recursing through the contents of each directory in PATH, and only adding paths that contain a file whose name ends with "OpenImageIO.dll" or something.

...

for path in os.getenv("PATH", "").split(os.pathsep):
    if os.path.exists(path) and path != ".":
        if any([i.lower().endswith("openimageio.dll") for i in [os.listdir(path)]]):
            os.add_dll_directory(path)

Additional context

USD is also doing something similar, although I'd be lying if I claimed to understand exactly what's going on. Maybe by wrapping the module loading procedure in a context manager, they're able to inoculate against the sorts of segfaults we're experiencing.

[zachlewis]

Just noting that the equivalent change for OpenColorIO has been made for v2.4.1; and I'd like to keep OIIO's behavior in sync with OCIO. 🎣 🎣 🎣

[lgritz]

I'm not enough of a Python expert to really have an opinion here, though I'm inclined to accept "OCIO already hashed through this, let's match what they do" as a reasonable default.

Can anybody else chime in here with thumbs up/down, or preferably turn this suggestion into a PR that can be reviewed and easily merged?