AcademySoftwareFoundation / Imath

Imath is a C++ and python library of 2D and 3D vector, matrix, and math operations for computer graphics
https://imath.readthedocs.io
BSD 3-Clause "New" or "Revised" License
356 stars 104 forks source link

Fix python module install destination #400

Open lgritz opened 1 month ago

lgritz commented 1 month ago

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

lgritz commented 1 month ago

@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?

cary-ilm commented 1 month ago

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 commented 1 month ago

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

lgritz commented 1 month ago

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?

lgritz commented 1 month ago

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?

lamiller0 commented 1 month ago

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.

lgritz commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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. https://github.com/AcademySoftwareFoundation/openexr/pull/751 11/9/20 - Kimball reenabled Windows builds and removed all python references. https://github.com/AcademySoftwareFoundation/openexr/commit/cc7b31aecd5889ffbf58f75802cdb33aabc677ed (part of 3.0 release prep PR)

lgritz commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.