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

ENH: When using cmake > 3.12 use FindPython3 module #1536

Merged

Conversation

hjmjohnson
Copy link
Member

@hjmjohnson hjmjohnson commented Jan 6, 2020

The FindPython3 module is more robust for ensuring that python 3 or greater is found.

The FindPythonInterp and FindPythonLib modules are deprecated
in cmake greater than 3.12.0

The GoogleTest framework uses the deprecated FindPythonInterp, so this
the patch uses the FindPython3 module, and then uses that more robust
information to set the FindPythonInterp cmake variables so that
wrapping, google tests, documentation generation, etc.. all use the
same found a version of Python 3.

PR Checklist

  • [Makes design changes]

@hjmjohnson hjmjohnson self-assigned this Jan 6, 2020
The FindPython3 module is more robust for ensuring that python 3 or greater is found.

The FindPythonInterp and FindPythonLib modules are deprecated
in cmake greater than 3.12.0

The GoogleTest framework uses the deprecated FindPythonInterp, so this
patch uses the FindPython3 module, and then uses that more robust
information to set the FindPythonInterp cmake variables so that
wrapping, google tests, documentation generation, etc.. all use the
same found version of Python 3.
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good

@hjmjohnson hjmjohnson merged commit 58d0016 into InsightSoftwareConsortium:master Jan 7, 2020
@blowekamp
Copy link
Member

blowekamp commented Jan 7, 2020

I am now getting a configuration error on my Mac system because it only finds py2 and not py3. Python 3 is not required for the Doxygen, or HeaderTests. OSX, and and many linux based systems still come with Python 2 by default. And this patch breaks the builds.

@hjmjohnson
Copy link
Member Author

I am now getting a configuration error on my Mac system because it only finds py2 and not py3. Python 3 is not required for the Doxygen, or HeaderTests. OSX, and and many linux based systems still come with Python 2 by default. And this patch breaks the builds.

@blowekamp I thought that it was already established that we would only support Python3 (for wrapping at least). From a maintenance standpoint, it would be easier if we simply require python3 for any use of python in ITK. I think that Doxygen and HeaderTests are both "developer-level" features that are OK to require Python3 (even if they don't strictly need them). I know we could provide support for python2, but I want to ensure that the extra complexity and maintenance burden will be worth that level of complexity. (If Doxygen and not wrapping, then python2, else python3 is required).

My motivation for this change is that during the configuration of cmake with ITK I was finding that several versions of Python were being found and in some cases overwriting the intended version. I found it really confusing when multiple versions of the python executable were identified for different parts of the code base.

I'm willing to re-add python2 support for developer builds that only use HeaderTests or Doxygen tests if that is the consensus, but I think it is important to have a conversation regarding the pros/cons of the added complexity. Let me know what you think.

Hans

@blowekamp
Copy link
Member

On a new configuration this is the error I'm getting:

CMake Error at /Users/blowekamp/Applications/CMake.app/Contents/share/cmake-3.14/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter)
Call Stack (most recent call first):
  /Users/blowekamp/Applications/CMake.app/Contents/share/cmake-3.14/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /Users/blowekamp/Applications/CMake.app/Contents/share/cmake-3.14/Modules/FindPython/Support.cmake:1170 (find_package_handle_standard_args)
  /Users/blowekamp/Applications/CMake.app/Contents/share/cmake-3.14/Modules/FindPython3.cmake:174 (include)
  CMake/ITKModuleDoxygen.cmake:23 (find_package)
  CMake/ITKModuleMacros.cmake:6 (include)
  CMakeLists.txt:142 (include)

I don't even have Doxygen enabled.

The options are:

  1. When python 2 or no python is detect, gracefully disable the "developer-level" features.
  2. Allow python 2 and python3 for the developer-level feature. This would require changing from CMake "FindPython3" to "FindPython".

@hjmjohnson
Copy link
Member Author

@blowekamp could you please try the following:
At line 547 of the primary CMakeLists.txt file: Remove "REQUIRED" from the find_package(Python3 COMPONENTS Interpreter REQUIRED). I think that was a mistake.

We would also need to add 'if(Python3_Interpreter_FOUND ` to line 550.

@hjmjohnson
Copy link
Member Author

@blowekamp I'm working on a solution. it is almost ready for option #1 given above. I've removed python3 from one of my (slower) computers and I am testing the build now.

@hjmjohnson hjmjohnson deleted the force-use-of-python3 branch February 8, 2020 20:11
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