Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

commands: Show less generic error when no manifest is loaded #671

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

57300
Copy link

@57300 57300 commented Jun 2, 2023

Minor quirk that stuck out to me while testing the recent changes. Before 23db6e1, if I were to try west manifest --resolve in Zephyr without cloning bsim, I would see this:

FATAL ERROR: project bsim (tools/bsim): cannot import contents of west.yml; do you need to run "west update"?

which is helpful enough. Now, this generic message gets displayed:

FATAL ERROR: can't run west manifest; it requires the manifest, which was not available. Try "west manifest --validate" to debug.

Ironically, west manifest --validate produces the same error, which is not very helpful. So, I tried to fix that.

If loading the manifest fails, running a command which needs to read the
manifest could result in this being shown:

   FATAL ERROR: can't run west manifest; it requires the manifest, which
   was not available. Try "west manifest --validate" to debug.

In this case, trying "west manifest --validate" would also display the
same error, which is not very helpful.

Instead of that, west can print the deferred error from load_manifest(),
which contains a more specific message. Rework the WestCommand.manifest
property handling by adding a dummy _ManifestRequired exception. It is
caught by run_builtin() in main.py, which can access the stored error.

Signed-off-by: Grzegorz Swiderski <grzegorz.swiderski@nordicsemi.no>
@57300
Copy link
Author

57300 commented Jun 2, 2023

@mbolivar-nordic Hi, I can't add you as reviewer, but do you want to look over this?

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should probably be handled in the relevant built-in commands themselves by checking WestCommand.has_manifest and erroring out as necessary. Did you consider this? If so, why did you reject it?

@57300
Copy link
Author

57300 commented Jun 2, 2023

I think this should probably be handled in the relevant built-in commands themselves by checking WestCommand.has_manifest and erroring out as necessary. Did you consider this? If so, why did you reject it?

Yes, I had briefly considered it, but I figured it would probably involve passing WestApp.mle to each command, so that it could still report why not self.has_manifest. I decided against it, thinking it might be better to have common handling in one place. There's already some common handling in handle_builtin_manifest_load_err(), which seems to cover quite a few cases.

I'm fine with per-command handling, if you prefer, since it looks like only west manifest needs it right now anyway.

In fact, on second thought, the easiest fix for west manifest specifically might be to just remove it from this list:

west/src/west/app/main.py

Lines 277 to 283 in 9e8f500

# A few commands are always safe to run without a manifest.
# The update command is sometimes safe and sometimes not, but
# we need to include it in this list because it's the only way
# to fix a manifest-rev revision in a project which is being
# imported to point from a bogus manifest to a non-bogus one.
no_manifest_ok = ['help', 'config', 'topdir', 'init', 'manifest',
'update']

since it will always bail without a manifest:

def do_run(self, args, user_args):
manifest = self.manifest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants