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

cmake: Simplify python finding #65995

Closed
wants to merge 1 commit into from
Closed

cmake: Simplify python finding #65995

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 47 additions & 34 deletions cmake/modules/python.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,58 @@

include_guard(GLOBAL)

message("Finding python interpreter using CMake's FindPython,
if the incorrect interpreter is found setting Python_EXECUTABLE
will explicitly specify the correct interpreter to use")

# On Windows, instruct Python to output UTF-8 even when not
# interacting with a terminal. This is required since Python scripts
# are invoked by CMake code and, on Windows, standard I/O encoding defaults
# to the current code page if not connected to a terminal, which is often
# not what we want.
if (WIN32)
set(ENV{PYTHONIOENCODING} "utf-8")
set(PYTHON_MINIMUM_REQUIRED 3.8)

find_package(Deprecated COMPONENTS PYTHON_PREFER)

if(NOT DEFINED Python3_EXECUTABLE AND DEFINED WEST_PYTHON)
set(Python3_EXECUTABLE "${WEST_PYTHON}")
endif()

if(NOT Python3_EXECUTABLE)
# We are using foreach here, instead of
# find_program(PYTHON_EXECUTABLE_SYSTEM_DEFAULT "python" "python3")
# cause just using find_program directly could result in a python2.7 as python,
# and not finding a valid python3.
foreach(candidate "python" "python3")
find_program(Python3_EXECUTABLE ${candidate})
if(Python3_EXECUTABLE)
execute_process (COMMAND "${Python3_EXECUTABLE}" -c
"import sys; sys.stdout.write('.'.join([str(x) for x in sys.version_info[:2]]))"
RESULT_VARIABLE result
OUTPUT_VARIABLE version
OUTPUT_STRIP_TRAILING_WHITESPACE)

if(version VERSION_LESS PYTHON_MINIMUM_REQUIRED)
set(Python3_EXECUTABLE "Python3_EXECUTABLE-NOTFOUND" CACHE INTERNAL "Path to a program")
endif()
endif()
endforeach()
endif()

find_package(Python3 ${PYTHON_MINIMUM_REQUIRED} REQUIRED)

# Zephyr internally used Python variable.
set(PYTHON_EXECUTABLE ${Python3_EXECUTABLE})

else()
# Set the strategy of the FindPython module to LOCATION to find the first found
# python that matches the requirements rather than the most recent
set(Python_FIND_STRATEGY "LOCATION")

find_package(Python 3.8...3.12 COMPONENTS Interpreter)

# Set the Zephyr internally used Python variable.
set(PYTHON_EXECUTABLE ${Python_EXECUTABLE})
endif()

set(PYTHON_MINIMUM_REQUIRED 3.8)

find_package(Deprecated COMPONENTS PYTHON_PREFER)

if(NOT DEFINED Python3_EXECUTABLE AND DEFINED WEST_PYTHON)
set(Python3_EXECUTABLE "${WEST_PYTHON}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

But you are changing this functionality which is described here: https://github.com/zephyrproject-rtos/zephyr/blob/main/cmake/modules/west.cmake#L11

Copy link
Collaborator Author

@teburd teburd Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is exactly the problem, as WEST_PYTHON doesn't seem to use the path to the symlinked python interpreter in my virtualenv?

Like if I were to keep the existing find python code, it finds not the virtualenv python but the actual installed python on my system, and this isn't correct!

Shouldn't this simply be finding the python that is my path not trying to do some odd "yeah but use the one west was called with" nonsense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then you've installed west at a system level, you can't have it both ways, either install west and all zephyr dependencies at a system level, or install them at a virtual env level, not one at system and rest in virtual env.

Copy link
Collaborator Author

@teburd teburd Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Both west and python are installed in the virtualenv, and no it doesn't choose the virtualenv python it uses the system python.

It does this because the static link is looked past in west.cmake I believe? Like with nix there's two levels of python environments basically... the actual interpreter, the interpret + python packages, then my virtualenv.

Without this patch I get...

(env) tburdick@zefire ~/z/z/doc (main)> make html-fast
make html DT_TURBO_MODE=1
make[1]: Entering directory '/home/tburdick/z/zephyr/doc'
cmake \
	-GNinja \
	-B_build \
	-S. \
	-DDOC_TAG=development \
	-DSPHINXOPTS="-j auto -W --keep-going -T" \
	-DSPHINXOPTS_EXTRA="" \
	-DLATEXMKOPTS="-halt-on-error -no-shell-escape" \
	-DDT_TURBO_MODE=1
Loading Zephyr module(s) (Zephyr base (cached)): doc
-- Found Python3: /nix/store/d9h22m8c7irvn866jmc6ilkk8j460bxg-python3-3.10.13-env/bin/python (found suitable version "3.10.13", minimum required is "3.8") found components: Interpreter 
-- Cache files will be written to: /home/tburdick/.cache/zephyr
-- Zephyr base: /home/tburdick/z/zephyr
-- Could NOT find LATEX (missing: LATEX_COMPILER PDFLATEX) 
CMake Warning at CMakeLists.txt:39 (message):
  LaTeX components not found.  PDF build will not be available.


-- Configuring done
-- Generating done
-- Build files have been written to: /home/tburdick/z/zephyr/doc/_build
cmake --build _build --target html
[0/2] Generating Devicetree bindings documentation...
Traceback (most recent call last):
  File "/home/tburdick/z/zephyr/doc/_scripts/gen_devicetree_rest.py", line 21, in <module>
    from devicetree import edtlib
  File "/home/tburdick/z/zephyr/scripts/dts/python-devicetree/src/devicetree/edtlib.py", line 79, in <module>
    import yaml
ModuleNotFoundError: No module named 'yaml'
FAILED: CMakeFiles/devicetree /home/tburdick/z/zephyr/doc/_build/CMakeFiles/devicetree 
cd /home/tburdick/z/zephyr/doc/_build && /nix/store/h8q4r7f1j906rlffj96dlbbmwc9zbirk-cmake-3.25.3/bin/cmake -E env PYTHONPATH=/home/tburdick/z/zephyr/scripts/dts/python-devicetree/src: ZEPHYR_BASE=/home/tburdick/z/zephyr /nix/store/d9h22m8c7irvn866jmc6ilkk8j460bxg-python3-3.10.13-env/bin/python /home/tburdick/z/zephyr/doc/_scripts/gen_devicetree_rest.py --vendor-prefixes /home/tburdick/z/zephyr/dts/bindings/vendor-prefixes.txt --dts-root /home/tburdick/z/zephyr --turbo-mode /home/tburdick/z/zephyr/doc/_build/src/build/dts/api
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:21: html] Error 1
make[1]: Leaving directory '/home/tburdick/z/zephyr/doc'
make: *** [Makefile:18: html-fast] Error 2
(env) tburdick@zefire ~/z/z/doc (main) [2]> which python
/home/tburdick/z/env/bin/python
(env) tburdick@zefire ~/z/z/doc (main)> which west
/home/tburdick/z/env/bin/west

With this patch cmake correctly chooses my virtualenv python as the executable.

Hard to understand why any of this cmake to root out a different python interpreter is even needed, if I have python in my path it should be choosing the first python in my path not go spelunking into the static links looking for something.

Copy link
Collaborator

@marc-hb marc-hb Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Without this patch I get...

@teburd could you move this data to a new bug?

EDIT:

Hard to understand why any of this cmake to root out a different python interpreter is even needed,

Isn't the answer in your commit message? "Now that the minimum version required is 3.20,..."

Copy link
Collaborator Author

@teburd teburd Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd be happy with an explanation as to why west.cmake tries to find the real python elf. I couldn't find one in the git history or file comments.

Python is also broken on archlinux for me and has been for a long time, which is why I ended up using nix in the first place. Because I was tired of seeing some python dep suddenly breaking when python was updated on there.

Current situation on my arch machine without this patch..

(py3) ~/z/z/doc ❯❯❯ make html-fast
make html DT_TURBO_MODE=1
make[1]: Entering directory '/home/tburdick/zephyrtrees/zephyr/doc'
cmake \
	-GNinja \
	-B_build \
	-S. \
	-DDOC_TAG=development \
	-DSPHINXOPTS="-j auto -W --keep-going -T" \
	-DSPHINXOPTS_EXTRA="" \
	-DLATEXMKOPTS="-halt-on-error -no-shell-escape" \
	-DDT_TURBO_MODE=1
Loading Zephyr module(s) (Zephyr base (cached)): doc
-- Cache files will be written to: /home/tburdick/.cache/zephyr
-- Found west (found suitable version "1.2.0", minimum required is "1.0.0")
-- Zephyr base: /home/tburdick/zephyrtrees/zephyr
CMake Warning at CMakeLists.txt:39 (message):
  LaTeX components not found.  PDF build will not be available.


-- Configuring done (0.7s)
-- Generating done (0.0s)
-- Build files have been written to: /home/tburdick/zephyrtrees/zephyr/doc/_build
cmake --build _build --target html
[0/2] Generating Devicetree bindings documentation...
[1/2] Running Sphinx HTML build...
Running Sphinx v7.2.6

Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/sphinx/registry.py", line 447, in load_extension
    mod = import_module(extname)
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1204, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1140, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'breathe'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/lib/python3.11/site-packages/sphinx/cmd/build.py", line 293, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/sphinx/application.py", line 233, in __init__
    self.setup_extension(extension)
  File "/usr/lib/python3.11/site-packages/sphinx/application.py", line 406, in setup_extension
    self.registry.load_extension(self, extname)
  File "/usr/lib/python3.11/site-packages/sphinx/registry.py", line 450, in load_extension
    raise ExtensionError(__('Could not import extension %s') % extname,
sphinx.errors.ExtensionError: Could not import extension breathe (exception: No module named 'breathe')

Extension error:
Could not import extension breathe (exception: No module named 'breathe')
FAILED: CMakeFiles/html /home/tburdick/zephyrtrees/zephyr/doc/_build/CMakeFiles/html 
cd /home/tburdick/zephyrtrees/zephyr/doc/_build && /usr/bin/cmake -E env DOXYGEN_EXECUTABLE=/usr/bin/doxygen DOT_EXECUTABLE=/usr/bin/dot /usr/bin/sphinx-build -b html -c /home/tburdick/zephyrtrees/zephyr/doc -d /home/tburdick/zephyrtrees/zephyr/doc/_build/doctrees -w /home/tburdick/zephyrtrees/zephyr/doc/_build/html.log -t development -j auto -W --keep-going -T /home/tburdick/zephyrtrees/zephyr/doc/_build/src /home/tburdick/zephyrtrees/zephyr/doc/_build/html
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:21: html] Error 1
make[1]: Leaving directory '/home/tburdick/zephyrtrees/zephyr/doc'
make: *** [Makefile:18: html-fast] Error 2
(py3) ~/z/z/doc ❯❯❯ python                                                                                                                                                               ✘ 2 
Python 3.11.6 (main, Nov 14 2023, 09:36:21) [GCC 13.2.1 20230801] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import breathe
>>> 
KeyboardInterrupt
>>> 
(py3) ~/z/z/doc ❯❯❯ which python
/home/tburdick/py3/bin/python
(py3) ~/z/z/doc ❯❯❯ which pip
/home/tburdick/py3/bin/pip
(py3) ~/z/z/doc ❯❯❯ uname -a
Linux saturn 6.4.11-arch2-1 #1 SMP PREEMPT_DYNAMIC Sat, 19 Aug 2023 15:38:34 +0000 x86_64 GNU/Linux

With this patch...

(py3) ~/z/z/doc ❯❯❯ make html-fast
make html DT_TURBO_MODE=1
make[1]: Entering directory '/home/tburdick/zephyrtrees/zephyr/doc'
cmake \
	-GNinja \
	-B_build \
	-S. \
	-DDOC_TAG=development \
	-DSPHINXOPTS="-j auto -W --keep-going -T" \
	-DSPHINXOPTS_EXTRA="" \
	-DLATEXMKOPTS="-halt-on-error -no-shell-escape" \
	-DDT_TURBO_MODE=1
Loading Zephyr module(s) (Zephyr base (cached)): doc
-- Found Python: /home/tburdick/py3/bin/python3.11 (found suitable version "3.11.6", required range is "3.8...3.12") found components: Interpreter 
-- Cache files will be written to: /home/tburdick/.cache/zephyr
-- Found west (found suitable version "1.2.0", minimum required is "1.0.0")
-- Zephyr base: /home/tburdick/zephyrtrees/zephyr
CMake Warning at CMakeLists.txt:39 (message):
  LaTeX components not found.  PDF build will not be available.


-- Configuring done (0.7s)
-- Generating done (0.0s)
-- Build files have been written to: /home/tburdick/zephyrtrees/zephyr/doc/_build
cmake --build _build --target html
[0/2] Generating Devicetree bindings documentation...
[1/2] Running Sphinx HTML build...
Running Sphinx v6.2.1
making output directory... done
Building Kconfig database...... done
Preparing Doxyfile...
Checking if Doxygen needs to be run...
Running Doxygen...
Syncing Doxygen output...
building [mo]: targets for 0 po files that are out of date
writing output... 
building [html]: targets for 1 source files that are out of date
updating environment: [new config] 1603 added, 0 changed, 0 removed
reading sources... [ 34%] boards/shields/mikroe_adc_click/doc/index .. boards/xtensa/m5stickc_plus/doc/index

So no its not just nix actually

Copy link
Collaborator

Choose a reason for hiding this comment

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

Python is also broken on archlinux for me and has been for a long time, which is why I ended up using nix in the first place. Because I was tired of seeing some python dep suddenly break when python was updated on there.

This would only be the case if you did something like ignore this warning:

｢1 ~｣$ pip install blah
error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try 'pacman -S
    python-xyz', where xyz is the package you are trying to
    install.
    
    If you wish to install a non-Arch-packaged Python package,
    create a virtual environment using 'python -m venv path/to/venv'.
    Then use path/to/venv/bin/python and path/to/venv/bin/pip.
    
    If you wish to install a non-Arch packaged Python application,
    it may be easiest to use 'pipx install xyz', which will manage a
    virtual environment for you. Make sure you have python-pipx
    installed via pacman.

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.

If you ignore it, you do so at your own risk, which may or may not work for you (personally for me, it's been working fine on the same VM for years)

Copy link
Collaborator Author

@teburd teburd Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I never saw a warning like the above, but I guess there's no argument convincing enough here for you.

As I said, I'd be happy with an explanation as to why west.cmake code in particular does what it does. The git history and file have no clues for me.

Copy link
Collaborator

@tejlmand tejlmand Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd be happy with an explanation as to why west.cmake tries to find the real python elf. I couldn't find one in the git history or file comments.

Did you look here: 9284e60
or here: #34368

Generally speaking, the CMake find python handling has always been broken when having more than just a single python interpreter installed.

If not using venv and having multiple Pythons installed, but only the west package / other Zephyr required pip-packages in one of them can give strange results if using the plain CMake FindPython3 mechanism.
That said, CMake's FindPython3 has improved significantly over the years, and is quite good nowadays.

Also we have a requirement / expectation that if west build is used for building Zephyr, then we want to use that exact same Python interpreter that was used for invoking west.

A bit of some history to check up on, to see what we have tried to handle:
Please take a look at the history, some PRs and issues (note, the list is not exhaustive):

PRs

Issues:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Noticing this:

(env) tburdick@zefire ~/z/z/doc (main)> make html-fast

is this only a problem when building the docs or also in general ?

Cause in this particular case then west is not used to invoke the build and hence the WEST_PYTHON is not set.
Did you try to set Python3_EXECUTABLE pointing directly to the interpreter you want ?
(not because that's what you should generally do, but in order to help further investigations)

endif()

if(NOT Python3_EXECUTABLE)
# We are using foreach here, instead of
# find_program(PYTHON_EXECUTABLE_SYSTEM_DEFAULT "python" "python3")
# cause just using find_program directly could result in a python2.7 as python,
# and not finding a valid python3.
foreach(candidate "python" "python3")
find_program(Python3_EXECUTABLE ${candidate})
if(Python3_EXECUTABLE)
execute_process (COMMAND "${Python3_EXECUTABLE}" -c
"import sys; sys.stdout.write('.'.join([str(x) for x in sys.version_info[:2]]))"
RESULT_VARIABLE result
OUTPUT_VARIABLE version
OUTPUT_STRIP_TRAILING_WHITESPACE)

if(version VERSION_LESS PYTHON_MINIMUM_REQUIRED)
set(Python3_EXECUTABLE "Python3_EXECUTABLE-NOTFOUND" CACHE INTERNAL "Path to a program")
endif()
endif()
endforeach()
endif()

find_package(Python3 ${PYTHON_MINIMUM_REQUIRED} REQUIRED)

# Zephyr internally used Python variable.
set(PYTHON_EXECUTABLE ${Python3_EXECUTABLE})
Loading