Make deprecated calls explicit during testing or remove them if
unnecessary.
Change the update_config and delete_config helper methods to use a
Configuration class instead of the global deprecated config functions.
Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
Fixes new `west diff --manifest` option added by commit
0d5ee4eb08 ("app: project: Allow to diff against manifest revision")
Do not pass `project.revision` to `git diff` because `project.revision`
is unresolved user input and does not always resolve to a commit. For
instance, `project.revision` can be the name of a remote branch like
`v3.7-branch` that does not exist locally; then `git diff` fails. Or
even worse: there could be a local branch of the same name which points
to a totally different commit.
This bug was found when discussing issue #747, see 7th comment there for
a longer description of what `manifest-rev` is and how it works.
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Add a testcase to parse a manifest and validate the as_dict output.
Add an invalid manifest file for missing required submodule properties.
Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
Stop using `shlex.split()` in conftest.py as a way to prepare test
commands for subprocess.run() because it does not always work. When we
have quoting issues, just avoid them entirely by passing the test
command as a list.
Replace shlex.split() with a simple str.split() for convenience in
simple cases. This simple split() will immediately fail when trying to
use quote which is working exactly as intended.
This also makes the cmd() interface more similar to subprocess.run() and
its many friends, which is good thing.
This is similar to commit 41007648d9 ("Project.git(list/str): reduce
reliance on shlex.split()") but applied to tests/
Before commit 624880e8ff, shlex.split() was used unconditionally in
conftest.py. As expected, this eventually broke on Windows: shlex is
not compatible across all operating systems and shells.
https://docs.python.org/3/library/subprocess.html#security-considerations
> If the shell is invoked explicitly, via shell=True, it is the
> application’s responsibility to ensure that all whitespace and
> metacharacters are quoted appropriately to avoid shell injection
> vulnerabilities. On _some_ platforms, it is possible to use shlex.quote()
> for this escaping.
(Emphasis mine)
Then commit 624880e8ff made the "interesting" decision to stop using
shlex.split()... only on Windows. This was discussed in
https://github.com/zephyrproject-rtos/west/pull/266#discussion_r284670076
So after this commit, parsing of test commands was delegated to the
shell but... only on Windows! This worked for a long time but eventually
broke testing white spaces for the new `west alias` #716. Rather than
making that Windows-specific hack even more complex with a special case,
finally do the right thing and ask more complex test commands to use a
list. Now we can drop shlex.
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Add a new description optional field to the schema. This field is merely
informative, it has no effect whatsoever in the manifest and/or project
processing.
Because descriptions can be multiline strings, additional code has been
added to support prettier dumping of those. PyYAML by default uses
single-line strings in YAML with '\n' characters in it, which look ugly
when dumped. To avoid this, use block-style literals (i.e. the ones
beginning with '|' in the YAML) but only for multi-line description
fields. The rest of the field handling are left untouched to preserve
backwards compatibility.
Note that if we used ruamel we would not need the additional code, since
it supports this natively.
Signed-off-by: Carles Cufi <carles.cufi@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>
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>
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>
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>
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>
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>
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>
Add a regression test for
https://github.com/zephyrproject-rtos/west/issues/545.
Mark it xfail for now since we don't have a fix. We'll remove that
once we have the fix in place.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Add test cases for this option, making sure not to use it in every
situation where we run west update with submodules to maintain
coverage for the case where it's not given.
Note that this is actually a work around for the test suite failing
using recent versions of git as well.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is a work-around for:
https://github.blog/2022-10-18-git-security-vulnerabilities-announced/#cve-2022-39253
Our test cases are failing since they are using 'git submodule add' on
a URL which is a file. Recent versions of git are now rejecting this
by default due to the above CVE. We can override this default behavior
on a per-git-command basis by overriding the configuration option for
the duration of that process. There is no risk here since the
associated repositories are not maliciously crafted.
This work-around follows a suggestion here:
https://bugs.launchpad.net/ubuntu/+source/git/+bug/1993586/comments/3
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Test that west leaves the local repo alone when HEAD~0 is used.
HEAD~0 is introduced in the docs as the canonical way of doing this.
HEAD~0 is used instead of HEAD because HEAD causes west to instead fetch
the remote HEAD.
Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>
Commit c757d6650f
("configuration: add Configuration class") was part of a general
re-work of how configuration file handling went.
As part of this change, an internal _whence() method was added, which
returns a list of configuration file paths on the file system for a
given ConfigFile enumerator. _whence() is currently erroring out with
a RuntimeError() when a local configuration file is requested, but not
found.
This breaks error handling in some cases.
For example, consider a flow like this:
# This command fails for some reason, leaving a .west
# directory but no .west/config
$ west init
# This command bombs out with RuntimeError: a topdir is
# found, so main.py tries to create a Manifest, which asks
# for the 'manifest.path' configuration option from the
# local file, which ... boom
$ west list
It would be better to raise MalformedConfig from here instead. This
lets higher layers handle this error better. In the above case,
we now get this instead:
$ west list
FATAL ERROR: can't load west manifest
local configuration file not found
This is clearly better. There are probably still some other error
handling edge cases that aren't being handled properly as a result of
this change, but I'd be curious to know how many of them this fixes.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Without a file argument, we should still use fall_back=True to search
for the workspace in the environment. This behavior was removed in the
recent rework of path handling, introducing a regression where the
manifest repository cannot be found when outside the workspace. Fix
it.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
(cherry picked from commit d275142374)
This is an API break. We are still allowed to do that until west 1.0.
The west.manifest.Manifest class's path handling has become
unmaintainable and we need to overhaul it. The from_data() and
from_file() constructors are too clever and try to allow too many use
cases, which results in the guts of the manifest construction code
often not being able to tell whether it's in a real workspace or not.
This makes it difficult to know when we can safely read configuration
files or not. That in turn makes it difficult to accurately
distinguish between the 'manifest.path' value in the local configuration
file and the 'self: path:' value in the manifest data, especially when
we can take keyword arguments that say "hey, pretend this is your
path".
Additionally, there are a few assumptions in the code that pretend
that the manifest file itself always lives in the top level directory
of the manifest repository. That was a valid assumption up to the
point the 'manifest.file' configuration variable was introduced, but
it's no longer valid anymore, since 'manifest.file' can point to a
subdirectory of the manifest repository. This leaves us with some
hacks to find the git repository we live in that shouldn't be
necessary and which we can now remove with this overhaul.
Rework the whole thing so that we can correctly handle the following
situations:
- when running in a workspace, you should now use from_file() or
a newly introduced from_topdir() to make your Manifest
- when running outside of any workspace, use from_data()
From now on, we forbid creating a manifest from data "as if" it were
in a real workspace. If all you have is data, the abspath attributes
will all be None, reflecting reality.
Accordingly, rework the Manifest.__init__() method to either take a
topdir or a bit of data, but not both. Additionally, now that we have
both manifest.path and manifest.file configuration options, it's not
usually the right thing to ask for a Manifest from a *file*: what you
really usually want is a Manifest from a *workspace*, and for that you
just need a topdir. From the topdir, you can create a
west.configuration.Configuration, and from there you can read
manifest.path and manifest.file to get the complete path to the top
level manifest within its repository. For that reason, we remove the
source_file keyword argument from __init__() in favor of topdir and a
new 'config' argument.
For backwards compatibility, it's still allowed to use from_file() to
load another manifest file than the one pointed to by
topdir/manifest.path/manifest.file. However, this handling code is now
just a bit of special case trickery in from_file(), and it also
introduces a restriction that the other file is still in a git
repository in the workspace. This moves potential sources for error or
confusion out of Manifest.__init__() and the functions it calls. (This
has proven to be a useful feature in practice; I am aware of west
extensions that use it to compare the parsed contents of two different
manifest files within the same workspace.)
To migrate away from the now-outdated from_file(), introduce a new
from_topdir() factory method and use it throughout the tree. This is
clearer and more accurate code, which always avoids the hacks left
behind for compatibility in from_file().
Finally, add various new instance attributes to the Manifest class
that will let us tell the YAML path from the actual path, and the path
to the manifest file from the path to the manifest repository in the
workspace. These in turn let us remove some hacky code from the 'west
list' implementation, and allow us to rework our internals so that
ManifestProject is just a legacy remnant at this point, further
helping along with #327.
Keep tests up to date, removing obsolete code as needed and updating
cases to reflect API breakage.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Commit 3f81eb4195
("manifest: project names must be unique") fixed an issue where
projects with duplicate names were previously permitted, but the test
case file that was meant to catch regressions was incorrectly left
empty. Fix it.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
It will be convenient in a future patch for west.manifest.validate()
to always return the validated dict which is loaded from YAML if all
we have is a str, so extend the behavior of the function by returning
the input dictionary it receives, or returning the dictionary it
parses.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Tweak some incidental details and add a couple of extra asserts.
Prep work for adding another test later on.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The argparse module changed some implementation details in in a way
that's causing a false failure in testing. Handle it.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This isn't working properly because the ManifestProject's revision is
"HEAD", so it looks like an ordinary project and we try to get
manifest-rev from it. That is obviously incorrect. Fix it by detecting
the ManifestProject explicitly and doing the right thing there.
Fixes: #550
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Pieter De Gendt