manifest: model west-commands as per-entry records

[pdgendt]

Replace the WestCommandsType alias and the parallel _west_commands_manifest_dirs dict with a frozen WestCommandsEntry dataclass carrying (path, base_dir). Project.west_commands_entries is the new canonical attribute; the old west_commands becomes a deprecated read-only property. Schema accepts str / list[str] / list[{file, base-dir}] so resolved manifests roundtrip through as_yaml() without losing the base_dir needed to resolve file: paths inside specs imported from a project subdirectory.

Fixes #959
Supersedes #965

[codecov]

Codecov Report

❌ Patch coverage is 98.61111% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.45%. Comparing base (186168a) to head (00a5f5a).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/west/manifest.py	98.52%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #966      +/-   ##
==========================================
+ Coverage   86.40%   86.45%   +0.04%     
==========================================
  Files          11       11              
  Lines        3532     3566      +34     
==========================================
+ Hits         3052     3083      +31     
- Misses        480      483       +3     

Files with missing lines	Coverage Δ
src/west/commands.py	95.63% <100.00%> (ø)
src/west/manifest.py	95.22% <98.52%> (+0.05%)	⬆️

... and 1 file with indirect coverage changes

[marc-hb]

I think the tests in #962 should got first because they record/"document" the existing behavior, as it was just changed. Then, this PR can adjust the tests if needed which will again document any behavior change (or none if none)

[marc-hb]

I'm afraid "entry" sounds pretty vague.

We have a massive and self-inflicted name collision between:

• The west-commands: field in the west.yml manifest
• The "west-commands.yml" file(s) it points at
• The OTHER west-commands: header line in those files!
• The "west commands" descriptors in those files. "Descriptor" is an exception: theonly specific name around here? I wish we had more of these.
• The "west commands" .py files that actually implement the commands
• The mypy WestCommandsType
• The WestCommand python class
• A new "entry" class added by this PR
• project.west_commands
• others?

EVERYTHING is a "West command"! Unless it's an "extension", which does not help at all.

This mass collision starts in the user interface and documentation at https://docs.zephyrproject.org/latest/develop/west/extensions.html and unsurprisingly gets worse in the code. That relevant code has already given me many headaches and this one PR will obviously increase the count. I would like those collisions to ideally stop or at least start be mitigated.

Naming is hard, but this PR is definitely the right time to formalize some better terminology. I'm not implying a mass search/replace, at least not yet. But at least finalizing some "correct" names to accelerate communication, bug tracking and reviews.

To be fair, some of the above are unique enough not to need a specific name. For instance, there is only one Python file (per command), so a "west command python file" is specific enough. Not concise but good enough. On the other hand, the stuff in YAML files definitely needs more specific names. Even more so considering it's the user interface, even more so considering we have multiple YAML files.

Let me think about some terminology, just a heads-up.

[marc-hb]

I'm confused for now, sorry. I don't understand why this requires a schema change. Maybe I need more time to better understand the problem statement.

I think I understand the value of having some base_dir in the internal implementation but I don't understand yet why this has to be exposed in the (changed) user interface. Why can't base_dir simply be added as a prefix in the output of west manifest --resolve?

Adding that base_dir does not seem to add any value to the user interface, it looks like a pure hack to make the output of west manifest --resolve work again.

Also, do we really need to add yet another level of lists? We already have two. That bit really looks like a new feature, not a bug fix.

---

Let me think about some terminology, just a heads-up.

• west-commands.yml is a DESCRIPTOR LIST (e.g: cmd_descs_list or longer). LIST sounds vague but I think we have no other, top-level list in this context and we can always extend it to "(command) descriptor list" in any context where it could be ambiguous. Hopefully, making a difference between the list, its file and the pointer to it from the manifest is obvious enough from the context when needed and they don't need different names.

• Each DESCRIPTOR has a single pointer to one command PYTHON FILE.

• Each PYTHON FILE has a list of command CLASSES. No collision there.

[marc-hb]

In general, prefer the newer case and pattern matching. But don't go and change this before we close the high-level discussions.

[pdgendt]

Sure

[marc-hb]

The term "project" is also problematic :-(

For west, a project (not my choice...) is a git repo:
https://docs.zephyrproject.org/latest/develop/west/manifest.html#projects

But in most IDEs and even Zephyr sometimes, a "project" can be the workspace:

• https://code.visualstudio.com/docs/editing/userinterface#_basic-layout
• https://docs.zephyrproject.org/latest/develop/getting_started/index.html#get-zephyr-and-install-python-dependencies

In the following instructions the name "zephyrproject" is used for the workspace,...

EDIT: one possible reason for this confusion is: many development "projects" are single-repo. That is: they don't use west or anything like it.

I think in the case the fix is easy: use either "workspace topdir" or "git top dir".

[pdgendt]

I think for West, it's clear what a project is? Maybe should just name it like that.

Suggested change

	'''A single west-commands spec declared in a project.
	'''A single west-commands spec declared in a West project.

[marc-hb]

Such an override should print at least a DEBUG.

[pdgendt]

Sure, the existing code also did not print though.

[marc-hb]

Why not just fail? This wasn't part of any documented API AFAICT

[pdgendt]

I was under the impression that all fields that do not start with an underscore are considered public? Anyway we don't need to break users of this.

[marc-hb]

That is a typical Python convention but in west I believe the supported API is only the functions that have a pydoc - which I believe is leveraged to produce https://docs.zephyrproject.org/latest/develop/west/west-apis.html

[pdgendt]

west_commands is documented public API, though. In the Zephyr docs (doc/develop/west/west-apis.rst) the Project autoclass documents it explicitly, including a version directive:

.. autoclass:: west.manifest.Project

   .. (note: attributes are part of the class docstring)
   ...
   .. versionchanged:: 0.8.0
      The *west_commands* attribute is now always a list. In previous
      releases, it could be a string or ``None``.

It's rendered on https://docs.zephyrproject.org/latest/develop/west/west-apis.html (attributes come from the Project class docstring, which autoclass picks up).

We should make it deprecated IMO.

[pdgendt]

I'm confused for now, sorry. I don't understand why this requires a schema change. Maybe I need more time to better understand the problem statement.

I think I understand the value of having some base_dir in the internal implementation but I don't understand yet why this has to be exposed in the (changed) user interface. Why can't base_dir simply be added as a prefix in the output of west manifest --resolve?

The problem is that west manifest --resolve outputs a single yaml file, aggregated from the imported ones. The west-commands.yml descriptor files stay in their projects untouched.
We can't simply add the prefix, that conflates two genuinely different directories:

• path = location of the west-commands.yml spec file, relative to project root (e.g. mf_subdir/scripts/winsubA/west-commands.yml)
• base_dir = the root that the file: entries inside that spec resolve against (e.g. mf_subdir)

The Python file resolves as base_dir / desc['file'], i.e. relative to mf_subdir, not relative to the spec file's own directory mf_subdir/scripts/winsubA/. So you can't fold base_dir into the path prefix; they point at different places.

Adding that base_dir does not seem to add any value to the user interface, it looks like a pure hack to make the output of west manifest --resolve work again.

True, base-dir is meaningless for a hand-authored manifest, but it is needed for a resolved manifest to work. I wouldn't call it a hack, but it is an internal detail surfacing in the user-facing schema.

Also, do we really need to add yet another level of lists? We already have two.

There's no level added? Currently we have the following "levels":

1. the manifest west-commands: field -> pointer(s) to spec file(s). Was str | list[str].
2. inside each west-commands.yml, the top-level west-commands: key -> list of descriptors.
3. inside each descriptor, commands: -> list of command classes.

This PR changes level 1 only, and it widens the element type of that existing list from str to str | {file, base-dir}.

That bit really looks like a new feature, not a bug fix.

We're not adding new features? Users can't express anything new. It is genuinely a bug fix IMO.

Let me think about some terminology, just a heads-up.

• west-commands.yml is a DESCRIPTOR LIST (e.g: cmd_descs_list or longer). LIST sounds vague but I think we have no other, top-level list in this context and we can always extend it to "(command) descriptor list" in any context where it could be ambiguous. Hopefully, making a difference between the list, its file and the pointer to it from the manifest is obvious enough from the context when needed and they don't need different names.
• Each DESCRIPTOR has a single pointer to one commands PYTHON FILE.
• Each PYTHON FILE has a list of command CLASSES. No collision there.

I like the idea of using descriptor.

[pdgendt]

I think for West, it's clear what a project is? Maybe should just name it like that.

Suggested change

	'''A single west-commands spec declared in a project.
	'''A single west-commands spec declared in a West project.

[pdgendt]

Sure

[pdgendt]

Sure, the existing code also did not print though.

[pdgendt]

I was under the impression that all fields that do not start with an underscore are considered public? Anyway we don't need to break users of this.

[nmunnich]

EVERYTHING is a "West command"! Unless it's an "extension", which does not help at all.

To come back to this a bit, I very much agree that this is a massive source of confusion and likely a reason why the related bits are hard to reason about. I propose "West Commands Index" be used for the west-commands.yml file in which west-command-python-files are listed along with the west extension commands that they define. We can then call "WestCommandsEntry" "WestCommandsIndexPath" or something, we can refer to those defined within a west manifest as its west commands indices...

I think picking a proper name for that particular file actually resolves a lot of the unclarity simply by providing a good mental reference point from which other naming can branch off of.

[marc-hb]

The problem is that west manifest --resolve outputs a single yaml file, aggregated from the imported ones. The west-commands.yml descriptor files stay in their projects untouched.

Great summary, thanks. I've been thinking about this... "inconsistency" a little bit (not as much as I wanted to yet), and maybe west manifest --resolve should aggregate not just the manifests but the "commands index" files too, producing not just one but two files. The former would point at the latter. Could be something like workspace/.west/all-resolved-commands.yml by default, with the possibility to name it differently with a command line option. That would certainly avoid a backwards-incompatible change to solve a problem that should be solved, but that was discovered more or less by "chance" and that no user seems to have been complaining about so far. I mean how many people use west extensions through the output of west manifest --resolve? Apparently and unsurprisingly very few.

---

To come back to this a bit, I very much agree that this [lack of names] is a massive source of confusion

Thanks for concurring, that means a lot from the person who did 96ad4f3 and friends :-)

[pdgendt]

The problem is that west manifest --resolve outputs a single yaml file, aggregated from the imported ones. The west-commands.yml descriptor files stay in their projects untouched.

Great summary, thanks. I've been thinking about this... "inconsistency" a little bit (not as much as I wanted to yet), and maybe west manifest --resolve should aggregate not just the manifests but the "commands index" files too, producing not just one but two files. The former would point at the latter. Could be something like workspace/.west/all-resolved-commands.yml by default, with the possibility to name it differently with a command line option. That would certainly avoid a backwards-incompatible change to solve a problem that should be solved, but that was discovered more or less by "chance" and that no user seems to have been complaining about so far. I mean how many people use west extensions through the output of west manifest --resolve? Apparently and unsurprisingly very few.

Not particularly thrilled about this. west manifest --freeze/--resolve simply output to stdout, there's no "file" concept to start with.

[marc-hb]

Not particularly thrilled about this. west manifest --freeze/--resolve simply output to stdout, there's no "file" concept to start with.

Not thrilled about this either but I think it's a small enough egg to break.

I think we should look at what are the practical use cases for west manifest --resolve?

In an informative, read-only, "pure SBOM" use case, west extensions don't matter. This is probably the most common use case and why we never heard of this bug before and just discovered it "by chance".

So in this particular context we must focus on the more "active" use case where people actually try to feed the resolved manifest back to west, for instance to reproduce a build. If they use the resolved manifest only to "bootstrap" the initial download and then pivot to the "real" manifest (while of course making sure nothing changes in git), then we don't care about commands in the "resolved" manifest either. Note they can (and should) re-produce the resolved manifest after pivoting and compare as a safety check.

So I think that leaves only the case of people actually trying to use the resolved manifest to both:

1. bootstrap the download
2. not carry the resolved-commands.yml file.
3. not pivot to the real one and actually use the former to west build etc.

So yes: the (small minority of?) people bent on doing all that would have to change 2. and copy one extra file, which is still not the end of the world.

Unless I missed some use cases?

Could be something like workspace/.west/all-resolved-commands.yml by default, with the possibility to name it differently with a command line option.

Maybe we could have a middle-ground with a smaller workspace/.west/all-painful-subdir-commands.yml (better name wanted) containing ONLY the extensions that have "subdir issues"? Even less pretty (granted) but now we've hopefully reduced the number of users who actually need to carry this file to close to zero.

True, base-dir is meaningless for a hand-authored manifest, but it is needed for a resolved manifest to work. I wouldn't call it a hack, but it is an internal detail surfacing in the user-facing schema.

So base-dir not the prettiest thing either... pick your poison?

[marc-hb]

True, base-dir is meaningless for a hand-authored manifest, but it is needed for a resolved manifest to work. I wouldn't call it a hack, but it is an internal detail surfacing in the user-facing schema.

If we must make a backwards-incompatible schema change, then how about offering the possibility of embedding west commands descriptors inside manifest files? Now that would be "real" feature offered to users. One less file!

This could be used by west manifest --resolve for either all commands, or only for the ones having "subdir issues".

---

... where people actually try to feed the resolved manifest back to west, for instance to reproduce a build.

Interestingly, NONE of the solutions mentioned so far achieves "perfect" reproducibility for the simple reason that:

The problem is that west manifest --resolve outputs a single yaml file, aggregated from the imported ones.

That will always be a major difference no matter what. It's the "original sin" and why we are stuck between a rock and a hard place.

west manifest --freeze/--resolve "simply" outputs to stdout

That "simplicity" (quotes mine) is deceiving.

[nmunnich]

That would certainly avoid a backwards-incompatible change to solve a problem that should be solved

I don't see the backwards incompatible change that you're seeing. From what I can see, the new schema should be backwards compatible with existing schemas?

Personally I would be in favor of adding base-dir to the schema as a solution. It feels like the alternative suggested approach would be much more complex, and has a much higher chance of followup issues appearing. I think a schema change is necessary anyway to allow round trips, because we (as far as I can tell) currently do not support lists of west-command-index. Consider:

manifest:
 self:
  import:
   - a/west.yml
   - b/west.yml

where both of those import a west-command-index via self. While slightly more meaningful for a hand-authored manifest than base-dir, I would say in practice they are likely equally meaningless.

[marc-hb]

I don't see the backwards incompatible change that you're seeing. From what I can see, the new schema should be backwards compatible with existing schemas?

My bad, I meant "incompatible schema change" and I added "backwards" by force of habit.

I think a schema change is necessary anyway to allow round trips, because we (as far as I can tell) currently do not support lists of west-command-index. Consider: ...

How would some generated aggregated-commands.yml not support this example?

[marc-hb]

I just remembered that rsync v2.6.7 already solved a similar "base_dir" issue with a small but powerful change in how it treats source arguments when using its --relative option. https://download.samba.org/pub/rsync/rsync.1#opt--relative

Imagine you want to copy subdir/file.yml to destination/ in a single step / single command. That is, you want destination/subdir/file.yml to be created. destination/subdir/ does not exist yet.

Before rsync v2.6.7, none of these did it:

• rsync base_dir/subdir/file.yml destination copies to destination/file.yml
• rsync --relative base_dir/subdir/file.yml destination/ copies to destination/base_dir/subdir/file.yml - wrong again.

You could of course always pushd base_dir/ && rsync subdir/file.yml ../destination but that is tedious and error-prone, and it becomes impractical when you want to copy from a number of base_dir1/, base_dir2/, ... scattered in different places.

So, rsync v2.6.7 added the following "dot separator":

• rsync --relative base_dir/./subdir/file.yml destination/

This creates destination/subdir/file.yml at last.

Would such a "dot separator" work in this case too? I think we can assume that no one used any dot separator in their west-commands before (why would they have?!), meaning a new, incompatible schema and version bump is not required! I also think this dot separator syntax would be a "goldilocks" amount of schema complexity for "an internal detail surfacing in the user-facing schema".

EDIT: any other separator should work too. Maybe a separator that filesystems do NOT parse successfully would be more explicit - and then it would absolutely not require any schema version bump. I suspect rsync used a dot for CLI auto-complete reasons.

[nmunnich]

How would some generated aggregated-commands.yml not support this example?

I suppose it could, it just feels to me like it would become even more of a hacky solution, especially considering that there might be some edge case where someone does want west-commands-index to be a list.

Would such a "dot separator" work in this case too?

To me this feels like it would be an even hackier way of doing things. Especially if we wanted to make a separator that filesystems do not parse successfully, then to my eyes this wouldn't be much different to having
west-commands: "subdir/file.yml base_dir: base_dir
which I feel would be adding a hack to pretend that the schema wasn't changed.

If we were starting from scratch, the correct way of handling it would likely be to just state that the west-command-python-file paths specified in a west-commands-index file must be relative to the project root, rather than relative to the west manifest. Perhaps we can do something where we deprecate the current path handling and set the schema type as str | list[str], and accept that the round-trip cannot happen when the west-commands-index file uses a deprecated setup?