Skip to content
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

Adsk Contrib - Allow PyOpenColorIO module to load DLLs from Windows PATH environment variable #1759

Conversation

cedrik-fuoco-adsk
Copy link
Contributor

@cedrik-fuoco-adsk cedrik-fuoco-adsk commented Jan 13, 2023

This is a re-take of the PR (from @anderslanglands) with modifications.
See Allow loading dlls from PATH on windows and python>=3.8 for context.

I made the following modifications:

  • Minor change was needed to make sure that the setup_ocio.bat and setup_ocio.sh were configured correctly.
  • Modified the comments in __init__.py.
  • Changed it to an opt-out behaviour as they did in OpenImageIO. OCIO_PYTHON_LOAD_DLLS_FROM_PATH must be set to 0 to use the default behaviour of Python 3.8+ (which is to not load the DLLs from the PATH environment variable).

Signed-off-by: Cédrik Fuoco cedrik.fuoco@autodesk.com

… variable with an opt-out option in case the user want the default behavior of Python 3.8+.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@remia
Copy link
Collaborator

remia commented Jan 19, 2023

I'm not really familiar with the Python 3.8+ Windows specifics but this looks good to me. Only thing is that I think the package layout has now changed to include a directory, so I would be curious to see if the wheel are still working fine with the current setup.py script, could you try running the wheel workflow? Maybe we should trigger a wheel workflow anytime something under src/bindings/python is changed from there? Might be too much but could be tested.

@cedrik-fuoco-adsk
Copy link
Contributor Author

cedrik-fuoco-adsk commented Jan 26, 2023

I'm not really familiar with the Python 3.8+ Windows specifics but this looks good to me. Only thing is that I think the package layout has now changed to include a directory, so I would be curious to see if the wheel are still working fine with the current setup.py script, could you try running the wheel workflow? Maybe we should trigger a wheel workflow anytime something under src/bindings/python is changed from there? Might be too much but could be tested.

I'm not familiar with the python wheel but I've tried to build it on my Macbook M1 and it seems to be built correctly. I've executed the wheel workflow manually and used pipx run cibuildwheel --platform macos. Is that the right approach?

I have tried it on my Windows setup, but it seems to be erroring out somewhere in OCIO docs. But it might just be a setup issue. I haven't found the issue so far.

@remia
Copy link
Collaborator

remia commented Jan 28, 2023

Thanks for testing @cedrik-fuoco-adsk, I think you did it correctly and we can monitor the wheel workflow nightly build in any case.

@doug-walker doug-walker merged commit f651a1a into AcademySoftwareFoundation:main Mar 9, 2023
@doug-walker doug-walker deleted the adsk_contrib/allow-PyOpenColorIO-module-to-load-dlls-from-path-on-windows branch March 9, 2023 05:57
@remia remia mentioned this pull request Mar 13, 2023
cedrik-fuoco-adsk added a commit to autodesk-forks/OpenColorIO that referenced this pull request Mar 24, 2023
…ATH environment variable (AcademySoftwareFoundation#1759)

* Allow PyOpenColorIO module to load DLLs from Windows PATH environment variable with an opt-out option in case the user want the default behavior of Python 3.8+.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>

* Fixing typos in comments

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>

---------

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants