Open 57300 opened 1 year ago
@mbolivar-nordic Hi, I can't add you as reviewer, but do you want to look over this?
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:
https://github.com/zephyrproject-rtos/west/blob/9e8f5002f2ee86b7150c039cb69aa653ff454162/src/west/app/main.py#L277-L283
since it will always bail without a manifest: https://github.com/zephyrproject-rtos/west/blob/9e8f5002f2ee86b7150c039cb69aa653ff454162/src/west/app/project.py#L547-L548
Minor quirk that stuck out to me while testing the recent changes. Before https://github.com/zephyrproject-rtos/west/commit/23db6e1f3cc7fb50e0a3e0bda8235892db74c971, if I were to try
west manifest --resolve
in Zephyr without cloningbsim
, I would see this:which is helpful enough. Now, this generic message gets displayed:
Ironically,
west manifest --validate
produces the same error, which is not very helpful. So, I tried to fix that.