feat: Disable loading Python DLLs from PATH by default on Windows

[zachlewis]

Users can enable the legacy behavior by setting env var OIIO_PYTHON_LOAD_DLLS_FROM_PATH=1.

This commit also changes the environment variable name from OIIO_LOAD_DLLS_FROM_PATH to OIIO_PYTHON_LOAD_DLLS_FROM_PATH to match the convention used by OCIO.

This PR addresses issue #4519

[zachlewis]

Hmmm. Welp, it appears this breaks the CI / Windows-2022 py3.9 test.

@lgritz , how would you prefer we handle this?

Various options:

1. For the CI task, simply set env var OIIO_PYTHON_LOAD_DLLS_FROM_PATH=1 to opt-in to the legacy DLL-loading behavior
2. Revert to less-secure "opt-out" method like we had before, but try harder to load DLLs from fewer paths in $PATH, at the expense of time when first importing OpenImageIO module in a given Python process (session)
3. Do something fancy at build-time to insert a install-specific hardcoded path reference into the init.py code.

IMO:

Option 1 is the path of least resistance, but may surprise folks. This is what OCIO is doing, and is the intended way of proceeding.

Option 2 is the least surprising, but ostensibly the most expensive and / or least secure, depending on our approach. I think this is what USD is doing.

Option 3 is the most contrived (IMO) and potentially the least portable, but inexpensive and secure. It's an idea that popped up at a recent OCIO TSC meeting. I can follow up with @Shootfast if we want to explore this route.

[lgritz]

I'm honestly not enough of a Python expert (and sure not a Windows) expert to have an informed opinion. I'm willing to go with your recommendation, especially if OCIO is doing the same thing.

I'm not sure I understand what this code is doing, nor the implications for regular users (not CI) of change any of this behavior.

@anderslanglands Do you have an opinion? I think this was originally your code in __init.py__.

[anderslanglands]

This seems fine to me. I think my original PR had this disabled by default anyway (same as for OCIO) but we changed it for ease of use at some point.

[zachlewis]

Alrighty. Thanks for chiming in, Anders, much appreciated.

I'll adjust the ci workflow to enable the now-legacy DLL loading behavior.

So, remember -- if Windows folks who are using the traditional (i.e. non-pip-based) means of building OIIO's Python bindings suddenly start having trouble loading the module, the workaround is to set $OIIO_PYTHON_LOAD_DLLS_FROM_PATH=1.

If enough folks are having trouble, we can revisit reverting or adjusting this mechanism in the future.

[lgritz]

So just so I'm clear on this:

• Only will come up as a problem on Windows
• Only for people who build from source instead of using "pip install openimageio"
• Solution is OIIO_PYTHON_LOAD_DLLS_FROM_PATH=1
• There is no way to detect this situation and have it do the right thing automatically?

[lgritz]

By the way, the convention we've been following for environment variables is:

• OPENIMAGEIO_FOO : things for users that we document in the manual and support like any other feature
• OIIO_BAR : undocumented things for developers / debugging, could change at any time

[zachlewis]

• Only will come up as a problem on Windows

Correct!

• Only for people who build from source instead of using "pip install openimageio"

Short answer: Yes.

People using pip install openimageio will pull from the prebuilt bdists created and uploaded to pypi.org via our wheels.yaml github workflow.

People can also build from source by navigating to the repository root and using pip install .. The pip-based method of building from source simply downloads cmake and ninja if needed, and invokes the underlying cmake-based build system, with a handful of cmake options pre-set (governed by the [tool.scikit-build.cmake.define] section in pyproject.toml). One can also override and set CMake options from the command line (pip install . --config-settings=cmake.args=-DSOME_DEFINE=ON;-DOTHER=OFF); or via the environment (SKBUILD_CMAKE_ARGS: "-DSOME_DEFINE=ON;-DOTHER=OFF").

Building from source via pip will "do the right thing" without end-user intervention, because skbuild controls for where libOpenImageIO lives, relative to init.py.

• Solution is OIIO_PYTHON_LOAD_DLLS_FROM_PATH=1

Correct!

• There is no way to detect this situation and have it do the right thing automatically?

As I understand it: sort of, but not really without violating the security precautions this PR intends to rectify in the first place!

The problem has to do with blindly adding all directories in $PATH to Python's DLL search path.

We could be more careful about this -- for example, we could use some filename globbing heuristics to check each path in PATH for a valid-sounding OpenImageIO library filename before adding a given directory to the DLL search path, prior to trying to import OpenImageIO. I've not seen a library that takes this approach, but... hey, there are all sorts of things I've not seen...!

It's worth noting that the legacy behavior (always adding all paths under PATH to the DLL load paths) can also cause Python to segfault when trying to import OpenImageIO on some installations; so it's very possible this change will fix stuff that used to not work for some folks.

Before going down the rabbit hole of trying to outsmart and preempt any adverse, surprising behavior, I think it might be worth seeing if this change actually causes problems "in the wild" -- presumably, we'd hear about problems with OpenColorIO as well, after which point, we could regroup and find a more robust solution.

What do you think?

By the way, the convention we've been following for environment variables is:

I'm happy to use OPENIMAGEIO instead of OIIO for the environment variable prefix, if you'd like. I'm currently using OIIO just cuz that's how it was before, and because OCIO is using a similarly-named OCIO_PYTHON_LOAD_DLLS_FROM_PATH environment variable.

Your call!

[lgritz]

Yes, let's do what OCIO did, I trust your intuition on this (and I have literally NO intuition about the situation myself), and as you say, if it causes problems in the wild, we can just fix it on the next patch. Let's get this finalized and merged in time to have the Feb 1 release include a published python wheel.

Please do switch the environment variable to the usual convention and document it as clearly as we can in the docs (other env variables are at the end of imageioapi.rst) and in the INSTALL.md file.

[lgritz]

LGTM

[lgritz]

Is this ready to merge, @zachlewis ?

[zachlewis]

Ready to merge! 🫡