fixes python virtualenv detection

[benjizhai]

fixes #28723

Signed-off-by: Ben Zhai [email protected]

[tejlmand]

Hi @benjizhai ,

It seems you found a fix for the issue you raised: #28723
so thanks for contributing, but before approving, a couple of things must be done.

• Squashing the commits into 1
• Addressing the code comment

So far neither @mbolivar-nordic or myself seems to have been able to reproduce this issue, and as stated further down, i'm not sure the fix is optimal.

If you look here:

zephyr/scripts/west_commands/build.py

Line 408 in d40fad4

final_cmake_args = ['-DWEST_PYTHON={}'.format(sys.executable),

then the python used by west is actually passed to CMake, so a proper fix is to use that information for selecting python, instead of only testing it here:

zephyr/cmake/west.cmake

Lines 13 to 14 in d40fad4

	get_filename_component(west_realpath ${WEST_PYTHON} REALPATH)
	get_filename_component(python_realpath ${PYTHON_EXECUTABLE} REALPATH)

zephyr/cmake/west.cmake

Line 42 in d40fad4

if(NOT (${west_realpath} STREQUAL ${python_realpath}))

but what is still curious to me, is how west / CMake manage to escape your python virtualenv, cause that should not be possible if it is setup correctly.

Could you check the value in:

<build-folder>/CMakeCache.txt

and check the WEST_PYTHON value ?

[tejlmand]

I don't believe this is a proper fix.
If what you experiences regarding Python virtualenv is a real issue, then this might also happen on windows.
This fix will keep existing (and potentially broken) behavior for windows users, which is not acceptable.

We either need to ensure this issue doesn't exists in Windows, or have a patch that is not relying on the OS.

[benjizhai]

WEST_PYTHON:UNINITIALIZED=/usr/bin/python3

I'm not sure how that happens since I'm not familiar with west. From the original comments I understand that this portion of the code is meant to fix some path issues in Windows only, and would not have any impact on Linux. However in my experience there is an impact, so I modified the code to be used in Windows only accordingly.

[tejlmand]

From the original comments I understand that this portion of the code is meant to fix some path issues in Windows only, and would not have any impact on Linux.

Correct, but it is not related to the handling of virtual env.
Therefore, if there is an issue regarding virtualenv, then we must understand why, and find a proper fix.
Including one that works for windows users.

WEST_PYTHON:UNINITIALIZED=/usr/bin/python3

This is interesting, cause this means west is not using your virtualenv.
You should see something like:

(zephyrproject) $ cat build/CMakeCache.txt |grep -i PYTHON
PYTHON_PREFER_EXECUTABLE:FILEPATH=/home/<userid>/zephyrproject/venv/bin/python
Python3_EXECUTABLE:FILEPATH=/home/<userid>/zephyrproject/venv/bin/python3
WEST_PYTHON:UNINITIALIZED=/home/<userid>/zephyrproject/venv/bin/python3

if west is not executed in the virtualenv, then don't expect CMake will either.

Note: python is a link to python3 in the above output, so everything is good.

(zephyrproject) $ ll bin/python 
bin/python -> python3*

Is west installed in your virtualenv ?
After activating your virtual environment the first time, did you remember to execute:

(zephyrproject) $ python3 -m pip install -r scripts/requirements.txt

to ensure everything is setup correctly ?

[benjizhai]

west is installed in my venv and the venv is activated when the bug happens. The cmake FindPython3 does work for me, however you're using a non-standard solution here.

It could potentially be due to the fact that I ran west first outside of venv and the cmake cache messed things up when I then try to run west inside venv.

Closing this PR, but feel free to reopen it if other people run into the same issue.

[tejlmand]

It could potentially be due to the fact that I ran west first outside of venv and the cmake cache messed things up when I then try to run west inside venv.

Yes, this actually explain things.
Two things will happen (or more correct not happen).
When running west later, then it will not re-run CMake configure, as seen:

zephyr/scripts/west_commands/build.py

Lines 168 to 172 in 97696f3

	else:
	self._update_cache()
	if (self.args.cmake or self.args.cmake_opts or
	self.args.cmake_only):
	self.run_cmake = True

but if you force a cmake rerun through west, using west build -c after switching to the venv, then you'll see that the cache var WEST_PYTHON actually gets updated.

The cmake FindPython3 does work for me,

Not sure what you mean by working in this context ?
If I create the following CMakeLists.txt

cmake_minimum_required(VERSION 3.13.1)

find_package(Python3)

and then run CMake outside venv, i'll get:

$ cmake -Bbuild .
...
-- Found Python3: /usr/bin/python3.8 (found version "3.8.0") found components:  Interpreter 
...
$ cat build/CMakeCache.txt |grep -i PYTHON
Python3_EXECUTABLE:FILEPATH=/usr/bin/python3.8

and then switch to my venv and re-run CMake:

$ . ~/zephyrproject/venv/bin/activate.fish 
(zephyrproject) $ cmake -Bbuild 
-- Configuring done
-- Generating done
...
(zephyrproject) $ cat build/CMakeCache.txt |grep -i PYTHON
Python3_EXECUTABLE:FILEPATH=/usr/bin/python3.8

my Python in the CMake cache stays the same.
This is actually what I would expect, and is a feature of CMake.

After you have configured the build system using CMake, then it will keep those settings for all subsequent build invocations unless the USER specifically request CMake to use other settings, by changing the values in the CMakeCache.

Once one of the calls succeeds the result variable will be set and stored in the cache so that no call will search again.
https://cmake.org/cmake/help/git-stage/command/find_package.html

Clearing the cache, and re-run CMake, will then naturally pick up the venv python, as expected:

(zephyrproject) $ rm build/CMakeCache.txt 
(zephyrproject) $ cmake -Bbuild .
...
(zephyrproject) $ cat build/CMakeCache.txt |grep -i PYTHON
Python3_EXECUTABLE:FILEPATH=/home/<userid>/zephyrproject/venv/bin/python3

however you're using a non-standard solution here.

which is not the issue here, as the use of find_package(Python3) behave the same in Zephyr in terms of Python CMake cache variables, as I just demonstrated.

The reason for the adjustments to the vanilla FindPython3 is due to it's inability to distinguish between x86 and 64 bit versions, when both are installed. Not prefering the version in PATH over other versions, and some more, see #24252, #24340, #24692
However, FindPython3 does have some improvements that fixes other issues, like #11103 and #23922.