TriBITSPub / TriBITS

TriBITS: Tribal Build, Integrate, and Test System,
http://tribits.org
Other
36 stars 46 forks source link

Switch from find_package(PythonInterp) to find_package(Python3) #610

Open bartlettroscoe opened 1 month ago

bartlettroscoe commented 1 month ago

CC: @sebrowne, @ccober6, @rppawlo

Parent Epic:

Description

The TriBITS module TribitsFindPythonInterp.cmake (created by Bill Spotz back in the day) currently calls find_package(PythonInterp) which uses the module FindPythonInterp.cmake. According to:

the FindPythonInterp.cmake module has been deprecated since CMake 3.12! This should be updated to use FindPython.cmake or FindPython3.cmake.

In fact, it seems that the old FindPythonInterp.cmake module can't find versions of Python 3.0 and greater (i.e. with the name python3). This causes problems (e.g. see #607).

The new FindPython3.cmake module returns a variable called Python3_EXECUTABLE while the old FindPythonInterp.cmake module returned a variable called PYTHON_EXECUTABLE. Therefore, you can't just swap calling find_package(PythonInterp ...) with find_package(Python3 ...). Doing so would break all of the existing configure scripts for TriBITS projects that are currently setting -DPYTHON_EXECUTABLE=$(which python3) and it would break all of the internal TriBITS code that keys off of PYTHON_EXECUTABLE. So this would be a major break in backward compatibility (but the changes would be easy to absorb on both ends).

NOTE: We could make it easier for users of TriBITS projects by putting in a check for a cache var PYTHON_EXECUTABLE and error out telling users that they must instead set Python3_EXECUTABLE. Then, we could make it easier for TriBITS projects using PYTHON_EXECUTABLE in internal CMake code by setting it to an error value like PYTHON_EXECUTABLE is no longer supported! Please use Python3_EXECTUABLE instead!. And we would put in a variable_watch(PYTHON_EXECUTABLE) to and execute and error if the project tried to set PYTHON_EXECUTABLE.

Workarounds

The current workaround is to just have users manually set:

-DPYTHON_EXECUTABLE=$(which python3)

But that is not great.

bartlettroscoe commented 1 month ago

NOTE: It appears that all of the Trilinos GenConfig PR builds are explicitly setting -DPYTHON_EXECUTABLE=$(which python3) (which is computed in a more verbose way). See:

https://github.com/trilinos/Trilinos/blob/1eab15637f2998d1e86fe127b78200f2c9687cb5/packages/framework/ini-files/config-specs.ini#L1805

rppawlo commented 1 month ago

I think it's a good thing to make the switch a soon as we can. Looks like pretty easy fixes on the trilinos side. We'll bring it up at the next leaders meeting to see if there are any worries. Thanks @bartlettroscoe !

bartlettroscoe commented 1 month ago

Related to this, I am already switching over all of the TriBITS Python scripts from:

#!/usr/bin/env python

to

#!/usr/bin/env python3

That should be 100% robust on all newer machines where 'python3' is the expected standard.

rppawlo commented 1 month ago

This was discussed at the last Trilinos leaders meeting. No one had any worries. We would just like to send out an email to the application teams and trilinos lists warning them of the change to make sure they are ready for it.

bartlettroscoe commented 1 month ago

Is there a desire to do this before the Trilinos 16.0 release? That would be a good time to do this since you are allowed to break backward compatibility with major releases.

Update: Wait, we are too late. Trilinos 16.0 was released on 7/22/2024?

ccober6 commented 1 month ago

Yeah, the release just went through. :(

bartlettroscoe commented 1 month ago

@rppawlo, @ccober6

Wait, we are too late. Trilinos 16.0 was released on 7/22/2024?

Yeah, the release just went through. :(

Then making this change now would technically break backward compatibility and violate sematic versioning.

However, it would not be too hard to change to use find_package(Python3) internally and then provide some backward compatibility for those users who are manually setting:

-D PTYHON_EXECUTAGBLE=<some-path>

where we would set that to Python3_EXECUTABLE before calling find_package(Python3).

So we would change from find_package(PythonInterp) to find_package(Python3) and update all of the internal TriBITS and Trilinos code to switch from PYTHON_EXECUTABLE to Python3_EXECUTABLE. (And other TriBITS projects like Charon2 and Drekar would have to make this change as well.) This would break backward compatibility for TriBITS but not for external CMake project users.

So here is what I propose:

  1. If PTYHON_EXECUTAGBLE is set in the cache by the user, then set it to the cache var Python3_EXECUTABLE and call message(WARNING "PYTHON_EXECUTABLE is set to <some-path> which is deprecated! Please set Python3_EXECUTABLE instead!")
  2. Switch TriBITS to call find_package(Python3) instead of find_package(PythonInterp)
  3. Refactor all TriBITS CMake code to use Python3_EXECUTABLE instead of PYTHON_EXECUTABLE
  4. Refactor all Trilinos CMake code to use Python3_EXECUTABLE instead of PYTHON_EXECUTABLE
  5. All other TriBITS-based projects (e.g. Charon2, Drekar, etc.) would need to change to use Python3_EXECUTABLE instead of PYTHON_EXECUTABLE
  6. Customers configuring with -D PTYHON_EXECUTAGBLE=<some-path> with updated Trilinos develop would see the warning could change to set -D Python3_EXECUTABLE=<some-path> to make the warning go away.
  7. When Trilinos 17.0 comes out, you change message(WARNING ...) to message(FATAL_ERROR ...) when a user sets -D PYTHON_EXECUTABLE=<some-path>.

Does that sound like a good plan?

rppawlo commented 1 month ago

Sounds good to me @bartlettroscoe ! Thanks for working on this!