Skip to content

Conversation

@Staudey
Copy link

@Staudey Staudey commented Feb 24, 2024

This uses the FindPython cmake module instead of the old FindPythonInterp, both to future-proof and avoid certain issues (e.g. in the Solus package build the old module lead to misdetections and build failures).

Unfortunately this means bumping the minimum cmake version to 3.12, which was released in July 2018. I'm not sure if this is an issue for this project.

@lindstro
Copy link
Member

@Staudey Thanks for your contribution. I think bumping CMake to 3.12 may be acceptable at this point. But these changes cause the MSVC tests to fail. Can you please look into what's needed to make them pass?

@Staudey
Copy link
Author

Staudey commented Feb 26, 2024

Thanks for the heads-up! I'll have to take another look at this because what I first thought the appveyor issue was turned out to be a red herring.

@lindstro
Copy link
Member

@Staudey I'm finally returning to this PR of yours now that we've ironed out several other zfpy issues. I was able to get everything to work on my end using your changes plus two others: Python_VERSION_MAJOR in FindPythonExtensions.cmake and Python_EXECUTABLE in tests/python/CMakeLists.txt.

I'm still hesitant to merge until we can figure out why the AppVeyor tests do not pass. I've not had a chance to look into this and likely won't have time to do so in the near future. Any help you can provide with that would be greatly appreciated.

@Staudey
Copy link
Author

Staudey commented Jan 23, 2025

Sorry, this PR kinda slipped my mind while I was working on other things. Thanks for the hints and I'll take another look at this soon.

@Staudey
Copy link
Author

Staudey commented Jan 24, 2025

Should be good now, CI was successful. I'll just push one more time to add the corresponding changes to the doc so it's up to date.

@lindstro
Copy link
Member

@Staudey Thank you so much for these latest updates. Before I merge, I have two comments:

First, although CMake 3.12 is already very old, since zfpy is optional and must be enabled with -DBUILD_ZFPY=ON, it may make sense to only conditionally require 3.12. That's how we're currently dealing with CUDA and HIP support on the staging branch, which both (will) require 3.23. We were already planning on bumping CMake to 3.10 (since older versions will soon be deprecated), so going to 3.12 isn't really much of a stretch. Still, it may make sense to document the reason for the 3.12 requirement along these lines: https://github.com/LLNL/zfp/blob/6e825731efd88d90f28c06ab4aa25a2705693857/CMakeLists.txt#L1-L13

Let me note that there's currently an issue with BUILD_ALL, which does not enable components until it's too late: https://github.com/LLNL/zfp/blob/8fe0059a33e298081409a520c254ebe1836bc20a/CMakeLists.txt#L287-L294

I will deal with that separately and can take care of these conditional dependencies then.

Second, the way I was able to get FindPython to load a specific version was to set Python_EXECUTABLE. For instance, FindPython would load 3.11 (which has some issues with Cython) unless I explicitly pointed the executable at version 3.9, even though python3 --version showed Python 3.9.12. Setting Python_INCLUDE_DIR and Python_LIBRARY, instead, does not work for me with FindPython (and CMake 3.23.1):

cmake .. -DBUILD_ZFPY=ON -DPython_INCLUDE_DIR=/usr/tce/packages/python/python-3.9.12/include -DPython_LIBRARY=/usr/tce/packages/python/python-3.9.12/lib
-- The C compiler identification is GNU 12.1.1
-- The CXX compiler identification is GNU 12.1.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/tce/packages/gcc/gcc-12.1.1/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/tce/packages/gcc/gcc-12.1.1/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Compiling with C standard: 99
-- Compiling with C++ standard: 98
-- Found OpenMP_C: -fopenmp (found version "4.5") 
-- Found OpenMP: TRUE (found version "4.5") found components: C 
-- Performing Test HAVE_MATH
-- Performing Test HAVE_MATH - Failed
-- Performing Test HAVE_LIBM_MATH
-- Performing Test HAVE_LIBM_MATH - Success
-- Found Python: /usr/bin/python3.11 (found version "3.11.10") found components: Interpreter

I don't know if you have a way of verifying this, but it might make sense to consolidate the two macros into one and pointing to the executable instead. I don't know what the "official" solution is, but Python_EXECUTABLE works for me.

@Staudey
Copy link
Author

Staudey commented Jan 25, 2025

I see, that all makes sense. I'm going to take another stab at this tomorrow (or the day after tomorrow, depending on how much time I can find)

find_package(NumPy REQUIRED)

include_directories(${ZFP_SOURCE_DIR}/include)
include_directories(${NumPy_INCLUDE_DIR})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
include_directories(${NumPy_INCLUDE_DIRS})

# Install to the typical python module directory
set(python_install_lib_dir "lib/python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}/site-packages/")
set(python_install_lib_dir "lib/python${Python_VERSION_MAJOR}.${Python_VERSION_MINOR}/site-packages/")
install(TARGETS zfpy LIBRARY DESTINATION ${python_install_lib_dir})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
install(TARGETS zfpy LIBRARY DESTINATION ${python_install_lib_dir})
install(TARGETS zfpy LIBRARY DESTINATION ${Python_SITELIB})


# Install to the typical python module directory
set(python_install_lib_dir "lib/python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}/site-packages/")
set(python_install_lib_dir "lib/python${Python_VERSION_MAJOR}.${Python_VERSION_MINOR}/site-packages/")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set(python_install_lib_dir "lib/python${Python_VERSION_MAJOR}.${Python_VERSION_MINOR}/site-packages/")

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.

3 participants