west: Add untracked files and folders to west status output #702
Conversation
Add untracked files and folder to west status output. This helps to keep the west checkout clean by alerting the user of folders that are no longer tracked by west. This is useful for detecting when a module has been removed, or moved. and is no longer updated by west update. Add the ability to ignore folders and files through a comma separated list in the west config file. Example: west config status.ignore .vscode/,optional/ Fixes: zephyrproject-rtos#622 Signed-off-by: Joakim Andersson <joakim.andersson@nordicsemi.no>
There was a problem hiding this comment.
Very quick review while this is "hot", apologies for any misunderstanding.
Parts of the code are hard to read and there's zero comment. Please name symbols more carefully and comment the least intuitive parts of the code.
New tests missing for this (non-trivial) feature.
As already discussed in #622. nested west projects are probably the trickiest use cases for this. Unsurprisingly, west seems to have a "don't ask, don't tell" attitude with respect to nested projects.
At the very least you need to give detailed examples of how nested projects handled and some tests for these use cases.
BTW at least one test is failing right now with the current commit:
https://github.com/zephyrproject-rtos/west/actions/runs/7520440151/job/20470116532?pr=702. I haven't looked why.
Various other code issues, see inline.
Thanks for the PR but I'm a bit concerned about the typical "How hard could this be? Famous last words" effect, especially wrt. to nested projects. Could these be just left for git to show?
| @@ -844,6 +844,53 @@ def do_run(self, args, user_args): | |||
| failed.append(project) | |||
| self._handle_failed(args, failed) | |||
|
|
|||
| # List projects not tracked by manifest. | |||
| projects = [project.abspath for project in self._cloned_projects(args, only_active=True)] | |||
There was a problem hiding this comment.
| projects = [project.abspath for project in self._cloned_projects(args, only_active=True)] | |
| active_projects = [project.abspath for project in self._cloned_projects(args, only_active=True)] |
One of the purposes is to make a difference between active an inactive projects but this first line does not.
| topdir = Path(self.topdir) | ||
| untracked = [] | ||
|
|
||
| self.do_find_untracked(topdir, projects, untracked) |
There was a problem hiding this comment.
| self.do_find_untracked(topdir, projects, untracked) | |
| untracked = self.do_find_untracked(topdir, active_projects, projects) |
Non-obvious mix of "OUT" and "IN" parameters is very confusing.
|
|
||
| if len(untracked) > 0: | ||
| self.banner('untracked:') | ||
| for project in untracked: |
There was a problem hiding this comment.
| for project in untracked: | |
| for entry in untracked: |
You seem to assume that anything in topdir is either an active or inactive "project". That's not true for everyone, in fact we (SOF) use topdir as a very convenient place to place build directories - very convenient because it does not require any .gitignore-like file (yet!)
| else: | ||
| untracked.append(os.path.relpath(entry.path, Path(self.topdir)) + '/') | ||
| if entry.is_file(): | ||
| untracked.append(os.path.relpath(entry.path, Path(self.topdir))) |
There was a problem hiding this comment.
You're using a mix or Path.relative_to() and os.path.relpath(). If there's a reason why then add a short comment explaining why, otherwise stick to Path.
Generally speaking this what this code does is not always very obvious and there's zero zero comment right now.
| return False | ||
|
|
||
| def do_find_untracked(self, path, projects, untracked): | ||
| obj = os.scandir(path) |
There was a problem hiding this comment.
| obj = os.scandir(path) | |
| entries = os.scandir(path) |
This is a list, use plural. Also, stick to the standard Python terminology when possible.
| for project in untracked: | ||
| self.inf(textwrap.indent(f'{project}', ' ' * 4)) | ||
|
|
||
| def is_manifest_subfolder(self, path, projects): |
There was a problem hiding this comment.
Either I don't understand this function, or the name is very confusing. Do you mean something like is_in_some_west_project()?
The manifest is a list of git projects, it's not the projects themselves.
Anyway you should not need such a path comparison because the directory walk should prune west projects as soon as it finds one?
| ignored_config = self.config.get('status.ignore') | ||
| ignored = ignored_config.split(',') if ignored_config else [] | ||
| # Ignore .west folder in untracked projects | ||
| ignored.append('.west/') |
There was a problem hiding this comment.
Why would anyone have a .west/ folder in untracked projects? This looks like a problem most people would want to see (while others can explicitly ignore it)
| pass | ||
|
|
||
| if len(untracked) > 0: | ||
| self.banner('untracked:') |
There was a problem hiding this comment.
| self.banner('untracked:') | |
| self.banner('Unknown:') |
Please do not re-use the same presentation as git to avoid misleading users into thinking this line comes from git.
| for ignore in ignored: | ||
| try: | ||
| untracked.remove(ignore) | ||
| except: |
There was a problem hiding this comment.
Does something this mundane really need a try/except? Dubious.
| self.do_find_untracked(topdir, projects, untracked) | ||
|
|
||
| ignored_config = self.config.get('status.ignore') | ||
| ignored = ignored_config.split(',') if ignored_config else [] |
There was a problem hiding this comment.
We need a separate .westignore file for this for consistency with git and others. This will let users put each pattern on a separate line and let them ignore files with commas in their names.
|
Please re-open if still interested. |
Add untracked files and folder to west status output. This helps to keep the west checkout clean by alerting the user of folders that are no longer tracked by west.
This is useful for detecting when a module has been removed, or moved. and is no longer updated by west update.
Add the ability to ignore folders and files through a comma separated list in the west config file.
Example:
west config status.ignore .vscode/,optional/
Fixes: #622