AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.78k stars 452 forks source link

Improve handling of Windows path change in Python 3.8+ #1785

Open doug-walker opened 1 year ago

doug-walker commented 1 year ago

In PR #1759 we made a fix to try and solve problems caused by the change in library path behavior of Python >= 3.8 on Windows. Following the solution used in OIIO, we basically defaulted to adopting the previous Python behavior. However, based on feedback in PR #1668, this should be improved.

There is not agreement that the solution is simply to make the existing env var default to the Python 3.8+ behavior since the people most likely to be affected by the problem are users with limited development expertise.

Based on TSC discussion, we will investigate building the Python binding statically, similar to the wheels. At that point we would remove the OCIO_PYTHON_LOAD_DLLS_FROM_PATH env var option. Users would need to manage the paths on their own.

Another solution would be to add only the path(s) where the OCIO required libraries are, although initial discussion suggested this might be more difficult to get right.

We're open to feedback on the best approach. The recommended solution might be something that should be coordinated among ASWF projects, as it's likely that many of them are experiencing this issue.

For reference, here was the OIIO discussion on this topic: https://github.com/OpenImageIO/oiio/pull/3470 And here was the USD approach: https://github.com/PixarAnimationStudios/USD/commit/2a6c08f921bc8e58bd3fb9d8cda30a68f76b758a

JeanChristopheMorinPerso commented 1 year ago

Should https://github.com/AcademySoftwareFoundation/OpenColorIO/pull/1759 be reverted in the meantime?

JeanChristopheMorinPerso commented 1 year ago

It might be worth asking for feedback in ASWF's wg-python3 Slack channel. The working group is archived, but the channel is still used from time to time to ask Python related questions.

remia commented 1 year ago

Just wanted to restart the discussion here as the point was raised again by @JeanChristopheMorinPerso and we had a discussion about it in yesterday TSC.

The way I currently understand things, we are building the Python wheels using BUILD_SHARED_LIBS=OFF and OCIO_INSTALL_EXT_PACKAGES=ALL which means all dependencies and OCIO are built and linked statically to the Python module. In a recent PR #1802 we are now switching to BUILD_SHARED_LIBS=ON which means the OCIO library is now a shared library and the Python module need to load it at import time. Following this PR, the layout of the wheel have the Python module in the root folder and the OCIO DLL in a bin subfolder. This is accounted for by manually adding the bin folder to Python search path or PATH depending on the version here. This should hopefully work out of the box for most users.

Now if we want to build the Python module manually from the OCIO source tree, and use dynamically linked OCIO dependencies (expat, ...) and/or dynamically linked OCIO library, some additional folders might need to be added to Python search paths. We could investigate this use case further to see what would exactly be needed if this is a common scenario.

The current solution of adding everything from PATH to the Python search directory seems a bit brute force to me, I agree with JC that it might be better to have it opt-in.