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

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

Open
bartlettroscoe opened this issue Jul 26, 2024 · 8 comments
Open

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented Jul 26, 2024

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
Copy link
Member Author

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
Copy link
Contributor

rppawlo commented Jul 26, 2024

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 added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 26, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Jul 26, 2024
This allows all of these Python scripts to run on more modern systems where
'python3' is the name of the Python interpreter executable.
@bartlettroscoe
Copy link
Member Author

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
Copy link
Contributor

rppawlo commented Aug 1, 2024

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
Copy link
Member Author

bartlettroscoe commented Aug 1, 2024

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
Copy link
Collaborator

ccober6 commented Aug 1, 2024

Yeah, the release just went through. :(

@bartlettroscoe
Copy link
Member Author

@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
Copy link
Contributor

rppawlo commented Aug 1, 2024

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

bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Aug 1, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Aug 1, 2024
This allows all of these Python scripts to run on more modern systems where
'python3' is the name of the Python interpreter executable.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Aug 1, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Aug 1, 2024
This allows all of these Python scripts to run on more modern systems where
'python3' is the name of the Python interpreter executable.
@bartlettroscoe bartlettroscoe moved this to Selected in TriBITS Refactor Aug 30, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
We want to use default behavior to just pick up python3 by default
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
…CUTABLE (TriBITSPub#610)

Change from calling deprecated find_package(PythonInterp) to call
find_package(Python3).  To maintain backward compatibility for external users,
set Python3_EXECUTABLE to PYTHON_EXECUTABLE if the former is unset and the
later is set.

The rest of TriBITS will need to be refactored to absorb this.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
…TriBITSPub#610)

These are reusable helper tools that we might as well snapshot with the rest
of TriBITS.  (These don't have automated tests but it is easy to verify they
are working as they should.)
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
…riBITSPub#610)

This just renames PYTHON_EXECUTABLE to Python3_EXECUTABLE.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
…LE to PYTHON_EXECUTABLE (TriBITSPub#610)

NOTE: This passes the unit tests for tribits_find_python_interp() but not all
of the TriBITS tests, yet.  These changes need to be propagated across the rest
of TriBITS.
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Oct 3, 2024
bartlettroscoe added a commit that referenced this issue Oct 9, 2024
When changing to find_package(Python3) (see #610), the variable
PYTHON_VERSION_MAJOR no longer gets set.  That caused HAS_PYTHON3 to get set
to FALSE and for the test TriBITS_sphinx_rst_generator_UnitTests to not get
enabled (but is actually disabled using property DISABLED).

NOTE: The CDash feature that reports when tests go missing caught this
problem!  (I never noticed that when doing local testing).  Here is what that
CDash email looked like:

----

Summary: [tribits-checkins] FAILED (m=1): TriBITS/TriBITS -
tribits_cmake-3.25.2_makefiles_python-3.8_g++-11 - Continuous

A submission to CDash for the project TriBITS has missing tests. You have been
identified as one of the authors who have checked in changes that are part of
this submission or you are listed in the default contact list.

Details on the submission can be found at https://my.cdash.org/build/2685504.

Project: TriBITS
SubProject: TriBITS
Site: ubuntu-latest
Build Name: tribits_cmake-3.25.2_makefiles_python-3.8_g++-11
Build Time: 2024-10-09 16:55:55
Type: Continuous
Total Missing Tests: 1

*Missing Tests*
TriBITS_sphinx_rst_generator_UnitTests (https://my.cdash.org/viewTest.php?buildid=2685505)

-CDash

----

That is such a nice feature!
bartlettroscoe added a commit that referenced this issue Oct 10, 2024
…UTABLE (#610)

This typo did not get discovered on the PR review that added this.
bartlettroscoe added a commit that referenced this issue Oct 10, 2024
…UTABLE (#610)

This typo did not get discovered on the PR review that added this.
bartlettroscoe added a commit that referenced this issue Oct 10, 2024
Now this is safer to run on a project's entire source tree and it is
documented to explain what this is for.
bartlettroscoe added a commit that referenced this issue Oct 10, 2024
Fixup for change to find_package(Python3) (#610)
bartlettroscoe pushed a commit to sebrowne/TriBITS that referenced this issue Oct 10, 2024
As per https://peps.python.org/pep-0394, it is best practice to not use
'python' in shebangs. Use the explicit `python3` entry point instead.

Signed-off-by: Samuel E. Browne <[email protected]>
bartlettroscoe pushed a commit to sebrowne/TriBITS that referenced this issue Oct 10, 2024
bartlettroscoe pushed a commit to sebrowne/TriBITS that referenced this issue Oct 10, 2024
As per https://peps.python.org/pep-0394, it is best practice to not use
'python' in shebangs. Use the explicit `python3` entry point instead.

Signed-off-by: Samuel E. Browne <[email protected]>
bartlettroscoe pushed a commit to sebrowne/TriBITS that referenced this issue Oct 10, 2024
bartlettroscoe added a commit to trilinos/Trilinos that referenced this issue Oct 10, 2024
Origin repo remote tracking branch: 'github/master'
Origin repo remote repo URL: 'github = [email protected]:TriBITSPub/TriBITS.git'
Git describe: tribits_start-3502-g28a36351

At commit:

commit 3dca096437acbb7a16b88bee7d9569bccd8e6317
Author:  Samuel E. Browne <[email protected]>
Date:    Thu Jun 6 15:26:15 2024 -0600
Summary: Add Python3 requirement to user guide (#610)

Mostly, this was done to brings in the changes for transitioning to
find_package(Python3) (see TriBITSPub/TriBITS#610).

But the full set of TriBITS PRs merged in were:

* TriBITSPub/TriBITS#608 sebrowne/python3
* TriBITSPub/TriBITS#617 TriBITSPub/610-python3-2
* TriBITSPub/TriBITS#616 bartlettroscoe/610-python3
* TriBITSPub/TriBITS#615 bartlettroscoe/enable-disable-doc-update-2024-08-29
* TriBITSPub/TriBITS#611 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright-2
* TriBITSPub/TriBITS#614 bartlettroscoe/tribits-updates-2024-08-01
* TriBITSPub/TriBITS#613 TriBITSPub/612-improve-tribits-version-error
* TriBITSPub/TriBITS#609 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
Origin repo remote tracking branch: 'github/master'
Origin repo remote repo URL: 'github = [email protected]:TriBITSPub/TriBITS.git'
Git describe: tribits_start-3502-g28a36351

At commit:

commit 3dca096437acbb7a16b88bee7d9569bccd8e6317
Author:  Samuel E. Browne <[email protected]>
Date:    Thu Jun 6 15:26:15 2024 -0600
Summary: Add Python3 requirement to user guide (trilinos#610)

Mostly, this was done to brings in the changes for transitioning to
find_package(Python3) (see TriBITSPub/TriBITS#610).

But the full set of TriBITS PRs merged in were:

* TriBITSPub/TriBITS#608 sebrowne/python3
* TriBITSPub/TriBITS#617 TriBITSPub/610-python3-2
* TriBITSPub/TriBITS#616 bartlettroscoe/610-python3
* TriBITSPub/TriBITS#615 bartlettroscoe/enable-disable-doc-update-2024-08-29
* TriBITSPub/TriBITS#611 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright-2
* TriBITSPub/TriBITS#614 bartlettroscoe/tribits-updates-2024-08-01
* TriBITSPub/TriBITS#613 TriBITSPub/612-improve-tribits-version-error
* TriBITSPub/TriBITS#609 bartlettroscoe/tril-13133-TriBITS-Update-License-and-Copyright

Signed-off-by: Roscoe A. Bartlett <[email protected]>
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
This is the result of running the script:

   cmake/tribits/refactoring/to-python3.sh

on the entire Trilinos source tree.

This just changes from "PYTHON_EXECUTABLE" to "Python3_EXECUTABLE".

NOTE: The script also fixes up then carriage-return/line-feed chars (which is
why all of the lines of the file packages/compadre/CMakeLists.txt was
changed).
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
…riBITSPub/TriBITS#610)

I don't want to assume for non-TriBIT code in Compadre to change from "python"
to "python3".
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
TriBITSPub/TriBITS#610)

We should not be touching snapshotted code that we are not even using in
Trilinos (at least I don't think we are).
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
…SPub/TriBITS#610)

There are a bunch of non-CMake or RST files in PyTrilinos that use
PYTHON_EXECUTABLE.  Not sure how all of this works but hopefully this does the
right thing.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
…iBITS#610)

Python3_EXECUTABLE will evaluate to true in Python if it is set to use.
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
This is the result of running the script:

   cmake/tribits/refactoring/to-python3.sh

on the entire Trilinos source tree.

This just changes from "PYTHON_EXECUTABLE" to "Python3_EXECUTABLE".

NOTE: The script also fixes up then carriage-return/line-feed chars (which is
why all of the lines of the file packages/compadre/CMakeLists.txt was
changed).

Signed-off-by: Roscoe A. Bartlett <[email protected]>
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
…riBITSPub/TriBITS#610)

I don't want to assume for non-TriBIT code in Compadre to change from "python"
to "python3".

Signed-off-by: Roscoe A. Bartlett <[email protected]>
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
TriBITSPub/TriBITS#610)

We should not be touching snapshotted code that we are not even using in
Trilinos (at least I don't think we are).

Signed-off-by: Roscoe A. Bartlett <[email protected]>
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
…SPub/TriBITS#610)

There are a bunch of non-CMake or RST files in PyTrilinos that use
PYTHON_EXECUTABLE.  Not sure how all of this works but hopefully this does the
right thing.

Signed-off-by: Roscoe A. Bartlett <[email protected]>
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue Oct 11, 2024
…iBITS#610)

Python3_EXECUTABLE will evaluate to true in Python if it is set to use.

Signed-off-by: Roscoe A. Bartlett <[email protected]>
bartlettroscoe added a commit to trilinos/Trilinos that referenced this issue Oct 17, 2024
bartlettroscoe added a commit to bartlettroscoe/TriBITS that referenced this issue Nov 1, 2024
…IND_VERSION (TriBITSPub#610)

I somehow missed this in the previous commit:

  3e5471d "Change to find_package(Python3), set Python3_EXECUTABLE to PYTHON_EXECUTABLE (TriBITSPub#610)"
  Author: Roscoe A. Bartlett <[email protected]>
  Date:   Thu Oct 3 12:03:52 2024 -0600 (4 weeks ago)

Signed-off-by: Roscoe A. Bartlett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Selected
Development

No branches or pull requests

3 participants