kpeeters / cadabra2

A field-theory motivated approach to computer algebra.
https://cadabra.science/
GNU General Public License v3.0
230 stars 37 forks source link

Use Python_EXECUTABLE instead of PYTHON_EXECUTABLE in cmake files #309

Closed badshah400 closed 1 month ago

badshah400 commented 3 months ago

Cadabra version 2.5.4.

CMake's FindPython sets the variable Python_EXECUTABLE¹ but all CMakeLists.txt files in the cadabra source code refer to PYTHON_EXECUTABLE (wrong case). The latter seems to no longer work at all with cmake version 3.30,which is what openSUSE/Tumbleweed has, leading to lots of python not found related errors during build and, in case one gets around that by setting PYTHON_SITE_PATH explicitly during configure, when running ctest.

I ended up patching as follows to get the build to succeed. Please let me know if this is the correct thing to do and, if it is, whether you would like me to send a PR. Thanks in advance.

Index: cadabra2-2.5.4/CMakeLists.txt
===================================================================
--- cadabra2-2.5.4.orig/CMakeLists.txt
+++ cadabra2-2.5.4/CMakeLists.txt
@@ -325,7 +325,7 @@ else()
     # calling into python's 'site' package (and hoping that the 0th
     # element is where we should be writing).
     execute_process(
-      COMMAND ${PYTHON_EXECUTABLE} -c "import site; print(site.getsitepackages()[0])"
+      COMMAND ${Python_EXECUTABLE} -c "import site; print(site.getsitepackages()[0])"
       OUTPUT_VARIABLE PYTHON_SITE_PATH
       OUTPUT_STRIP_TRAILING_WHITESPACE
     )
@@ -339,7 +339,7 @@ message(STATUS "Installing Python module
 # contain the abi name. See
 # https://www.python.org/dev/peps/pep-3149/
 execute_process(
-   COMMAND ${PYTHON_EXECUTABLE} -c "import sysconfig; print(sysconfig.get_config_var('SOABI'))"
+   COMMAND ${Python_EXECUTABLE} -c "import sysconfig; print(sysconfig.get_config_var('SOABI'))"
    OUTPUT_VARIABLE PYTHON_SOABI
    OUTPUT_STRIP_TRAILING_WHITESPACE
    )
@@ -350,7 +350,7 @@ set(PYTHON_SITE_PATH_REL ${PYTHON_SITE_P

 if(NOT WIN32)
   execute_process(
-    COMMAND ${PYTHON_EXECUTABLE} -c "import site; print (site.getsitepackages()[0]);"
+    COMMAND ${Python_EXECUTABLE} -c "import site; print (site.getsitepackages()[0]);"
     OUTPUT_VARIABLE OLDER_PYTHON_SITE_PATH
     OUTPUT_STRIP_TRAILING_WHITESPACE
     )
Index: cadabra2-2.5.4/core/cadabra2.in
===================================================================
--- cadabra2-2.5.4.orig/core/cadabra2.in
+++ cadabra2-2.5.4/core/cadabra2.in
@@ -1,4 +1,4 @@
-#!${PYTHON_EXECUTABLE}
+#!${Python_EXECUTABLE}
 #
 # \ingroup pythoncore
 #
@@ -119,8 +119,8 @@ if __name__ == '__main__':

     if len(sys.argv)>1:
         if '-d' in sys.argv:
-            #rs = "lldb -ex r --args ${PYTHON_EXECUTABLE} "+sys.argv[0];
-            rs = "gdb -q -ex r --args ${PYTHON_EXECUTABLE} "+sys.argv[0];
+            #rs = "lldb -ex r --args ${Python_EXECUTABLE} "+sys.argv[0];
+            rs = "gdb -q -ex r --args ${Python_EXECUTABLE} "+sys.argv[0];
             for a in sys.argv[1:]:
                 if a!='-d':
                     rs += " "+a
Index: cadabra2-2.5.4/jupyterkernel/kernelspec/kernel.json.in
===================================================================
--- cadabra2-2.5.4.orig/jupyterkernel/kernelspec/kernel.json.in
+++ cadabra2-2.5.4/jupyterkernel/kernelspec/kernel.json.in
@@ -1,6 +1,6 @@
 {
   "argv": [
-    "@PYTHON_EXECUTABLE@", "-m", "cadabra2_jupyter", "-f", "{connection_file}"
+    "@Python_EXECUTABLE@", "-m", "cadabra2_jupyter", "-f", "{connection_file}"
   ],
   "display_name":"Cadabra2",
   "language":"python"
Index: cadabra2-2.5.4/tests/CMakeLists.txt
===================================================================
--- cadabra2-2.5.4.orig/tests/CMakeLists.txt
+++ cadabra2-2.5.4/tests/CMakeLists.txt
@@ -49,7 +49,7 @@ foreach(NBTEST ${NBTESTS})
     ARGS    ${CNBIN}/${NBTEST}.cnb ${CDBOUT}/${NBTEST}.cdb
     COMMENT "Creating ${NBTEST} notebook...")
   add_custom_target("${NBTEST}_test" ALL DEPENDS ${CDBOUT}/${NBTEST}.cdb)
-  add_test(${NBTEST}  ${PYTHON_EXECUTABLE} ${CMAKE_SOURCE_DIR}/core/cadabra2 ${CDBOUT}/${NBTEST}.cdb)
+  add_test(${NBTEST}  ${Python_EXECUTABLE} ${CMAKE_SOURCE_DIR}/core/cadabra2 ${CDBOUT}/${NBTEST}.cdb)
   set_tests_properties(${NBTEST}      PROPERTIES ENVIRONMENT "PYTHONPATH=${PYTHONPATH};LANG=en_US.UTF-8;LC_ALL=en_US.UTF-8;PYTHONIOENCODING=utf-8")    
 endforeach()

@@ -100,7 +100,7 @@ if(MATHEMATICA_FOUND)
   set(RTESTS ${RTESTS} mma)
 endif()
 foreach(RTEST ${RTESTS})
-  add_test(${RTEST}  ${PYTHON_EXECUTABLE} ${CMAKE_SOURCE_DIR}/core/cadabra2 ${CMAKE_SOURCE_DIR}/tests/${RTEST}.cdb)
+  add_test(${RTEST}  ${Python_EXECUTABLE} ${CMAKE_SOURCE_DIR}/core/cadabra2 ${CMAKE_SOURCE_DIR}/tests/${RTEST}.cdb)
   # We need to set the Python path so that we pick up the correct cadabra2.so Python module
   # even if we did not do 'make install' yet.
   set_tests_properties(${RTEST}      PROPERTIES ENVIRONMENT "PYTHONPATH=${PYTHONPATH};LANG=en_US.UTF-8;LC_ALL=en_US.UTF-8;PYTHONIOENCODING=utf-8")    

¹ https://cmake.org/cmake/help/latest/module/FindPython.html

kpeeters commented 3 months ago

Ah, thanks, that solves some Python mysteries (or PYTHON mysteries, or PyThOn mysteries)... The above is sufficient, I'll merge it in.

kpeeters commented 3 months ago

I have pushed these changes (along with a few others related to PYTHON -> Python) to the devel branch, can you give that a shot and let me know if anything is still not right?

badshah400 commented 2 months ago

Hi, Sorry for the delay. I have been testing commit 15bb0214dd790ccdfe42c36f9cc4d76b2e5c52aa but there seems to be an issue still and cmake errors out at the configure stage:

[   31s] -- Found Python: /usr/bin/python3.11 (found version "3.11.9") found components: Interpreter Development Development.Module Development.Embed
[   31s] -- Performing Test HAS_FLTO
[   31s] -- Performing Test HAS_FLTO - Success
[   31s] -- Found pybind11: /usr/include (found version "2.12.0")
[   31s] -- Found python
[   31s] -- Python version is ..
[   31s] -- Installing Python modules in
[   31s] -- Python abi name
[   31s] CMake Error at CMakeLists.txt:348 (string):
[   31s]   string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
[   31s]   command.
[   31s]
[   31s]
[   31s] -- Python module extension .so

This is with Python 3.11.

kpeeters commented 2 months ago

@badshah400 That last error seems to combine the devel branch with your patch in https://github.com/kpeeters/cadabra2/issues/310. In any case, the fact that nothing is displayed in the 'Found python' is weird. This should display the python library; on my machine this line reads

-- Found python /usr/lib/aarch64-linux-gnu/libpython3.10.so

This string and other Python-related variables seem to be all empty for you, which is why that string REGEX REPLACE fails.

Just so we are on the same page, can you check the current devel branch once more and let me know if you still get that error above? This works for me with CMake 3.30.2.

kpeeters commented 1 month ago

I think this is now all correct, and 2.5.6 introduces some other simplifications to the Python logic which means that the cadabra2 executables do no longer have hardcoded absolute paths in them. I will close this for now, but please re-open if you still think there is a problem.

badshah400 commented 1 month ago

Awesome, thanks. Sorry, I have not had enough time to test recent commits but shall try the new release this week and let you know if I run into any issues.