Skip to content
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.

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

manifest: Do not allow projects inside the west directory #754

Merged
merged 3 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/west/app/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def do_run(self, args, _):
This forces west to search for a workspace there.
Try unsetting ZEPHYR_BASE and re-running this command.''')
else:
west_dir = Path(self.topdir) / '.west'
west_dir = Path(self.topdir) / WEST_DIR
msg = ("\n Hint: if you do not want a workspace there, \n"
" remove this directory and re-run this command:\n\n"
f" {west_dir}")
Expand Down Expand Up @@ -2094,7 +2094,7 @@ def projects_unknown(manifest, projects):
#

# Top-level west directory, containing west itself and the manifest.
WEST_DIR = '.west'
WEST_DIR = util.WEST_DIR

# Default manifest repository URL.
MANIFEST_URL_DEFAULT = 'https://github.com/zephyrproject-rtos/zephyr'
Expand Down
4 changes: 2 additions & 2 deletions src/west/configuration.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
from typing import Any, Dict, Iterable, List, Optional, Tuple, TYPE_CHECKING
import warnings

from west.util import west_dir, WestNotFound, PathType
from west.util import WEST_DIR, west_dir, WestNotFound, PathType
Copy link
Collaborator

Choose a reason for hiding this comment

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

not wrong, but looks a bit strange to import both WEST_DIR and west_dir.
Same name, just different casing.

Easy to confuse readers.

Why not just use west_dir(), when we anyway import it, instead of doing path concatenation.
(see other comment).


class MalformedConfig(Exception):
'''The west configuration was malformed.
Expand Down Expand Up @@ -616,7 +616,7 @@ def _location(cfg: ConfigFile, topdir: Optional[PathType] = None,
return env['WEST_CONFIG_LOCAL']

if topdir:
return os.fspath(Path(topdir) / '.west' / 'config')
return os.fspath(Path(topdir) / WEST_DIR / 'config')
Copy link
Collaborator

Choose a reason for hiding this comment

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

west_dir() supports a dir argument, and if that dir is having the .west then the logic returns immediately.

This means we can have a uniform way to always fetch west_dir() and thus keep the knowledge of how .west is placed under topdir at a single location (west.util).

Doing so will also remove the need of import WEST_DIR.

Suggested change
return os.fspath(Path(topdir) / WEST_DIR / 'config')
return os.fspath(Path(west_dir(topdir)) / 'config')

Ref:

west/src/west/util.py

Lines 71 to 73 in c3aadf5

while True:
if (cur_dir / '.west').is_dir():
return os.fspath(cur_dir)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, applied your suggestion here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to revert here, it's possible that the path does not exist yet. So it would require more changes.

I'll create a follow-up PR for v1.4 or v2.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Had to revert here, it's possible that the path does not exist yet. So it would require more changes.

As usual, initialization/"bootstrapping" is at the top of complex situations. Always underestimated :-)


if find_local:
# Might raise WestNotFound!
Expand Down
4 changes: 4 additions & 0 deletions src/west/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -2435,6 +2435,10 @@ def _load_project(self, pd: Dict, url_bases: Dict[str, str],
f'normalizes to {ret_norm}, which escapes '
f'the workspace topdir')

if Path(ret_norm).parts[0] == util.WEST_DIR:
self._malformed(f'project "{name}" path {ret.path} '
f'is in the {util.WEST_DIR} directory')

return ret

def _validate_project_groups(self, project_name: str,
Expand Down
6 changes: 4 additions & 2 deletions src/west/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
# otherwise, but it doesn't seem worth it.
PathType = Union[str, os.PathLike]

WEST_DIR = '.west'

def escapes_directory(path: PathType, directory: PathType) -> bool:
'''Returns True if `path` escapes parent directory `directory`.

Expand Down Expand Up @@ -58,7 +60,7 @@ def west_dir(start: Optional[PathType] = None) -> str:

Raises WestNotFound if no .west directory is found.
'''
return os.path.join(west_topdir(start), '.west')
return os.path.join(west_topdir(start), WEST_DIR)

def west_topdir(start: Optional[PathType] = None,
fall_back: bool = True) -> str:
Expand All @@ -69,7 +71,7 @@ def west_topdir(start: Optional[PathType] = None,
cur_dir = pathlib.Path(start or os.getcwd())

while True:
if (cur_dir / '.west').is_dir():
if (cur_dir / WEST_DIR).is_dir():
return os.fspath(cur_dir)

parent_dir = cur_dir.parent
Expand Down
7 changes: 7 additions & 0 deletions tests/manifests/invalid_project_in_west_dir.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Project paths can't be .west.

manifest:
projects:
- name: nope
url: ignore-me
path: .west
7 changes: 7 additions & 0 deletions tests/manifests/invalid_project_in_west_reldir.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Project paths can't be .west after normalization.

manifest:
projects:
- name: nope
url: ignore-me
path: foo/../.west
7 changes: 7 additions & 0 deletions tests/manifests/invalid_project_in_west_relsubdir.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Project paths can't be in .west normalized.

manifest:
projects:
- name: nope
url: ignore-me
path: foo/../.west/some/path
7 changes: 7 additions & 0 deletions tests/manifests/invalid_project_in_west_subdir.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Project paths can't be in .west/.

manifest:
projects:
- name: nope
url: ignore-me
path: .west/some/path