fix: Allow west commands to be imported from a project subdirectory if manifest is located in subdirectory

[nmunnich]

Fixes #725

Note that I relied on the test to tell me if the fix is working, I did not test it with a proper repository setup. Let me know if this is desired/this project is one where manual testing of such kind is necessary.

I'm unfamiliar with west release cycles - if this is accepted, could someone let me know when I could expect to see a new version of west released, so that we can start recommending setups relying on this to our users?

[marc-hb]

Thank you so much for taking this! See comments below.

Sorry if some comments appear twice, I had to make them again because you force pushed in the mean time. Please keep force-pushing, it's just Github support for force-pushes is pretty bad.

[nmunnich]

Thanks for the quick review! (I hope the linter is happy with me this time, uv run poe all doesn't seem to check whatever that last test is)

[nmunnich]

Ah, the formatter does run, but for whatever reason it thinks it needs to format 22 files (with no difference made to them) on my machine. I've kept that formatting on manifest.py, maybe now it'll be happy? Just another quirk of Windows I suppose.

[codecov]

Codecov Report

❌ Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.09%. Comparing base (aed10a6) to head (e4d0866).
✅ All tests successful. No failed tests found.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
- Coverage   86.09%   86.09%   -0.01%     
==========================================
  Files          11       11              
  Lines        3474     3488      +14     
==========================================
+ Hits         2991     3003      +12     
- Misses        483      485       +2     

Files with missing lines	Coverage Δ
src/west/commands.py	95.63% <100.00%> (+0.03%)	⬆️
src/west/manifest.py	95.41% <85.00%> (-0.15%)	⬇️

[nmunnich]

Hmmm. I'm not sold on adding tests to test that the exceptions are indeed raised. Don't think the AssertionError can ever be raised anyway, it's just good practice to have it there.

[marc-hb]

Hmmm. I'm not sold on adding tests to test that the exceptions are indeed raised. Don't think the AssertionError can ever be raised anyway, it's just good practice to have it there.

I agree. Error-handling is notoriously lacking coverage across the entire industry (one of the reasons software is so insecure), but I don't think we need test coverage for raise InternalBug("this should never happen").

@pdgendt is big on test coverage and rightly so, let's wait until next week when he is back. Every PR needs 2 approvals anyway.

[Copilot]

Pull request overview

This pull request fixes issue #725 where imported west-commands from project subdirectories were not resolving correctly. When a manifest file is located in a subdirectory of a project (e.g., app/west.yml), and that manifest defines custom west-commands, the paths to those commands need to be resolved relative to the manifest's location, not the project root.

Changes:

• Modified _import_path_from_project to pass the manifest path instead of None to _import_data_from_project
• Updated _import_data_from_project to accept both _import_map and str types using match/case syntax, extracting the manifest directory and prepending it to west_commands paths
• Added a test case to verify that west-commands are correctly resolved when imported from project subdirectories

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/west/manifest.py	Updated import logic to correctly resolve west-commands paths relative to manifest subdirectory location
tests/test_manifest.py	Added test case to verify west-commands resolution from project subdirectories

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

While the new test covers string path imports (e.g., import: "subdir/west.yml"), it would be beneficial to also add a test case for import maps with subdirectories (e.g., import: {file: "subdir/west.yml"}). Although both code paths converge at the same logic in _import_data_from_project, having explicit test coverage for both import styles would ensure the fix works correctly in all scenarios and prevent potential regressions.

Suggested change

	def test_import_map_error_handling():
	# Make sure we handle expected errors when loading import:
	# values that are maps.
	def test_import_project_submanifest_commands_from_project_subdirectory_import_map(
	manifest_repo,
	):
	# Similar to test_import_project_submanifest_commands_from_project_subdirectory,
	# but this tests using an import map with a 'file' key instead of a string path.
	# When a manifest is imported from a project subdirectory (e.g., mf_subdir/west.yml),
	# and that manifest defines west-commands, the paths should be resolved relative to
	# the manifest subdirectory. This tests _import_path_from_project with an import map.
	with open(manifest_repo / 'west.yml', 'w') as f:
	f.write('''\
	manifest:
	projects:
	- name: p1
	url: url-placeholder
	import:
	file: mf_subdir/west.yml
	''')
	p1 = manifest_repo / '..' / 'p1'
	create_repo(p1)
	create_branch(p1, 'manifest-rev', checkout=True)
	add_commit(
	p1,
	'add mf_subdir/west.yml with west-commands (import map)',
	files={
	'mf_subdir/west.yml': '''\
	manifest:
	projects:
	- name: p2
	url: url-placeholder2
	self:
	west-commands: p2subdir/west-commands.yml
	''',
	},
	)
	checkout_branch(p1, 'master')
	p1_proj = MF().get_projects(['p1'])[0]
	# The west_commands path should be 'mf_subdir/p2subdir/west-commands.yml',
	# not 'west-commands.yml', to be resolved correctly relative to the project root.
	expected = ['mf_subdir/p2subdir/west-commands.yml']
	for a, e in zip(p1_proj.west_commands, expected, strict=True):
	assert PurePath(a) == PurePath(e)
	def test_import_map_error_handling():
	# Make sure we handle expected errors when loading import:
	# values that are maps.

[marc-hb]

That's an interesting suggestion. But that's also a MASSIVE duplication of the other test code for very little additional coverage :-( @nmunnich maybe you could easily combine both tests (yours and the copilot ~duplicate) into a single test like this:

        manifest:
          projects:
           
          - name: pA
            url: url-placeholderA
            import: mf_subdirA/west.yml

          - name: pB
            url: url-placeholderB
            import:
               file: mf_subdirB/west.yml

Then you can re-use the identical add_commit() twice in both A and B.

[Copilot]

Path should be PurePosixPath to ensure cross-platform compatibility. Git stores paths internally using POSIX-style forward slashes, even on Windows. Using Path on Windows will create paths with backslashes, which may not work correctly. The codebase already uses PurePosixPath in similar contexts (see line 312).

Suggested change

	mfst_dir = Path(mfst_path).parent
	mfst_dir = PurePosixPath(mfst_path).parent

[pdgendt]

@marc-hb maybe related to #921 ?

[marc-hb]

Yes, related to #921 (which I just demoted to draft).

On one hand we should stick to Pathlib abstractions, avoid hardcoding forward slashes and not use Unix paths objects internally when running on Windows. This is not just about backslashes, there are other differences.

On the other hand we don't want west manifest --resolve to convert forward slashes to backslashes on Windows. EDIT: or do we? See #928

I need again more time to think about it and do more testing, sorry :-(

[marc-hb]

This is a much bigger can of worms than I expected. I wish I could "unsee" it but I can't :-(

For instance, on Windows:

west list -f '{path}'      # forward slashes
west list -f '{abspath}'   # backslashes 
west list -f '{posixpath}' # forward slash
west list topdir           # absolute and forward since #375

https://docs.zephyrproject.org/latest/develop/west/west-apis.html#west.manifest.Project

Also relevant:

• Overhaul path handling #411 and more
• topdir: west topdir prints posix style path. #375

[nmunnich]

Personally, I would argue that resolving that inconsistency is out of scope here, and I would prefer not to block waiting for a fix/resolution. Would you be alright with taking the PurePosixPath suggestion and going forward with that here?

[nmunnich]

I've gone through the pain of doing a bunch of testing of actual imports in PowerShell. This may be simpler than we think:

• No matter what path is set to, it always resolves in west manifest --resolve with /. So path: modules/\//\/\/\\\\/\//\./\zmk\/\/\/\.\\\/\test turned into modules/zmk/test.
• No matter what west-commands is set to, it remained the same in the west manifest. In addition, regardless of how many extra \ and / combinations I put, my dummy west command was being picked up and executed correctly.

I've now modified the code somewhat, so that the west-commands will always resolve with posix paths, for consistency with path. I noticed a comment on L2468 that seems relevant to the conversation, and took the approach used there.

I did notice another bug while doing my testing:

manifest:
  projects:
    - name: p1
      url: placeholder
      revision: west-test
      import: app/west.yml
      west-commands: app/scripts/west-commands.yml

west-commands:
  - file: test/west-commands/west.py
    commands:
      - name: west
        class: West
        help: Print hello world to the console

When resolving this, while the west-commands.yml file is located correctly and the commands are visible in west help, trying to actually execute the command doesn't work because west is trying to resolve it as <module-path>/test/west-commands/west.py rather than <module-path>/app/test/west-commands/west.py. My current thinking is that we need to add manifestdir as a property to Project, so that we can refer to it on L691 of commands.py. That feels like a rather extensive change though, would you have any alternative idea?

[marc-hb]

I've been working on adding some 2 "backslash" tests related to this (#928). I have this one ready but I want to finish a similar one for west-commands:
https://github.com/marc-hb/west/commits/backslash-tests/

I should be able to get the last one ready on Friday. I would like these "pure" tests to be merged before any change (feature or bug fix) in west itself.

Just a heads-up, sorry for not answering your comment yet - will do that on Friday too.

[marc-hb]

I would like these "pure" tests to be merged before any change (feature or bug fix) in west itself.

• Add test coverage for special characters in manifest and west commands #928

[marc-hb]

Sorry I'm paying attention to this only now:

I did notice another bug while doing my testing:

projects:
    - name: p1
      import: app/west.yml
      west-commands: app/scripts/west-commands.yml

First, I'm really not a fan of such "shortcuts + import" combinations BTW. This is adding west-commands from another git repo directly and "shortcutting" the import app/west.yml which could also add the same west-commands.yml file a second time! Does it get added a second time in your test case? I don't think you said. If yes, is that second time an error? A warning? I hope it's documented somewhere but I'm not holding my breath. Also, why adding the same west-commands.yml file twice?

If app/west.yml is not adding west-commands.yml second time then why is it not in app/west.yml? Why force every app/west.yml importer to manually add extensions?

Don't get me wrong: I don't see anything wrong with having the west-commands: shortcut ALONE without the import. But if a project becomes big and bold enough to have its own west.yml manifest, then shouldn't that submanifest import be a the single module "entry point" and take care of implementation details like the location of west-commands too? Or am I missing some real-world use case here?

All this being said, the ship has sailed: this has been allowed so the behavior should be specified. But: can we deal with this separately from #920? (which has got surprisingly more complicated than I expected already). If yes separately, could you please file a separate bug? Thx in advance.

cc:

• Clean way to import a west.yml's west commands but no submodule #903

[nmunnich]

In this case the bug I was trying to describe is that which is shown by the newest test and fixed in this PR in the last commit. Writing it this way was simply a way to avoid writing the app/west.yml where the script gets imported down, I think but can't recall completely that I simply didn't import the commands in app/west.yml when testing this.

The interaction I care about is that of the newest test, and I agree that any interaction with multiple import locations of the same westcommands file should be delt with separately from this.

backslashes :-(

[marc-hb]

This normalizes west-commands: values for projects but not for self which seems to come from somewhere else :-(

[marc-hb]

If we go for west-commands normalization, then maybe just change the type to a Path across the board which can help catch inconsistencies.

[marc-hb]

As discussed previously, this PR breaks my new tests submitted in #928. It's probably OK and these tests in #928 can likely be adjusted but it needs to be discussed because it's still a technically backwards-incompatible change. The easiest way to discuss and to record that behavior change is to merge #928 to fill the test coverage gap :-)

[marc-hb]

One more thought: this needs to be split into at least 2 commits, maybe 2 PRs:

1. the west-commands Path normalization
2. the prefix bug fix

[nmunnich]

Just an fyi, I think it is unlikely that I'll find the time to continue on this until next month. If it's still open in April I'll have another go, but I'm encouraging anyone who has the time to continue this sooner.

[marc-hb]

One more thought: this needs to be split into at least 2 commits, maybe 2 PRs:

1. the west-commands Path normalization
2. the prefix bug fix

I can do 1. after #928 is merged. I can probably rebase 2. as well, should be trivial enough (famous last words)

[nmunnich]

Rebased onto main. I also added a test and a fix for the west-commands not being imported correctly after the manifests are imported.

[marc-hb]

This PR opened a big backslash / PosixPath can of worms that kept both of us busy for a while in this comment: #920 (comment)

But I don't see any corresponding test as of the latest version 6f600e3.

Please:

• use special characters like I did in #928
• add some naive assert(west manifest --resolve) at the end of the relevant tests.

If this test is "naive" then I'm afraid it will fail either on Linux or on Windows but cannot pass on both at the same time. Then we can discuss whether we make the test Windows-specific like I did in #928, or whether we change west behavior to make the output the same on both. I will work on either option.

[marc-hb]

I don't think you can use _is_yml() here. When looking at other places where _is_yml() is already used, you can see it is used only to collect all submanifests/*.{yml,yaml} files. _is_yml() is not used to decide whether submanifests is a file or a directory.

In the same places where _is_yml() is used, you will find the code that decides whether submanifests is a file or a directory. That code seems consistent with the documentation (but please check).

I'm afraid this could require a git query in some cases.

BTW this call to _is_yml() seems new, I can't find it in earlier revision 51c2f44. What changed?

[marc-hb]

Is this "hiding" manifest-rev to make sure the import sticks to it anyway and does not look at the checkout instead? If I'm correct, that's subtle west behavior and worth a one-line comment. If misunderstood this, then maybe this is worth another comment :-)

[nmunnich]

I have no idea about this one - I was simply following the same format used in many other tests in this file, see e.g. L2110.

[marc-hb]

Can you start from west_init_tmpdir instead of repos_tmpdir and avoid this? Sorry if not possible.

[nmunnich]

Does not seem to be possible. I think it's initialised too early, but have not dug deeper.

[nmunnich]

This PR opened a big backslash / PosixPath can of worms that kept both of us busy for a while in this comment: #920 (comment)

But I don't see any corresponding test as of the latest version 6f600e3.

Please:

* use special characters like I did in #928

* add some naive `assert(west manifest --resolve)` at the end of the relevant tests.

If this test is "naive" then I'm afraid it will fail either on Linux or on Windows but cannot pass on both at the same time. Then we can discuss whether we make the test Windows-specific like I did in #928, or whether we change west behavior to make the output the same on both. I will work on either option.

I'm quite uncertain what you mean here. My understanding of the situation is that I happened to be developing on Windows, and that ended up finding an inconsistency in the way West has been handling filepaths. I still don't really consider it in the scope of this PR to test or fix this inconsistency - my goal here is to fix the bug described by the original issue.