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.

Sign up for GitHub

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

Jump to bottom

cmake: PythonInterp: Fixed a bug related to python detection #11986

Merged
merged 1 commit into from Dec 11, 2018
Merged

cmake: PythonInterp: Fixed a bug related to python detection #11986

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmake/backports/FindPythonInterp.cmake
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@ endfunction()
# and choose it if it does. This gives this executable the highest
# priority, which is expected behaviour.
find_program(PYTHON_EXECUTABLE python)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this look for an executable called python? That won't work on most systems, as python is usually Python 2.

I still get Python 3.4 on my system, even though I have 3.6 and 3.7 as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this look for an executable called python?

yes

That won't work on most systems, as python is usually Python 2.

It still works, this is only one piece in the "find python" machinery.

I still get Python 3.4 on my system, even though I have 3.6 and 3.7 as well.

What is the output of "which python", and "which python3" on your system? (And what is the versions of these outputs?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

ulf huvuddator ~ $ which python python3
/usr/bin/python
/usr/bin/python3
ulf huvuddator ~ $ python --version
Python 2.7.15+
ulf huvuddator ~ $ python3 --version
Python 3.6.7

Copy link
Collaborator

@ulfalizer ulfalizer Dec 10, 2018 •
edited

Choose a reason for hiding this comment

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

And here's what CMake says:

-- Found PythonInterp: /usr/bin/python3.4 (found suitable version "3.4.9", minimum required is "3.4")`

That's what gets used too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, I guess we should have python3 take priority over python3.4.

But I'm a bit busy at the moment, so I don't have time to implement this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[activate] will change your $PATH so its first entry is the virtualenv’s bin/ directory. (You have to use source because it changes your shell environment in-place.) This is all it does; it’s purely a convenience.

https://virtualenv.pypa.io/en/latest/userguide/?highlight=activate#activate-script
https://virtualenv.pypa.io/en/latest/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree to document it but disagree to use the latest. We can always specify what to use with -DPYTHON_EXECUTABLE. The default should be latest stable interpreter, which is whatever python3 points to. Or at least that's what I thought.

CMake version 3.12 introduced FindPython which seems to find Python 3.7 even if my python3 points to 3.6. 🤔 If we are going to use FindPython when we require new version of CMake, it might be better to behave the same way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CMake version 3.12 introduced FindPython which seems to find Python 3.7 even if my python3 points to 3.6. thinking If we are going to use FindPython when we require new version of CMake, it might be better to behave the same way?

Maybe, let's deal with that question when we get there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels a bit magic to me to assume that whatever python3 points to is what's intended to be used. Always use latest is super simple, and it's backwards compatible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is going to be the priority:

'python'
'python3'
'python3.x'

This makes sense to me. Is simple to understand, and meets the requirements described in: #11857

if(NOT (${PYTHON_EXECUTABLE} STREQUAL FIND_PY-NOTFOUND))
if(NOT (${PYTHON_EXECUTABLE} STREQUAL PYTHON_EXECUTABLE-NOTFOUND))
determine_python_version(${PYTHON_EXECUTABLE} ver)
if(${ver} VERSION_LESS PythonInterp_FIND_VERSION)
# We didn't find the correct version on path, so forget about it
Expand Down