Multimanifest prep #266

mbolivar · 2019-05-15T16:52:31Z

Prep work commits for the multimanifest work.

The main changes needed to enable multimanifest are:

manifest: support explicit project URLs
manifest: project names must be unique

The rest are just cleanup and minor things.

There's no good reason to have these tests in subdirectories. Flatten them out. Keep the directory of invalid manifests separate to keep the directory listing clean, though. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

It's not clear what this is doing here. It doesn't begin with "test", so it's not being tested, and it seems to be a copy of test_config.py with some important features needed to avoid modifying the user's configuration. Delete it. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

Don't copy the west tree; tox already installs it for us into the new virtualenv, and we don't run any code out of a checked out repository anymore, so doing things related to that is unnecessary. This also makes the tests run a little bit faster (around a 5% or more speedup on my system). Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

It takes 20 seconds on my machine to run the full set of tests, which is slow enough that testing breaks me out of flow state. On the suspicion that creating git repositories and using the file:// protocol when cloning (which prevents use of hardlinks) is slowing things down, use some pytest features to avoid creating git repositories repeatedly. Also let git use hardlinks when they are available when cloning repositories. On my system, this brings the average of 10 runs from 20.129 seconds spent testing to 17.649, a 12% improvement overall. Still not ideal, but not worth throwing away, either. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

I'm not sure this ever worked; the args field is an empty tuple even in Python 3.4. Use cmd and returncode attributes appropriately instead. Don't offer the 'for a stack trace' message here anymore: this doesn't indicate a west error, which is what that is meant to capture. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

Add a helper for saving the current traceback to a temporary file and use it appropriately from the exception handlers in main(), This avoid throwing away information in case the error was nondeterministic. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

We're going to add explicit project URLs to the manifest, so the remote should no longer be a positional. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

Allow each project element to explicitly specify its URL. This avoids forcing users to name projects according to their URLs, which can be inconvenient (and prevents us from enforcing a rule that project names are unique). Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

We don't apply the west default revision correctly if defaults is None. Since we find it convenient to let west figure out the defaults for us, demote it to a kwarg as well and let it be None more gracefully. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

mbolivar · 2019-05-15T16:53:35Z

@tejlmand regarding tests: remove empty_config.py; any insight into why it was added? Just the result of git commit -a or so?

Make sure to update comments for existing tests with modified semantics. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

It is always possible to satisfy this constraint now that project URLs may be specified explicitly. (This restriction will be necessary to make manifest imports work sensibly -- we should have done this before...). Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

We need a common definition for canonicalizing paths; the lack of one is causing issues on Windows. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

'west config' testing is giving false negatives on Windows. To fix: - canonicalize and fix up global config path testing - avoid passing paths to cmd(); they don't play well with shlex.split() and aren't necessary to make sure the config command is behaving properly Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

This fixes test_project.py::test_list on Windows, as otherwise quoting and splitting the relevant paths using shlex with POSIX rules behaves incorrectly. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

This file is giving false negatives on Windows. To fix: - test_manifest_freeze: expect (and escape) Windows paths - test_forall: echo may be available in cmd.exe, but single-quoted strings sure aren't - test_extension_command_*: handle newlines portably Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

mbolivar · 2019-05-15T19:01:07Z

Added some additional fixes to get all our tests passing on Windows 10 as well.

carlescufi · 2019-05-16T11:51:44Z

src/west/main.py

@@ -557,12 +557,10 @@ def main(argv=None):
    except KeyboardInterrupt:
        sys.exit(0)
    except CalledProcessError as cpe:
-        log.err('command exited with status {}: {}'.format(
-            cpe.args[0], quote_sh_list(cpe.args[1])))


I can guarantee this worked, I tested it explicitly. That said, this is the right way to do this.

I'm confused. This change is what we discussed in previous PRs, so that users who initialize CalledProcessError with kwargs instead of positionals don't get faced with an IndexError when we try to access cpe.args.

IOW: I know you're not asking for a change here, but you specifically asked for this change earlier :). Even though it "works" if you properly initialize CalledProcessError as the python subprocess module does, users who make their own instances won't always, as we discovered.

The change is required and I am completely happy with it. I am just saying that this old code actually worked, to my amazement :)

carlescufi · 2019-05-16T11:53:02Z

src/west/main.py

            traceback.print_exc()
        else:
-            log.inf(for_stack_trace)
+            log.err(msg, 'See {} for a traceback.'.format(dump_traceback()))


Nice, I like this.

carlescufi · 2019-05-16T11:56:40Z

src/west/manifest.py

@@ -281,6 +282,10 @@ def _load(self, data):
            except ValueError as ve:
                self._malformed(ve.args[0])

+            # Project names must be unique.
+            if project.name in project_names:


Doesn't this fix a GitHub issue? I seem to recall there was one

This is essential to the design of the multimanifest feature. Projects are identified by their names in that work; it's how we decide what projects to apply overrides of attributes like url, name, etc. to. This means it doesn't make sense to continue allowing two projects with the same "name" in a single manifest. Arguably it never did, but it was necessary before because some projects might have had the same final path component in a URL but different URL bases. That case can be handled now with an explicit url: for at least one of the two.

Understood, thanks for the explanation

carlescufi · 2019-05-16T11:57:25Z

tests/conftest.py

-    cmdlst = shlex.split('west ' + cmd)
-    print('running:', cmdlst)
+    cmd = 'west ' + cmd
+    if platform.system() != 'Windows':


We typically use sys.platform, maybe use that for consistency

Why is this needed btw? We don't usually make this difference in normal invokations of check_output et al.

We typically use sys.platform, maybe use that for consistency

Really? We use platform.system() in configuration.py ,which is what this is testing. I can make that change, though.

Why is this needed btw? We don't usually make this difference in normal invokations of check_output et al.

It's breaking the tests because shlex splits by POSIX rules, which isn't right for windows shells and led to bad results when applied to paths. Example:

In [2]: shlex.split('west foo bar\\baz') Out[2]: ['west', 'foo', 'barbaz']

Right, I see. I don't have a strong preference either way, just wanted to keep it consistent. I didn't know we used platform.system() in configuration.py, but I've always seen sys.platform elsewhere, like the tox.ini or the requirements.txt.

5 years later, dropping shlex.split() in #730

carlescufi

Minus some minor nits, I am happy with this. Thanks!

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 4100764 ("Project.git(list/str): reduce reliance on shlex.split()") but applied to tests/ Before commit 624880e, 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 624880e made the "interesting" decision to stop using shlex.split()... only on Windows. This was discussed in zephyrproject-rtos#266 (comment) 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` zephyrproject-rtos#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>

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 4100764 ("Project.git(list/str): reduce reliance on shlex.split()") but applied to tests/ Before commit 624880e, 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 624880e made the "interesting" decision to stop using shlex.split()... only on Windows. This was discussed in #266 (comment) 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>

mbolivar-nordic added 9 commits May 14, 2019 09:15

tests: flatten the hierarchy

41fbf25

There's no good reason to have these tests in subdirectories. Flatten them out. Keep the directory of invalid manifests separate to keep the directory listing clean, though. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

manifest: demote Project remote to a kwarg

f98b719

We're going to add explicit project URLs to the manifest, so the remote should no longer be a positional. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

mbolivar requested review from tejlmand and carlescufi May 15, 2019 16:52

mbolivar force-pushed the multimanifest-prep branch from cf7e7f2 to 69981e5 Compare May 15, 2019 17:09

mbolivar-nordic added 4 commits May 15, 2019 12:51

tests: add test cases for explicit project URLs

a774380

Make sure to update comments for existing tests with modified semantics. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

util: add canon_path()

ebae08f

We need a common definition for canonicalizing paths; the lack of one is causing issues on Windows. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

mbolivar force-pushed the multimanifest-prep branch from 69981e5 to a6733c7 Compare May 15, 2019 18:51

mbolivar-nordic added 2 commits May 15, 2019 12:58

tests: conftest: use command strings exactly on Windows

781ac46

This fixes test_project.py::test_list on Windows, as otherwise quoting and splitting the relevant paths using shlex with POSIX rules behaves incorrectly. Signed-off-by: Marti Bolivar <marti.bolivar@nordicsemi.no>

mbolivar force-pushed the multimanifest-prep branch from a6733c7 to 2df535a Compare May 15, 2019 18:58

This was referenced May 15, 2019

Command to add external module dependency #244

Closed

West throws away stack traces on some errors #223

Closed

carlescufi reviewed May 16, 2019

View reviewed changes

carlescufi approved these changes May 16, 2019

View reviewed changes

carlescufi merged commit c724ee6 into zephyrproject-rtos:master May 20, 2019

marc-hb mentioned this pull request Aug 28, 2024

app: support alias commands #716

Merged

marc-hb mentioned this pull request Aug 28, 2024

tests: stop using shlex.split() and use lists instead #730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multimanifest prep #266

Multimanifest prep #266

mbolivar commented May 15, 2019

mbolivar commented May 15, 2019

mbolivar commented May 15, 2019

carlescufi May 16, 2019

mbolivar May 16, 2019

mbolivar May 16, 2019

carlescufi May 20, 2019

carlescufi May 16, 2019

carlescufi May 16, 2019

mbolivar May 16, 2019 •

edited

Loading

carlescufi May 20, 2019

carlescufi May 16, 2019

carlescufi May 16, 2019

mbolivar May 16, 2019

carlescufi May 20, 2019

marc-hb Aug 28, 2024

carlescufi left a comment

Multimanifest prep #266

Multimanifest prep #266

Conversation

mbolivar commented May 15, 2019

mbolivar commented May 15, 2019

mbolivar commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi left a comment

Choose a reason for hiding this comment

mbolivar May 16, 2019 •

edited

Loading