Skip to content

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

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.

Sign up for GitHub

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

Draft
nmunnich wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions src/west/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2496,7 +2496,7 @@ def _load_project(self, pd: dict, url_bases: dict[str, str], defaults: _defaults
path=path,
submodules=self._load_submodules(pd.get('submodules'), f'project {name}'),
clone_depth=pd.get('clone-depth'),
west_commands=pd.get('west-commands'),
west_commands=Path(pd.get('west-commands')).as_posix() if pd.get('west-commands') else None,
Copy link
Collaborator

@marc-hb marc-hb Mar 3, 2026

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@marc-hb marc-hb Mar 3, 2026

Choose a reason for hiding this comment

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

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

topdir=self.topdir,
remote_name=remote,
groups=groups,
Expand Down Expand Up @@ -2628,7 +2628,7 @@ def _import_path_from_project(self, project: Project, path: str) -> None:
return

for data in imported:
self._import_data_from_project(project, data, None)
self._import_data_from_project(project, data, path)

_logger.debug(f'done resolving import {path} for {project}')

Expand All @@ -2647,16 +2647,22 @@ def _import_map_from_project(self, project: Project, imp: dict) -> None:
_logger.debug(f'done resolving import {imap} for {project}')

def _import_data_from_project(
self, project: Project, data: Any, imap: _import_map | None
self, project: Project, data: Any, imap_or_mfpath: _import_map | str
) -> None:
# Destructively add the imported data into our 'projects' map.

if imap is not None:
imap_filter = _compose_imap_filters(self._ctx.imap_filter, _imap_filter(imap))
imap_path_prefix = imap.path_prefix
else:
imap_filter = self._ctx.imap_filter
imap_path_prefix = '.'
match imap_or_mfpath:
case _import_map():
imap_filter = _compose_imap_filters(
self._ctx.imap_filter, _imap_filter(imap_or_mfpath)
)
imap_path_prefix = imap_or_mfpath.path_prefix
mfst_path = imap_or_mfpath.file
case str():
imap_filter = self._ctx.imap_filter
imap_path_prefix = '.'
mfst_path = imap_or_mfpath
case _:
raise AssertionError(f'imap_or_mfpath has unexpected type {type(imap_or_mfpath)}')

child_ctx = self._ctx._replace(
imap_filter=imap_filter,
Expand All @@ -2674,13 +2680,21 @@ def _import_data_from_project(
try:
submanifest = Manifest(topdir=self.topdir, internal_import_ctx=child_ctx)
except RecursionError as e:
raise _ManifestImportDepth(None, imap.file if imap else None) from e
raise _ManifestImportDepth(None, mfst_path) from e

# Patch up any extension commands in the imported data
# by allocating them to the project.
project.west_commands = _west_commands_merge(
project.west_commands, submanifest._ctx.manifest_west_commands
)

# If the manifest was imported from a project subdirectory
# (manifest_path is a relative path within the project),
# we need to adjust the west_commands paths to be relative
# to the project root, not to the manifest subdirectory.
mfst_dir = Path(mfst_path).parent if _is_yml(mfst_path) else Path(mfst_path)
west_commands_to_merge = [
(mfst_dir / cmd).as_posix() for cmd in submanifest._ctx.manifest_west_commands
]

project.west_commands = _west_commands_merge(project.west_commands, west_commands_to_merge)

def _import_content_from_project(self, project: Project, path: str) -> ImportedContentType:
if not (self._ctx.import_flags & ImportFlag.FORCE_PROJECTS) and project.is_cloned():
Expand Down
61 changes: 61 additions & 0 deletions tests/test_manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2095,6 +2095,67 @@ def test_import_project_submanifest_commands_both(manifest_repo):
assert p1.west_commands == expected


def test_import_project_submanifest_commands_from_project_subdirectory(manifest_repo):
# 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 a string path.

with open(manifest_repo / 'west.yml', 'w') as f:
f.write('''\
manifest:
projects:
- name: p1
url: url-placeholder
import: 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',
files={
'mf_subdir/west.yml': '''\
manifest:
projects:
- name: p2
url: url-placeholder2
self:
west-commands: p2subdir/west-commands.yml
''',
},
)
checkout_branch(p1, 'master')

# Case A: import as a string path to the submanifest file.
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. See issue #725.
expected = ['mf_subdir/p2subdir/west-commands.yml']
assert p1_proj.west_commands == expected

# Case B: import using an import-map whose 'file' is a directory.
# Re-write the top-level manifest to use an import map instead of
# a string; the imported manifest still lives at mf_subdir/west.yml.
with open(manifest_repo / 'west.yml', 'w') as f:
f.write('''\
manifest:
projects:
- name: p1
url: url-placeholder
import:
file: mf_subdir
''')

# Reload and check the west_commands were resolved the same way.
p1_proj = MF().get_projects(['p1'])[0]
expected = ['mf_subdir/p2subdir/west-commands.yml']
assert p1_proj.west_commands == expected


def test_import_map_error_handling():
# Make sure we handle expected errors when loading import:
# values that are maps.
Comment on lines 2159 to 2161
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@marc-hb marc-hb Feb 19, 2026

Choose a reason for hiding this comment

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

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.

Expand Down
Loading