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: west: invoke west using same python as rest of build system #26060

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

tejlmand
Copy link
Collaborator

@tejlmand tejlmand commented Jun 8, 2020

When running CMake, then Python3 will be used.
This is detected through FindPython3, with a preference for using the
python or python3 in path, if any of those matches the required Python
minimal version in Zephyr.

It is also possible for users to specify a different Python, as example
by using:
cmake -DPYTHON_PREFER=/usr/bin/python3.x

However, when running west as native command, then west will be
invoked on linux based on the python defined in:
west launcher, which could be: #!/usr/bin/python3.y

Thus there could be mismatch in Pythons used for west and the python
used for other scripts.

This is even worse on windows, where a user might experience:

>.\opt\bin\Scripts\west.exe --version
Traceback (most recent call last):
  File "C:\Python37\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  ...
  File "C:\Python37\lib\socket.py", line 49, in <module>
    import _socket
ImportError: Module use of python38.dll conflicts with this version of
Python.

when testing out a newer Python, but the python in path is still a 3.7.

By importing west into zephyr_module.py and by using
python -c "from west.app.main import main; main(['<command>'])"
we ensure the same python is used in all python scripts.

Also it allows the user to control the python to use for west.

It also ensures that the west version being tested,m is also the version
being used, where old code would test the version imported by python,
but using the west in path (which could be a different version)

If the west version installed in the current Python, and west invocation
is using a different Python interpreter, then an additional help text
is printed, to easier assist users with debugging.

Signed-off-by: Torsten Rasmussen Torsten.Rasmussen@nordicsemi.no

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jun 8, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@tejlmand
Copy link
Collaborator Author

tejlmand commented Jun 8, 2020

Example of improved help text, if wanting to build with python3.7 when python3 points to another version.

west build -bnrf52840dk_nrf52840 -dbuild_west -- -DPYTHON_PREFER=/usr/bin/python3.7
...
CMake Error at /projects/github/ncs/zephyr/cmake/host-tools.cmake:38 (message):
  The detected west version is unsupported.

    The version was found to be 0.7.0:
      
    But the minimum supported version is 0.7.1
    Please upgrade with:
        /usr/bin/python3.7 -mpip install --upgrade west

    Note:
    The Python version used by west is:  /usr/bin/python3
    The Python version used by CMake is: /usr/bin/python3.7
    This might be correct, but please verify your installation.

@tejlmand tejlmand force-pushed the cmake_west_cleanup branch 2 times, most recently from 25d74b2 to 1ca778f Compare June 8, 2020 20:14
cmake/host-tools.cmake Outdated Show resolved Hide resolved
cmake/host-tools.cmake Outdated Show resolved Hide resolved
scripts/west_commands/build.py Show resolved Hide resolved
@tejlmand tejlmand force-pushed the cmake_west_cleanup branch 3 times, most recently from f9aca49 to 6af116b Compare June 9, 2020 11:08
@tejlmand tejlmand added the DNM This PR should not be merged (Do Not Merge) label Jun 9, 2020
@tejlmand tejlmand removed the DNM This PR should not be merged (Do Not Merge) label Jun 10, 2020
@tejlmand tejlmand force-pushed the cmake_west_cleanup branch 3 times, most recently from 296286f to 03bd929 Compare June 10, 2020 10:54
@tejlmand tejlmand force-pushed the cmake_west_cleanup branch 2 times, most recently from d0e3d5f to 57f7c28 Compare June 17, 2020 20:34
@tejlmand
Copy link
Collaborator Author

@mbolivar due to compliance test, I discovered that I wasn't testing the error message properly, when not in a west workspace.
Code now redirects sys.stderr to be able to correctly check the message, so please re-review.

@tejlmand tejlmand changed the title [wip] cmake: west: invoke west using same python as rest of build system cmake: west: invoke west using same python as rest of build system Jun 22, 2020
@tejlmand tejlmand removed the DNM This PR should not be merged (Do Not Merge) label Jun 22, 2020
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

I was testing this out before adding my +1, but this breaks west build for me.

Example steps to reproduce issues:

  1. west build -b nrf9160dk_nrf9160 -s samples/hello_world/: this runs cmake to generate a build system, but it doesn't actually build anything (west build normally invokes cmake in build tool mode to finish the job). Perhaps the zcmake changes need to be guarded by a kwarg that can be turned off when running cmake in build tool mode.
  2. Verbose builds don't seem to work at all:
$ west -v build -b nrf9160dk_nrf9160 -s samples/hello_world/ 
ZEPHYR_BASE=/home/mbolivar/zp/zephyr (origin: configfile)
cmake version 3.17.3 is OK; minimum version is 3.13.1
Running CMake: /usr/bin/cmake -DWEST_PYTHON=/home/mbolivar/.virtualenvs/west-dev/bin/python --build /home/mbolivar/zp/zephyr/build -- -v
CMake Error: The source directory "/home/mbolivar/zp/zephyr/-v" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
FATAL ERROR: command exited with status 1: /usr/bin/cmake -DWEST_PYTHON=/home/mbolivar/.virtualenvs/west-dev/bin/python --build /home/mbolivar/zp/zephyr/build -- -v

@tejlmand tejlmand added the DNM This PR should not be merged (Do Not Merge) label Jun 23, 2020
@tejlmand
Copy link
Collaborator Author

I was testing this out before adding my +1, but this breaks west build for me.

Fixed, see also: #26060 (comment)

@tejlmand tejlmand removed the DNM This PR should not be merged (Do Not Merge) label Jun 24, 2020
When running CMake, then Python3 will be used.
This is detected through FindPython3, with a preference for using the
python or python3 in path, if any of those matches the required Python
minimal version in Zephyr.

It is also possible for users to specify a different Python, as example
by using:
`cmake -DPYTHON_PREFER=/usr/bin/python3.x`

However, when running `west` as native command, then west will be
invoked on linux based on the python defined in:
`west` launcher, which could be: `#!/usr/bin/python3.y`

Thus there could be mismatch in Pythons used for `west` and the python
used for other scripts.

This is even worse on windows, where a user might experience:
```
>.\opt\bin\Scripts\west.exe --version
Traceback (most recent call last):
  File "C:\Python37\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  ...
  File "C:\Python37\lib\socket.py", line 49, in <module>
    import _socket
ImportError: Module use of python38.dll conflicts with this version of
Python.
```

when testing out a newer Python, but the python in path is still a 3.7.

By importing `west` into zephyr_module.py and by using, as example
`python -c "from west.util import west_topdir; print(topdir())"`
we ensure the same python is used in all python scripts.

Also it allows the user to control the python to use for west.

It also ensures that the west version being tested, is also the version
being used, where old code would test the version imported by python,
but using the west in path (which could be a different version)

If the west version installed in the current Python, and west invocation
is using a different Python interpreter, then an additional help text
is printed, to easier assist users with debugging.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Code looks fine and my basic tests are now passing. Thanks!

@tejlmand
Copy link
Collaborator Author

@nashif could you take a look ?

@@ -45,6 +45,7 @@ def run_cmake(args, cwd=None, capture_output=False, dry_run=False):
_ensure_min_version(cmake, dry_run)

cmd = [cmake] + args

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this blank line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is probably due to moving code into this file, and then out again, as discussed:
#26060 (comment)

unfortunately this must have resulted in a blank line in that process.
You seem to be the first who noticed.

@nashif nashif merged commit ef3c5e5 into zephyrproject-rtos:master Jul 9, 2020
tejlmand added a commit to tejlmand/fw-nrfconnect-nrf that referenced this pull request Sep 3, 2020
- Ensures same python is used for west as the rest of the build system
  See: zephyrproject-rtos/zephyr#26060

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@tejlmand tejlmand deleted the cmake_west_cleanup branch December 9, 2020 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants