For convenience, Project.git() supports passing either a list (good) or
a string with whitespaces (bad). The latter is parsed with shlex.split()
This saves some typing but the caller has to be extremely careful to
never use the shlex.split() convenience with unsanitized inputs.
Fixes commit 3ac600acaa ("git: clean west ref space after fetching")
where the caller was not careful and concatenated `update-ref -d ` with
unsanitized input, possibly containing special characters as found in
bug #679. Fix this bug by converting the string to a list.
While at it, look for a few other, frequent and risky invocations and
convert their string argument to a list too. The following test hack was
used to semi-automate the search for these other locations:
```
--- a/src/west/manifest.py
+++ b/src/west/manifest.py
@@ -897,6 +897,8 @@ class Project:
:param cwd: directory to run git in (default: ``self.abspath``)
'''
if isinstance(cmd, str):
+ print(cmd)
+ breakpoint()
cmd_list = shlex.split(cmd)
else:
cmd_list = list(cmd)
```
While at it, also convert to a list a couple non-risky but very frequent
invocations. This speeds up the test hack above.
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
It's not unusual to make "quick and dirty", temporary changes in a git
repo that is on the manifest: either add some untracked files or create
a branch. As expected, this makes the repository show in `west
compare`. If such repos and the manifest repo are the only repos
appearing in the `west compare` output, then no manifest-rev + HEAD
banner ever appeared. When no banner ever appears it's really not
obvious which repos are on the manifest-rev vs not. In other words: the
main `west compare` feature is defeated.
Clear this doubt by always showing the banner, even when `HEAD` and
`manifest-rev` are the same. See sample output below.
I think the original design idea was to follow diff's "spirit" not to
show anything that's identical and to save as many lines as
possible. However I don't think it works in this particular case because
invoking `git status` for a repo _without_ showing where its HEAD is at
is way too subtle, _especially_ when there is no other repo with a
banner to provide a comparison point and some contrast. Explicitly and
consistently prefixing every `git status` output with a manifest-rev
banner is much more clear and obvious.
Moreover, `git status` output is relatively verbose already so always
prefixing it with a 2 lines long banner makes negligible difference to
the total.
Before:
```
$ west compare
=== hal_xtensa (modules/hal/xtensa):
--- status:
On branch somebranch
nothing to commit, working tree clean
```
After:
```
$ west compare
=== hal_xtensa (modules/hal/xtensa):
--- manifest-rev: 41a631d4aeee (somebranch, upstream/master) cmake: enable using SOC_TOOLCHAIN_NAME ...
HEAD: 41a631d4aeee (somebranch, upstream/master) cmake: enable using SOC_TOOLCHAIN_NAME ...
--- status:
On branch somebranch
nothing to commit, working tree clean
```
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
The tox indirection layer is convenient except for the usual problems
caused by too many layers of indirection:
- Add some examples and show how it is still possible to use important
pytest features. This is also useful to boost people familiar with
Python but not with pytest.
- To stop developers wasting their time trying, document a major
limitation with the current approach: impossible to run the west
code in a debugger when it's started from a test.
- Show how the "printf" alternative can work.
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
This is required to test code that includes temporary test hacks that
don't pass style checks.
People write ugly code first. Then they make it prettier.
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
When setting up a project with west, the target directory may not be
initialized correctly. In the typical case, if a directory named
`./zephyr/` already exists, the user may find that checkout files are
located at `./zephyr/manifest-tmp/*` instead of the expected
`./zephyr/*`.
This patch will abort and refuse to complete `west init` if the
destination directory alread exists. This check would ideally occur
before the potentially lengthy clone operation, but `manifest_path` is
derived from the files retrieved...
NOTE: If the project quotes a value other than `zephyr` for
`manifest.self.path` in `/west.yml`, then this will affect that
directory instead.
Steps to reproduce before this patch:
mkdir ./zephyr/
west init ./ -m https://github.com/zephyrproject-rtos/zephyr.git
ls -l ./zephyr/
Possible fix for some of the symptoms described in #558
Signed-off-by: Attie Grande <attie.grande@argentum-systems.co.uk>
Fixes commit 1d220bbd62 ("commands: add grep")
Fix misplaced closing parenthese in:
```
shlex.split(self.config.get(f'grep.{tool}-args'), '')
```
The empty string is meant as a default value for config.get() but it was
passed as a second argument to shlex.split() by mistake.
Funny enough this hangs forever:
```
>>> shlex.split(None, comments='')
<stdin>:1: DeprecationWarning: Passing None for 's' to shlex.split() is deprecated.
(hangs forever)
```
PS: debuggers rulez
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
This has been requested various times. I haven't done it because 'west
forall -c "git grep"' has been a reasonable workaround for me, and I
never had time or need to think about what an ergonomic alternative
might look like.
That's changed now and I wanted something that would only print output
for projects where a result was found, similarly to the way "west
diff" only prints for projects with nonempty "git diff", etc.
Add a "grep" command that does similar, defaulting to use of "git
grep" to get the job done. Also support (my favorite) ripgrep and
plain "grep --recursive". See the GREP_EPILOG variable in the patch
for detailed usage information.
Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
This is useful when we want to be able to control whether a newline is
appended or not. The die() method is intentionally omitted: this is a
"scream and exit the process" method, and always flushing any
line-buffered I/O makes sense to enforce in this context.
Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
The WestCommand API for running subprocesses is OK, but
is missing some useful features; add them now:
- The current recommendation in the subprocess module is
to use subprocess.run(), so add a helper for doing that
since it's missing. (The name asymmetry is because
there is already a WestCommand.run() claimed.)
- Allow subprocess invocations to take any kwargs we want
Signed-off-by: Martí Bolívar <mbolivar@amperecomputing.com>
Clarify the user's options a bit more.
Requested-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Make this option consistent with all others by only having the highest
precedence configuration option take effect. We revisited this
decision following review.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The top level comment is not totally accurate. The 'project_importer'
attribute, for example, doesn't change.
The comment around 'project_filter' reflects an idea I had but
abandoned so that project filter behavior would be more consistent
with group filter behavior.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This reverts commit d0e6b9a54b.
West was right, the docs are wrong/inconsistent.
The commit I'm reverting was following the west documentation it
referenced correctly, but the documentation itself was wrong (and in
fact was inconsistent with earlier documentation on the same page).
The basis for making commit d0e6b9a54 was that the docs currently say:
In other words, let:
- the submanifest resolved from self-import have group filter self-filter
- the top-level manifest file have group filter top-filter
- the submanifests resolved from import-1 through import-N have
group filters filter-1 through filter-N respectively
The final resolved group-filter value is then filter1 + filter-2 +
... + filter-N + top-filter + self-filter, where + here refers to
list concatenation.
But earlier in the same section, they also say:
The resolved manifest has a group-filter value which is the result
of concatenating the group-filter values in the top-level manifest
and any imported manifests.
Manifest files which appear earlier in the import order have
higher precedence and are therefore concatenated later into the
final group-filter.
Where the "import order" is defined higher up as:
Importing is done in this order:
1. Manifests from self-import are imported first.
2. The top-level manifest file’s definitions are handled next.
3. Manifests from import-1, …, import-N, are imported in that order.
This means that import-1 happens **earlier** than import-N, and should
therefore have **higher** precedence when it comes to computing the
final group filter. But putting import-N **later** in the concanated
group filter actually means import-1 has **lower** precedence. So the
docs are inconsistent.
To fix the docs so they are consistent and match west's current
behavior, we should replace this:
The final resolved group-filter value is then filter1 + filter-2 + ...
+ filter-N + top-filter + self-filter, where + here refers to list
concatenation.
with this:
The final resolved group-filter value is then filter-N + ... +
filter-1 + top-filter + self-filter, where + here refers to list
concatenation.
That is:
- docs currently go from: filter-1 to filter-N
- they should go from: filter-N to filter-1.
Fixes: #663
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Use a deque instead of a list when accumulating group filters during
imports. This allows us to preserve the boundaries of each filter
list, which in turn allows us to improve the debug output.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Internal discussion at Nordic indicates that the semantics for 'west
manifest --resolve' should probably be something along the lines of
"resolve the manifest but ignore the value of manifest.project-filter,
and print the resolved result". This is consistent with the way that
these commands are not affected by the value of manifest.group-filter.
However, we don't have a way to support this right now: west.manifest
has no API for loading itself but with manifest.project-filter
ignored. Such an API would be straightforward to add, but we don't
have time for that right now, as we're under time pressure to add
support for this option to resolve an issue during the zephyr v3.4
stabilization period.
For now, work around the issue by just erroring out if the option is
set at all, telling the user that they can get in touch with us
if they need this. We'll let the level of noise that results be
our guide in prioritizing this enhancement.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Add support for this option. The option's value is a comma-separated
list of regular expressions, each prefixed with '+' or '-', like this:
+re1,-re2,-re3
Project names are matched against each regular expression (re1, re2,
re3, ...) in the list, in order. If the entire project name matches
the regular expression, that element of the list either deactivates or
activates the project. The project is deactivated if the element
begins with '-'. The project is activated if the element begins with
'+'. (Project names cannot contain ',' if this option is used, so the
regular expressions do not need to contain a literal ',' character.)
If a project's name matches multiple regular expressions in the list,
the result from the last regular expression is used. For example,
if manifest.project-filter is:
-hal_.*,+hal_foo
Then a project named 'hal_bar' is inactive, but a project named
'hal_foo' is active.
If a project is made inactive or active by a list element, the project
is active or not regardless of whether any or all of its groups are
disabled. (This is also now the only way to make a project that has no
groups inactive.)
Otherwise, i.e. if a project does not match any regular expressions in
the list, it is active or inactive according to the usual rules
related to its groups.
Within an element of a manifest.project-filter list, leading and
trailing whitespace are ignored. That means these example values
are equivalent:
+foo,-bar
+foo , -bar
Any empty elements are ignored. That means these example values are
equivalent:
+foo,,-bar
+foo,-bar
The lists in the manifest.project-filter options in the system,
global, and local configuration files are concatenated together.
For example, on Linux, with:
/etc/westconfig:
[manifest]
project-filter = +foo
~/.westconfig:
[manifest]
project-filter = -bar_.*
<workspace>/.west/config:
[manifest]
project-filter = +bar_local
Then the overall project filter when within the workspace <workspace>
is:
+foo,-bar_.*,+bar_local
This lets you set defaults in the system or global files that can be
overridden within an individual workspace as needed, without having to
maintain the defaults across multiple workspaces.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This adds initial support for loading a 'manifest.project-filter'
configuration option.
This option is special in that its values in the system, global,
and local configuration files are all considered at the same time,
rather than just whichever one is defined at highest precedence.
This is because the eventual purpose of this configuration option
is to allow people to deactivate or activate projects using the
configuration file system, and it seems nicer to allow people to
progressively refine a filter instead of having to synchronize
global settings that they always want applied into each workspace
they have or may create.
This patch doesn't actually do anything with the option values besides
defining the internal representation, validating the options on the
file system, and saving the results in the import context state that
we carry around while importing projects. Applying the filter itself
will come in future patches. See source code comments for details on
behavior (these will eventually make their way into the
documentation).
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
In order to support the manifest.project-filter configuration option,
we need to forbid certain characters from occurring in project names.
Making that happen is a backwards incompatible change, however, so
phase it in by emitting a warning.
We will make it a hard error if manifest.project-filter is set later
on. That way, users will only see an error if they are opting into the
latest west for now, and other users will get a warning and have time
to migrate their projects.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This will still fail or succeed in the same situations,
but it is convenient to do it this way as prep work for
future enhancements.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Commit 92c18ac55a
("main: move verbose manifest logging to project.py")
did not achieve its purpose.
We are loading the manifest long before we call the setup_logging()
methods in each of the project.py classes, so any messages from the
manifest class have long been discarded by the time that we enable
them.
Now that we have EarlyArgs in main.py, we can use that to centralize
logging and enforce the following:
- warnings and above for west APIs are emitted by default
- info and above emitted with west -v
- debug and above with west -vv or higher
Fixes: #665
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Right now if you run a zephyr extension like 'west build' outside of a
workspace, argparse says:
[...] invalid choice: ‘build’ [...]
This is because argparse's subcommand parser doesn't seem to have any
API to add a catch-all value for when the user provides an unknown
command, so it expects that exactly the subcommands we told it about
are available.
This is confusing to users, and now that we have our own EarlyArgs
parsed, we can do better by printing some west-specific help if we
aren't in a workspace:
usage: west [-h] [-z ZEPHYR_BASE] [-v] [-V] <command> ...
west: unknown command "build"; do you need to run this inside a
workspace?
as well as if you are:
usage: west [-h] [-z ZEPHYR_BASE] [-v] [-V] <command> ...
west: unknown command "foo"; workspace /home/mbolivar/zp does
not define this extension command -- try "west help"
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
We've been avoiding this for a long time, but we can't any longer
and it's time to bite the bullet.
There are some chicken-and-egg problems in our argument parsing that
can only be handled if we do some manual command line argument parsing
to determine what the common options (like --verbose) are set to,
and to determine what the command name (if any) is.
For example, we would ideally like to know the verbosity level of the
west command *before* we set up logging for the west.manifest module,
so that we can set the log level for that module appropriately.
Right now, we can't do that, because we load the manifest to figure
out what the extension commands are, and from there delegate to
argparse to count the '--verbose' options. By the time argparse runs,
then, it's too late: we already loaded the manifest, and any log
messages from west.manifest are lost.
Enable solving this and other problems by writing our own, very
limited, command line parser that just figures out the global
options (west -hvVz) and the command name. We won't need all of this
right away, but we might as well try to be complete from the start.
This is prep work only; we'll put it to work in later patches.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Moving all the "work" of main() into WestApp.run() will
help us in future patches where we need to set up logging
in a way that requires internal state from that object.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The error handling in the _load_imap() function is not being tested.
This was discovered by manually inspecting the HTML coverage data.
Add statement coverage for the error handling.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is a more convenient way to construct manifests, so we might as
well have it around and use it to save some typing.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Fixing the test case logic causes it to fail, so mark it xfail.
This is bug #663.
The test case test_group_filter_self_import() is incorrect, which
conveniently enough provides steps to reproduce.
The test case should read as follows (patch applies to f6f5cf6):
diff --git a/tests/test_manifest.py b/tests/test_manifest.py
index 14f2941..e934b71 100644
--- a/tests/test_manifest.py
+++ b/tests/test_manifest.py
@@ -2828,7 +2828,7 @@ def test_group_filter_imports(manifest_repo):
sha2 = setup_project('project2', '[+gy,+gy,-gz]')
v0_9_expected = ['+ga', '-gc']
- v0_10_expected = ['-ga', '-gb', '-gc', '-gw', '-gy', '-gz']
+ v0_10_expected = ['-ga', '-gb', '-gc', '-gw', '-gz']
#
# Basic tests of the above setup.
In other words, west incorrectly concludes that group 'gy' is disabled
in this scenario, when it should be enabled.
The test creates the following layout, where ./mp/west.yml is the main
manifest:
───────────────────────────────────────
File: ./mp/west.yml
───────────────────────────────────────
manifest:
version: "0.10"
group-filter: [+ga,-gc]
projects:
- name: project1
revision: ...
import: true
- name: project2
revision: ...
import: true
self:
import: self-import.yml
..
───────────────────────────────────────
───────────────────────────────────────
File: ./project1/west.yml
───────────────────────────────────────
manifest:
group-filter: [-gw,-gw,+gx,-gy]
───────────────────────────────────────
───────────────────────────────────────
File: ./project2/west.yml
───────────────────────────────────────
manifest:
group-filter: [+gy,+gy,-gz]
───────────────────────────────────────
───────────────────────────────────────
File: ./mp/self-import.yml
───────────────────────────────────────
manifest:
group-filter: [-ga,-gb]
───────────────────────────────────────
The west docs say:
In other words, let:
- the submanifest resolved from self-import have group filter self-filter
- the top-level manifest file have group filter top-filter
- the submanifests resolved from import-1 through import-N have
group filters filter-1 through filter-N respectively
The final resolved group-filter value is then filter1 + filter-2 +
... + filter-N + top-filter + self-filter, where + here refers to
list concatenation.
- https://docs.zephyrproject.org/latest/develop/west/manifest.html
Applying these rules, the final filter should be concatenated from
./project1/west.yml, ./project2/west.yml, ./mp/west.yml,
./mp/self-import.yml, in that order. Since neither ./mp/west.yml nor
./mp/self-import.yml have a group filter which affects gy, the result
should be that it is enabled, since ./project2/west.yml enables it
explicitly.
Fix the test so it matches the expected behavior. We'll fix the
implementation in a separate commit.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The filename attribute no longer exists. Instead, we have a better
str() result for a ManifestImportFailed. Use it instead.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
There's a field whose initialization procedure differs enough
from the comment describing it that it's worth clarifying.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
When a project is not on the `manifest-rev` it will often be on a branch
or tag. Use the `%d` format to make `west compare` show that
information.
As discussed in #643 and before, git status cannot be fully trusted to
display branch information.
Sample new output:
```
west compare
=== zephyr (zephyr):
--- manifest-rev: e40859f78712 (some_tag) Revert "dma: dw: Do ...
HEAD: e803b77463b4 (zephyrproject/main) samples: net: ...
--- status:
HEAD detached at zephyrproject/main
nothing to commit, working tree clean
```
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Isolate test_compare() from the environment's west config. This makes
the test pass again when the user sets `west config --global
ignore-branches True`. It was failing like this:
```
# By default, a checked-out branch should print output, even if
# the tree is otherwise clean...
check_output(['git', 'checkout', '-b', 'mybranch'], cwd=kconfiglib)
actual = cmd('compare')
> assert actual.startswith('=== Kconfiglib (subdir/Kconfiglib):')
E AssertionError: assert False
```
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
It will be convenient to manipulate configuration files in a safe
way from outside of test_config.py. To do this, make the config_tmpdir
global to all test modules by migrating it to conftest.py.
We still want to keep that fixture as an autouse in test_config.py,
so add a shim to keep that as-is.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Also rename internal parameter `search_for_local` to
`find_local`. `search_for_local` is easier to misinterpret as optional
(not finding a local file is fatal when True).
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
The default west manifest file in zephyrproject-rtos/zephyr
recently added a project with an 'import'.
That means that 'west build' will fail with the confusing
'invalid choice: "build"' error if you pull zephyr without
running 'west update'. This is a worse experience than what happens
when you run e.g. 'west list': running a built-in command that needs
the manifest to do its work will happily tell you you need to run
'west update' to resolve the failed import.
This is confusing people who pull and run 'west build', and we do have
enough information to do better from WestArgumentParser.error(), which
is the official argparse callback API invoked when
parse_known_arguments() fails in this situation.
Extract the error message formatting for this situation into a helper
function and use it from error() to print the same message when we
get ManifestImportError in these situations.
Make the error message take up fewer lines while we are here.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
A few concerns have been voiced about the west compare output:
1. its signal:noise ratio is too low (too many lines per
project without enough useful information per line)
2. the end of the status output from one project is hard to
separate from the status output for the next
3. it isn't obvious enough whether HEAD == manifest-rev or not
To try to resolve 2. without making 1. any worse, explicitly print
HEAD and manifest-rev if they differ, without adding any extra lines,
by consolidating some of the output into the relevant banner line.
To solve 3., indent the status output to the same level as the
'status:' text in the banner.
Fixes: #650
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This command prints information about manifest-rev and "git status"
output for a project if (and only if) at least one of the following is
true:
1. its checked out commit is different than the commit checked
out by the most recent successful "west update"
2. its working tree is not clean (i.e. there are local uncommitted
changes)
3. it has a local checked out branch (unless overridden via
the compare.ignore-branches config option or the --ignore-branches
command line option)
It does something similar for the manifest repository.
The purpose of this is to allow people who are developing on one or
more projects to quickly see if they have anything interesting going
on, or if their projects match the manifest-rev versions.
Fixes: #642
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
West requires python 3.8 or later now. No need to test for 3.6 or
earlier.
I've never received any reports of the ValueError happening for other
versions of Python, so it seems safe to remove the exception handling
as well. If people haven't been reporting, we'll get bug reports soon.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The rationale is to avoid requiring users to use 'version: 0.13' in
their manifest files now that west 1.0 has been released. We should
have done this in time for west 1.0.0, but that ship has sailed. We
can, and will, backport this to the release that becomes v1.0.1,
though.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
For missing files, the new output looks like this:
FATAL ERROR: manifest file not found: /home/mbolivar/zp/zephyr/nosuchfile
Please check manifest.file and manifest.path in /home/mbolivar/zp/.west/config
The old output looks like this:
FATAL ERROR: file not found: /home/mbolivar/zp/zephyr/nosuchfile
The new output is clearly more helpful.
Make a similar improvement for permission errors.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This was requested by a user in a comment in issue #144.
The purpose is to make it easy to run commands on interesting subsets
of projects in a convenient way.
See help text for details.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Marc Herbert