main: handle ManifestImportError from extensions better #654
Conversation
|
cc @nordicjm |
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 <[email protected]>
mbolivar-nordic
force-pushed
the
handle-manifest-import-error-better
branch
from
7242b69 to
3bd2bd9
Compare
|
Tacked on a commit to improve the lives of API users. |
|
I just tested (not reviewed) this and it does fix the |
@marc-hb any chance of a rubber-stamp? I can't merge this without review :( |
There was a problem hiding this comment.
The force is strong with this one. But the "bus factor" is very low.
I hate to ask for these few cosmetic changes because the clarity of the code and comments in west is generally very high and this PR is no exception. However the complexity of this part of the code is... even higher? and I suspect the number of people familiar with it is very close to 1.
Non-blocking, if you're in the rush to get this merged then LGTM.
| # do_run(). | ||
| cmd = app.builtins['help'] | ||
| cmd.config = west.configuration.Configuration( | ||
| topdir=app.topdir) |
There was a problem hiding this comment.
I don't understand why this .config "patch-up" is needed sorry.
There was a problem hiding this comment.
Because we aren't inside do_run(), and thus configuration isn't accessible in the usual way. This is part of the parameter contract for WestCommand.config:
'''Property for the config which was passed to run().
If `do_run` was given a *config* kwarg, it is returned.
Otherwise, a fatal error occurs.
'''
We're not running a command, so we're not in do_run(). So we have to patch this up manually.
There was a problem hiding this comment.
OK, it looks like I don't really know the meaning of the English verb "to patch up".
I thought "to patch up" meant modifying an existing .config but you're saying this code is making a missing one up? That makes more sense to me now.
So I would have written "make up a missing .config attribute" but you're the native speaker and I'm not so never mind!
| app = self.west_app | ||
| mle = self.west_app.mle | ||
| if app.cmd: | ||
| cmd = app.cmd |
There was a problem hiding this comment.
Maybe failed_cmd = app.cmd? I've been experiencing a fair amount of "shorthand fatigue" in this review...
There was a problem hiding this comment.
Maybe failed_cmd = app.cmd?
That's not accurate, though: we are just looking for a WestCommand we can call .die() on. That might be a command which has failed, but it also might be the case that we have no knowledge of what command to run, and are just grabbing some other WestCommand instance, somewhat at random, to use instead. This method is definitely a white-box sort of thing, relying on implementation details elsewhere.
Try to provide more helpful text. Signed-off-by: Martí Bolívar <[email protected]>
mbolivar-nordic
force-pushed
the
handle-manifest-import-error-better
branch
from
1383a63 to
7395d2b
Compare
|
@marc-hb I addressed your first two comments and tried to provide explanations for the second two. |
| # do_run(). | ||
| cmd = app.builtins['help'] | ||
| cmd.config = west.configuration.Configuration( | ||
| topdir=app.topdir) |
There was a problem hiding this comment.
OK, it looks like I don't really know the meaning of the English verb "to patch up".
I thought "to patch up" meant modifying an existing .config but you're saying this code is making a missing one up? That makes more sense to me now.
So I would have written "make up a missing .config attribute" but you're the native speaker and I'm not so never mind!
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.