west_commands: Add west command for bootstrapping #78610
Conversation
**WIP** Do not merge Signed-off-by: Yuval Peress <peress@google.com>
| foo: | ||
| urls: | ||
| "linux-amd64": | ||
| url: "https://path/to/linux.file.tar.gz" | ||
| paths: ["bin"] | ||
| "windows-amd64": | ||
| url: "https://path/to/windows.file.zip" | ||
| paths: ["bin"] |
There was a problem hiding this comment.
Please form a complete sentence.
| name: ".venv" | ||
| definitions: | ||
| cipd_url: "https://chrome-infra-packages.appspot.com/dl" | ||
| bin-requirements: |
There was a problem hiding this comment.
I take it this is meant as a sample not something that should ever be added, but I really do not like this idea. This should be brought up at architecture working group
|
I think a |
It's a hard call, honestly inside Google all the teams are using Bazel and I'm working on a Bazel port for Zephyr. It's nice because it handles all the dependencies automatically. Writing a
With this you'd get any new dependencies when you run step 3, then step 4 would update your virtual environment automatically. |
There was a problem hiding this comment.
Thanks for bringing up this topic.
Having some sort of easy to get started feature can surely be beneficial to many, but we need to be careful how such feature is integrated.
But let me provide some feedback regarding the most problematic parts of the proposal / rfc.
(Perhaps marking this as an RFC in the title would be helpful)
Having fields in the manifest which is not used by west itself is a no-go from me.
From what I can tell, then manifest schema must allow for keys like venv and more, but west itself are not going to use those keys and values. Those keys and values are only going to be used be the bootstrap extension command.
I believe the manifest should only contain information which is core to every west user.
The projects are core, everyone is going to use the information, but the venv is not core.
You may not need any of it, or will only need the part related to your OS.
This means the manifest gets bloated for many users for no reason.
On the Zephyr module parts then I really dislike modules to be able to define new tools which gets installed.
Especially if users are recommended to run bootstrap after every update.
If Zephyr wants to have a bootstrap command, then such command should only install packages which are defined inside of the Zephyr repo itself, perhaps as a wrapper around pip install -R scripts/requirements.txt.
If downstream projects wants to install more tools, then they can provide their custom my-bootstrap command, which then includes both the Zephyr command as well as any additions.
This will also help to ensure that new tools added as requirement inside Zephyr gets properly reviewed.
Perhaps take a look at #76283 and start on a similar small scale.
Note, i've used the word bootstrap above, as that is the name already used, but I consider bootstrapping very different.
Especially in this case I think it's a bad command name because you expect users to run it after each west update, and thus it's not really a bootstrap anymore.
Regarding the bootstrap command implementation then I noticed that you seem to assume every non-windows user is using bash.
Please note that other shell types has dedicated activate scripts and that the pure activate script is for bash.
Ref: # This file must be used with "source bin/activate" *from bash*
|
Marking as DNM for now since this requires further discussion in wider forums. |
There was a problem hiding this comment.
Interesting idea that seems to solve a real problem but I think I agree with all or most of @tejlmand 's comments.
I really see no reason for this to live in the main west.yml manifest file. You could use west config (examples: https://docs.zephyrproject.org/latest/develop/west/build-flash-debug.html#permanent-cmake-arguments, https://docs.zephyrproject.org/latest/develop/west/sign.html#rimage ) or a new, separate configuration file using either https://docs.zephyrproject.org/latest/develop/west/west-apis.html#module-west or your own, feature-specific .ini parser - very cheap in Python.
I agree the name "bootstrap" is a misleading: you'd really expect to run a "bootstrap" command only once. Maybe something like reqs-update for consistency with west update?
pip has some package security features built-in, what is the equivalent for these new "binary requirements"? https:// and that's it?
apt-get really does not belong here. Yes west is a swiss army-knife wrapper of many things but apt requirements and versions are not portable at all, not even across the distributions that are all using it! Linux fragmentation is a problem but it's too big of a problem for west to solve. Also, it's weird to be using in the same extension virtual environments because some users complain "the setup is affecting their global environment" on one hand, and then invoke sudo apt-get on the other hand. AFAIK nothing in west currently requires sudo/root/admin access and it should stay that way. Finally, users should run apt-get install once the first time (= actual "bootstrap") and extremely rarely if ever again. https://docs.zephyrproject.org/latest/develop/getting_started/index.html That does not require a convenience wrapper. Then their operating system should run apt-get upgrade regularly in a way totally independent from west and zephyr. If that ever clashes with Zephyr then => virtual environments.
Whether apt-get belongs to zephyr/west_commands or not, it is different enough to deserve a separate PR
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs
| foo: | ||
| urls: | ||
| "linux-amd64": | ||
| url: "https://path/to/linux.file.tar.gz" | ||
| paths: ["bin"] | ||
| "windows-amd64": | ||
| url: "https://path/to/windows.file.zip" | ||
| paths: ["bin"] |
There was a problem hiding this comment.
Please form a complete sentence.
| - ``${os}`` to the current operating system (``windows``, ``mac``, or | ||
| ``linux``). | ||
| - ``${arch}`` to the current architecture. West uses the | ||
| [CIPD](https://chrome-infra-packages.appspot.com/) standard naming which |
There was a problem hiding this comment.
I see no documentation at that specific link and no matter how great CPID is, its naming convention does not seem "standard".
On the other hand, aarch64 and x86_64 really seem "standard":
- System V ABI for the Arm® 64-bit Architecture (AArch64)
- https://gcc.gnu.org/onlinedocs/gcc/Submodel-Options.html
- https://llvm.org/doxygen/Triple_8h_source.html
- https://wiki.osdev.org/AArch64, https://wiki.osdev.org/X86-64
- https://github.com/zephyrproject-rtos/sdk-ng/releases/tag/v0.16.8
- etc.
Python did follow existing standards, CPID did not.
Interestingly, https://wiki.debian.org/Multiarch/Tuples uses "arm64" and "amd64" as (strange?) shortcuts for "aarch64-linux-gnu" resp. "x86_64-linux-gnu". Is CPID similar maybe?
| RED = "\033[91m" | ||
| BOLD = "\033[1m" | ||
| UNDERLINE = "\033[4m" | ||
| END = "\033[0m" |
There was a problem hiding this comment.
This should probably be in some common library for UI consistency.
| ) | ||
|
|
||
|
|
||
| def _rm_dir(path_to_delete: pathlib.Path) -> None: |
There was a problem hiding this comment.
This should also be in some west_commands/common.py file if needed.
Please share links here, thanks! |
|
Just remembered this sorry: did you look at "prior art" like for instance "PlatformIO"? #53303 It apparently did not keep up with Zephyr but I remember some people raving about it at the time so it must have got a few things right and should deserve a look. Some ideas may still apply. https://github.com/zephyrproject-rtos/docker-image is another thing to look at. Also: |
While looking at something else I just noticed the Getting Started Guide in the official documentation now defaults to using So how did this "lot of people" have their global environment affected? Did they skip the virtualenv and why?
Please first explain what's wrong with the existing workflow(s). Some incompatibility with Bazel maybe? |
why do we a need a DNM on a Draft? Drafts are by definition DNM.... |
agree fully, but the manifest is already bloated with many things that not everyone needs. Nothing against babblesim, but why the core manifest has 12 entries? Then all the hals everyone forced to download and keep updating, even if you are just interested in one etc. etc. BAsically what I am saying, this can't be the reason for not to do this, there are other good reasons other than bloat of a file. Having said that, again, I agree all of this bootstrap related content really does not belong here in the manifest, when this feature was mentioned and it was also discussed some time ago, I was thinking along the lines of having each module maintain the list of it dependencies in the module.yml file, i.e. instead of having in the main-tree for a module that is optional, put that requirement in the module.yml file and have a west command collect those requirements from all modules and display the list so the developer/user can install them, either in a venv or directly. This is obviously a bit further than that and is trying to automate whatever we have described in the getting started guide.
it should be ok to have those listed instead of having them required in zephyr, i.e. if I do not use a module, I do not have to deal with installing its requirements. Installing those can be an extra step, but even if we decide that we want to install with bootstrap, west will become a package manager that needs to deal with different user setups and preferences etc. Do we really want to spend time on this, probably not, probably it is enough to just list what packages are needed for a module to be useful instead of forcing module related package through a global requirements file.
Why? What is the difference? Why users should be forced to install unrelated packages of a module if they are not using the module? Thats what happens now.
yeah, it will get messy trying to 'bootstrap' the different user envirnments on different OSes etc. that is not going to be fun |
I think there is a distinction between "extra git repos that many people don't use" and data for brand new package management features that are not even git-related at all. The "bloat potential" of the latter seems orders of magnitude bigger.
BTW: |
Agree that not everyone needs all projects, but they do need projects. This is different from the tools described in this PR, cause those are not core, you can do without, especially if tools are tied to an OS. Projects are not tied to an OS.
Yes, I kind of consider babblesim to also bloat the manifest, but that's not an argument to go further down such a path.
but why is that in the main tree in the first place ? If Zephyr doesn't require protobuf, then imo that should go to the module which can then have its own
Then that should be cleaned up, see above. |
WIP Do not merge