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

Fix python module install destination #400

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lgritz
Copy link
Contributor

@lgritz lgritz commented May 10, 2024

We were installing python modules in the wrong locations (and double installing pyimath).

  • Get rid of the double install.
  • Make the one install go to the right destination: <install>/lib/pythonX.Y/site-packages
  • With that in place, we can restore the EXPORT that makes the python module show up as a target in the exported cmake config.

We were installing python modules in the wrong locations (and double
installing pyimath).

* Get rid of the double install.
* Make the one install go to the right destination:
  `<install>/lib/pythonX.Y/site-packages`
* With that in place, we can restore the EXPORT that makes
  the python module show up as a target in the exported cmake config.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz lgritz requested a review from cary-ilm May 10, 2024 06:48
@lgritz
Copy link
Contributor Author

lgritz commented May 10, 2024

@lamiller0 Can you please try this on your end and test if it installs the libraries in the right locations AND provides you with the expected target in the exported cmake?

@lgritz lgritz added build python Pull requests that update Python code labels May 10, 2024
@cary-ilm
Copy link
Member

The CI has a test that is supposed to validate this location, looks like that needs to be updated along with the change:

          # Confirm the python module loads. Query the site-packages directory and substitute ../_install
          export PYTHONPATH=`python -c "import site; print('../_install%s' % site.USER_SITE[len(site.USER_BASE):])"`          
          python -c "import imath;print(imath.__version__)"

This is attempting to set the path to the site-packages directory underneath the _install directory. I'm not sure the right way to get that location.

@lgritz
Copy link
Contributor Author

lgritz commented May 10, 2024

I think I don't have it quite right, will revise

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Contributor Author

lgritz commented May 11, 2024

Question for @lamiller0 : Is the only reason you needed the target in the first place that the python module was given a nonstandard name and location? If it followed the usual conventions, would you no longer need the target in the cmake exports?

Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Contributor Author

lgritz commented May 11, 2024

Does anybody know if there is a reason that Imath's CI only builds (let alone tests) python on Linux, so we aren't currently even trying on Windows or Mac in our CI? @xlietz do you happen to remember how this came about?

@@ -48,5 +50,7 @@ if(TARGET Python3::Python AND
LIBRARY_OUTPUT_NAME "imathnumpy"
DEBUG_POSTFIX ""
)
install(TARGETS imathnumpy_python3 DESTINATION ${PyImath_Python3_SITEARCH_REL})
install(TARGETS imathnumpy_python3
DESTINATION "${CMAKE_INSTALL_LIBDIR}/python${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR}/site-packages/Imath"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this path is the right one on Windows... It is ok for MinGW though (same as *nix). See also AcademySoftwareFoundation/OpenColorIO#1724

@lamiller0
Copy link
Contributor

Question for @lamiller0 : Is the only reason you needed the target in the first place that the python module was given a nonstandard name and location? If it followed the usual conventions, would you no longer need the target in the cmake exports?

Is it considered non-standard to embed the python major and minor version as part of the package? I suspect this was done as a way to accomodate multiple python versions installed on the same machine.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
@lgritz
Copy link
Contributor Author

lgritz commented May 13, 2024

Is it considered non-standard to embed the python major and minor version as part of the package? I suspect this was done as a way to accomodate multiple python versions installed on the same machine.

Well, this is my dilemma -- in this PR, I seem to have slowly backed myself into a thicket of issues outside the range of my skill set. There is a degree to which I'm just pushing the peas around on the plate until it resembles what OIIO does for its layout of python components. But of course, all I can say about that is that nobody seems to be complaining, which is definitely a weaker statement than that I know how it's supposed to be in modern Python best practices. It doesn't help that I'm pretty ok with pybind11, but my boost python knowledge is lacking.

Additionally, I found out in the process that Imath's CI for Windows and Mac both have for a long time (forever?) disabled the building of Python bindings entirely, so that seems like a recipe for more problems, but I'm struggling a bit with getting the CI fixed up to know if my changes are working on all platforms, but big changes to the CI setup wasn't my original goal, and also is confounded by the fact that CI tests that weren't building the python bindings also wasn't setting up the python and boost (because boost-python) dependencies and those aren't quite as smooth as I wanted either. (Aside: boy would it have been nice to approach this set of fixes with boost gone and the switch to pybind11 complete.)

I set out to take your 1-line fix and add like 3 more lines to fix the fact that the file was going to the wrong directory. But it's all kind of snowballed out of control. Am I one more little fix from wrapping it up, or am I sinking deeper into the rabbit hole? I would really love to turn this over entirely to somebody with more python binding experience and the knowledge and desire to get this whole thing right.

@xlietz
Copy link
Contributor

xlietz commented May 13, 2024

@lgritz sorry I missed the notification on this - yes the CI is intentionally NOT building the python bindings on Windows and Mac because of the frustrating Boost dependency. On Linux, Boost is installed in the docker image. But on Windows and Mac we need to get it from somewhere else. Originally I was downloading it from the boost.org in the CI but because it's so large, it seemed to be contributing to an overflow of the Boost host's download caps. We had some discussion about it at the time (I will try to find it) but ultimately there was never a resolution about how to create an image of boost for the Windows and Mac builds.

@xlietz
Copy link
Contributor

xlietz commented May 14, 2024

For historical context:
6/8/20 - Chris Horvath reported the boost downloads issue to the TAC on June 8, 2020 which he had seen on Boost's message boards and github repo.
6/9/20 - I disabled the Windows builds on June 9 and had planned to get boost from sourceforge but I never succeeded in getting that to work. AcademySoftwareFoundation/openexr#751
11/9/20 - Kimball reenabled Windows builds and removed all python references. AcademySoftwareFoundation/openexr@cc7b31a (part of 3.0 release prep PR)

@lgritz
Copy link
Contributor Author

lgritz commented May 14, 2024

For OIIO, I use boost on all 3 platforms, so I know it can be done and is not a big deal. (With the caveat that I just use some regular boost components, not boost python! OIIO has been pybind11 for many years.) I get Boos via Homebrew on Mac and VcPkg on Windows. The VcPkg build does add a lot of time to the CI, but even if we need to disable it later, I think it's vital that we get it working to test these changes (which I'm still failing at, ugh).

@xlietz
Copy link
Contributor

xlietz commented May 14, 2024

That's great to know it's working on the OIIO CI! I can take a look at it this weekend too if you think it would help.

@lgritz
Copy link
Contributor Author

lgritz commented May 15, 2024

That's great to know it's working on the OIIO CI! I can take a look at it this weekend too if you think it would help.

Wow, yeah, I would love any help you could give. I feel like I'm in above my head on this.

Feel free, also, to start with my patch (I think that having commit privileges, you are able to push onto my branch from this PR), maybe we can collaboratively get there in one joint PR.

I'm going to continue to tinker if I have free time, so there's some chance I will have it mostly done before the weekend, but even if that's the case, I would sure like you to look it over and make sure you think it's ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants