venv: Add support for virtual environment specs #736
Conversation
yperess
force-pushed
the
main
branch
2 times, most recently
from
5b0793c
to
b6d9ffb
Compare
|
@nashif what's the process for getting this PR reviewed? |
There was a problem hiding this comment.
This is IMHO missing some high-level descriptions of the problem statement and of the solution's design. I'm especially wondering what a "binary requirement" is?
I think the best way to fix that would be a corresponding documentation PR, for instance see how this unrelated feature had a separate documentation PR:
zephyrproject-rtos/zephyr#76027
west documentation is unfortunately located in the Zephyr repo for various reasons: fewer sites to maintain, single place for west built-ins and Zephyr extensions,...
The obvious drawback is: PRs like these must be split across two repos.
PS: there is a shortage of west maintainers right now.
src/west/manifest.py
Outdated
| elif current_os == "Linux": | ||
| defs["os"] = "linux" | ||
| else: | ||
| raise RuntimeError("Unknown OS: " + current_os) |
There was a problem hiding this comment.
Does this code run when you don't use the new feature?
There was a problem hiding this comment.
It did, thanks for catching it. I've moved it to a lazily initialized class so it only gets run if you're using the feature.
src/west/manifest.py
Outdated
| if current_os == "Windows": | ||
| defs["os"] = "windows" | ||
| elif current_os == "Darwin": | ||
| defs["os"] = "mac" |
There was a problem hiding this comment.
Is this indirection really necessary?
There was a problem hiding this comment.
I added more details above in the architecture remapping. Chromium provides a free package database which is very very robust. These OS names are what's used in CIPD so having these as the "standard" gives us access to the whole database without any complexity to the manifest.
| @@ -67,6 +68,64 @@ | |||
| # Internal helpers | |||
| # | |||
|
|
|||
| # Mapping of the architectures from platofrm.machine() to a common standard | |||
There was a problem hiding this comment.
Do you mean, to some "Zephyr" standard? I agree "Darwin" is a little bit cryptic but aaarch64 is not.
There was a problem hiding this comment.
ACK, I updated the comment to specify where the "standard" came from. I don't think it matters a ton, but I do think that it's a good standard to follow since it makes the URL constructions compatible with a very large database.
| @@ -66,6 +66,78 @@ mapping: | |||
| required: true | |||
| type: str | |||
|
|
|||
| # The venv key specifies requirements for the build environment for Zephyr | |||
There was a problem hiding this comment.
The code in this repo is supposed to be not specific to Zephyr. Is this PR Zephyr-specific? I don't have time for a full review sorry.
If it is Zephyr-specific, then it belongs to https://github.com/zephyrproject-rtos/zephyr/tree/main/scripts/west_commands
Considering west build is Zephyr-specific, I suspect this PR is too.
Even if it's not Zephyr-specific, https://github.com/zephyrproject-rtos/zephyr/tree/main/scripts/west_commands could be a good "staging" area to gather feedback, real-use and... more reviewers.
There was a problem hiding this comment.
This is not Zephyr specific but will be used in a Zephyr west command. The goal of this change is to add metadata about a project's requirements (both python and binary).
Thank you for the review. I've added a WIP CL that uses this and adds documentation at zephyrproject-rtos/zephyr#78610. It's a WIP because I haven't had the chance to track down all the binary dependencies though I got a few of them (enough to see how this PR will be used). |
yperess
force-pushed
the
main
branch
2 times, most recently
from
8faab3a
to
91d41dd
Compare
Adds handling for a venv key in the manifest. This key will then be used to provide virtual environment metadata to a west bootstrap command. The metadata includes: - A virtual environment name - A list of python requirement files, individual packages, constraints, and local directories. - A mapping of binary requirements and which URLs can be used to find them. These include architecture and OS specific mappings. Signed-off-by: Yuval Peress <[email protected]>
There was a problem hiding this comment.
this is a big no-go from my side.
The west manifest should not start to contain a bunch of keys and values not relevant or used by core west itself.
Nor should it contain a bunch of information which may not be of interest to several users of the manifest.
See also this comment for more details: zephyrproject-rtos/zephyr#78610 (review)
|
Thanks for submitting zephyrproject-rtos/zephyr#78610, I also replied there. Once the dust settles there we can revisit what belongs to Zephyr versus what belongs to west itself. |
Adds handling for a venv key in the manifest. This key will then be used to provide virtual environment metadata to a west bootstrap command. The metadata includes: