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

cmake: FindPythonInterp: Prioritize 'python' from path the highest #11907

Merged

Conversation

SebastianBoe
Copy link
Collaborator

Users expect that not just "some" compatible python is detected, but
also that it will be the 'python' executable from PATH, at least when
this executable is valid.

To this end rewrite FindPythonInterp to give this executable the
highest priority.

This fixes #11857

Signed-off-by: Sebastian Bøe sebastian.boe@nordicsemi.no

@SebastianBoe SebastianBoe requested a review from nashif as a code owner December 6, 2018 14:02
@codecov-io
Copy link

codecov-io commented Dec 6, 2018

Codecov Report

Merging #11907 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11907   +/-   ##
=======================================
  Coverage   48.16%   48.16%           
=======================================
  Files         281      281           
  Lines       43355    43355           
  Branches    10391    10391           
=======================================
  Hits        20883    20883           
  Misses      18318    18318           
  Partials     4154     4154

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a8d4d3...165302b. Read the comment docs.

Copy link
Collaborator

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Can we just error out if sys.version_info doesn't exist instead? It was added in Python 2.0, released about 18 years ago.

Would make this code a lot easier to read for CMake noobs like me, without any drawbacks in practice.

@@ -36,6 +36,45 @@
# get the currently active Python version by default with a consistent version
# of PYTHON_LIBRARIES.

function(determine_python_version exe result)
execute_process(COMMAND "${exe}" -c
"import sys; sys.stdout.write(';'.join([str(x) for x in sys.version_info[:3]]))"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can use a generator expression: ';'.join(str(x) for x in foo)' works the same as ';'.join([str(x) for x in foo]).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the ; that are then replaced with . by the way? A comment might be nice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can use a generator expression: ';'.join(str(x) for x in foo)' works the same as ';'.join([str(x) for x in foo]).

This is out-of-scope of the PR. This code has only been moved, not modified, so it's contents are not up for review.

Users expect that not just "some" compatible python is detected, but
also that it will be the 'python' executable from PATH, at least when
this executable is valid.

To this end rewrite FindPythonInterp to give this executable the
highest priority.

This fixes zephyrproject-rtos#11857

Signed-off-by: Sebastian Bøe <sebastian.boe@nordicsemi.no>
@SebastianBoe SebastianBoe force-pushed the fix_python_autodetection branch from a66ab78 to 165302b Compare December 7, 2018 12:40
@SebastianBoe
Copy link
Collaborator Author

Can we just error out if sys.version_info doesn't exist instead? It was added in Python 2.0, released about 18 years ago.

Would make this code a lot easier to read for CMake noobs like me, without any drawbacks in practice.

Fixed.

Copy link
Collaborator

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Code for ancient Python versions removed

@nashif nashif merged commit 9de5bc9 into zephyrproject-rtos:master Dec 7, 2018
@SebastianBoe
Copy link
Collaborator Author

Can we just error out if sys.version_info doesn't exist instead? It was added in Python 2.0, released about 18 years ago.
-- @ulfalizer

@ulfalizer : Evidently, users have 19 year old python versions installed: #12066 (comment)

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.

cmake does not detect most recent python, it is fixed somehow to 3.4.x
4 participants