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

Several issues when building PyImath #101

Closed
waebbl opened this issue Feb 21, 2021 · 19 comments
Closed

Several issues when building PyImath #101

waebbl opened this issue Feb 21, 2021 · 19 comments
Labels

Comments

@waebbl
Copy link
Contributor

waebbl commented Feb 21, 2021

I've encountered a few issues, when building Imath with Python support enabled.

  • The imathnumpy.so module has no executable bit set, although in the build directory the bit is set:
$ ls -l /var/tmp/portage/dev-libs/imath-9999/work/imath-9999_build/python3_8/
total 5836
-rwxr-xr-x 1 waebbl waebbl 5916200 Feb 21 21:15 imath.so
-rwxr-xr-x 1 waebbl waebbl   56872 Feb 21 21:15 imathnumpy.so

vs.

$ ls -l /var/tmp/portage/dev-libs/imath-9999/image/usr/lib/python3.8/site-packages/
total 4092
-rwxr-xr-x 1 waebbl waebbl 4148352 Feb 21 21:23 imath.so
-rw-r--r-- 1 waebbl waebbl   39920 Feb 21 21:23 imathnumpy.so
##
## SPDX-License-Identifier: BSD-3-Clause
## Copyright Contributors to the OpenEXR Project.
##

prefix=/usr
exec_prefix=bin
libdir=lib64
includedir=include
libsuffix=

Name: PyImath
Description: Python bindings for the Imath libraries
Version: 3.0.0
Libs: -L${libdir} -lImath${libsuffix} -lPyImath${libsuffix}
Cflags: -I${includedir} -I${includedir}/Imath

For details, please see the attached build log. System is Gentoo Linux, gcc-10.2.0, Python-3.8.8, boost-1.75.0, numpy-1.19.5. If you need any more information, don't hesitate to ping me.

build.log

@cary-ilm
Copy link
Member

cary-ilm commented Feb 21, 2021 via email

@waebbl
Copy link
Contributor Author

waebbl commented Feb 22, 2021

I'm sorry, I was probably wrong with the first bullet above. Removal of the executable bit is more likely to be done by the package manager routine I use to install the package from the build stage to the (sandboxed) final install location.
Yet the imathnumpy.so doesn't get installed at all by cmake's install call.

@meshula
Copy link
Contributor

meshula commented Feb 22, 2021

We set VERSION here, to PYIMATH_LIB_VERSION, but I haven't spotted where we set PYIMATH_LIB_VERSION. I wonder if that's the root cause of the problem.

VERSION ${PYIMATH_LIB_VERSION}

@waebbl
Copy link
Contributor Author

waebbl commented Feb 22, 2021

That's the only appearance of PYIMATH_LIB_VERSION in the code. Shouldn't the soversion of Imath and PyImath match anyway? Maybe this PYIMATH_LIB_VERSION could be replaced by IMATH_LIB_VERSION from https://github.com/AcademySoftwareFoundation/Imath/blob/master/config/version.cmake#L13?

@meshula
Copy link
Contributor

meshula commented Feb 22, 2021

We set VERSION here, to PYIMATH_LIB_VERSION, where do we set PYIMATH_LIB_VERSION?

VERSION ${PYIMATH_LIB_VERSION}

We specify the following for pymathnumpy:

  set_target_properties(imathnumpy_python3 PROPERTIES
    LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/python${Python3_VERSION_MAJOR}_${Python3_VERSION_MINOR}/"
    LIBRARY_OUTPUT_NAME "imathnumpy"
    DEBUG_POSTFIX ""
  )

did the library appear in an unexpected place, or not at all?

@meshula meshula added the build label Feb 22, 2021
@waebbl
Copy link
Contributor Author

waebbl commented Feb 22, 2021

did the library appear in an unexpected place, or not at all?

Not at all

$ find /var/tmp/portage/dev-libs/imath-9999/image -name 'imath*so'
/var/tmp/portage/dev-libs/imath-9999/image/usr/lib/python3.8/site-packages/imath.so

Searching for *numpy* reveals no result.
Currently, I install it manually, using a helper function provided for packagers.

You set the target properties, but IIRC there's no install called for the PyIMathNumpy subdir.

@waebbl
Copy link
Contributor Author

waebbl commented Feb 22, 2021

We set VERSION here, to PYIMATH_LIB_VERSION, where do we set PYIMATH_LIB_VERSION?

Grepping the code, I found no other appearance of the symbol. It's also not in the CMakeCache.txt file, so maybe it's not set at all?
I only found the https://github.com/AcademySoftwareFoundation/Imath/blob/master/config/version.cmake which defines IMATH_LIB_VERSION, but there's no equivalent file for the python bindings.

@cary-ilm
Copy link
Member

Separate version variables for Imath and PyImath is a relic of the earlier organization where IlmBase and PyIlmBase were packaged independently, we should consolidate that into a single variable.

@meshula
Copy link
Contributor

meshula commented Feb 22, 2021

Cary, is this work: AcademySoftwareFoundation/openexr#929 the corresponding work on the OpenEXR side to consolidating the variables?

@waebbl
Copy link
Contributor Author

waebbl commented Feb 22, 2021

There's also some redundancy in the PyImath related cmake files. Settings for c++ standard, installation of pkg-config files, LIB_SUFFIX, OUTPUT_SUBDIR, rpath settings, build and configuration types and OPENEXR_USE_CLANG_TIDY are repeated in Imath/src/python/config/PyImathSetup.cmake from Imath/config/ImathSetup.cmake

@meshula
Copy link
Contributor

meshula commented Feb 22, 2021

There's no reason they can't share a common setup.

@cary-ilm
Copy link
Member

@meshula, yes, #929 also cleans up some remnants of the previous organization. I'll also look into cleaning up the *Setup.cmake files, these can be simplified.

@waebbl
Copy link
Contributor Author

waebbl commented Feb 22, 2021

If you like, I can look into some of the cmake related topics.

waebbl added a commit to waebbl/Imath that referenced this issue Feb 23, 2021
waebbl added a commit to waebbl/Imath that referenced this issue Feb 23, 2021
waebbl added a commit to waebbl/Imath that referenced this issue Feb 24, 2021
waebbl added a commit to waebbl/Imath that referenced this issue Feb 25, 2021
cary-ilm pushed a commit that referenced this issue Feb 25, 2021
* clean up c++ standard

Signed-off-by: Bernd Waibel <[email protected]>

* clean-up pkg-config variables for PyImath

Signed-off-by: Bernd Waibel <[email protected]>

* clean-up output dir for PyImath

Signed-off-by: Bernd Waibel <[email protected]>

* clean redundant CMAKE_INCLUDE_CURRENT_DIR

Signed-off-by: Bernd Waibel <[email protected]>

* clean-up redundant rpath settings in PyImath

Signed-off-by: Bernd Waibel <[email protected]>

* clean-up redundant cmake build type setting

Signed-off-by: Bernd Waibel <[email protected]>

* clean-up redundant search for clang-tidy

Signed-off-by: Bernd Waibel <[email protected]>

* clean-up version and soversion for PyImath

Signed-off-by: Bernd Waibel <[email protected]>

* fix pkg-config file for PyImath

Bug: #101
Signed-off-by: Bernd Waibel <[email protected]>

* install imathnumpy.so module

Signed-off-by: Bernd Waibel <[email protected]>
@meshula
Copy link
Contributor

meshula commented Mar 4, 2021

@waebbl Is this issue, mentioned in PR #102, addressed in another commit, or is it still to be fixed? "Still todo is the -lPyImath${libsuffix} entry in PyImath.pc.in, which needs to be covered by the python variable changes as well."

@waebbl
Copy link
Contributor Author

waebbl commented Mar 4, 2021

@meshula As far as I can see it still needs to be fixed. I just pulled the latest commits, since PR #102 has been merged. They don't contain any changes to the python selection. To solve this issue, the separation to build for only one python version has to be done, which seems not the case so far.

I was thinking about trying to do this, after the PR got approved, but was distracted by other projects since then.

@meshula
Copy link
Contributor

meshula commented Mar 4, 2021

Ok, thanks for the notes.

@meshula
Copy link
Contributor

meshula commented Mar 14, 2021

@waebbl The separation has now been committed ~ if you are still thinking of trying to do the libsuffix bit, I think there's nothing blocking it.

@waebbl
Copy link
Contributor Author

waebbl commented Mar 14, 2021

@meshula
Copy link
Contributor

meshula commented Mar 14, 2021

Great, thanks for the confirmation.

@meshula meshula closed this as completed Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants