cmake: PythonInterp: Fixed a bug related to python detection #11986
Conversation
Fixed zephyrproject-rtos#11980, a bug where a build failure occured when 'python' was not on path. There was a bug in the implementation where it failed to detect that 'python' was not on path. This is now fixed. Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
SebastianBoe
added
bug
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
[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/
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Codecov Report
@@ Coverage Diff @@
## master #11986 +/- ##
=======================================
Coverage 48.13% 48.13%
=======================================
Files 281 281
Lines 43425 43425
Branches 10406 10406
=======================================
Hits 20902 20902
Misses 18368 18368
Partials 4155 4155Continue to review full report at Codecov.
|
|
@carlescufi , @nashif : This is a trivial bugfix that is affecting users, would be good to get it in quickly. |
Fixed #11980, a bug where a build failure occured when 'python' was
not on path.
There was a bug in the implementation where it failed to detect that
'python' was not on path. This is now fixed.
Signed-off-by: Sebastian Bøe sebastian.boe@nordicsemi.no