manifest: error if self.path doesn't match directory name #268
Conversation
As an example, the "Clone the Zephyr Repositories" section of Zephyr's
Getting Started Guide provides the following command to wrap a west
installation around an already cloned, standalone zephyr repository:
west init -l zephyr/
It feels natural to use the same command with some alternative
zephyr-blue/ or zephyr-red/ directory name, however that's not
compatible with the zephyr-blue/west.wml manifest which has its
self.path hardcoded to "zephyr". Hardcoding self.path is not a issue,
the problem is that the inconsistency is not caught and west pretends
the init command was successful. It's then much more difficult to
understand later why the build fails to find some file(s).
Implement a sanity check which make west init "fail fast" like this:
FATAL ERROR: Manifest self.path is 'zephyr' but located in \
directory '/home/marc/work/zephyr-blue'
Fixes zephyrproject-rtos#267
Signed-off-by: Marc Herbert <marc.herbert@intel.com>
| realpath = os.path.dirname(self.path) | ||
| realdir = os.path.basename(realpath) | ||
| if realdir not in [os.path.basename(path), 'manifest-tmp']: | ||
| raise MalformedManifest("""Manifest self.path is '{}' \ |
There was a problem hiding this comment.
Are you sure you don't want to implement this in the init command instead? I think it's legal to let the manifest parser be contained in a directory that has a "wrong" name; the problem comes when used with init -l, no?
There was a problem hiding this comment.
The only problem I found is in the init -l command, however:
- Would anyone ever want an incorrect "self.path"?
- How safe would that be? For instance letting west clone in self.path and manually renaming it later seems somewhat reckless.
self.path is optional. If some project makes the effort to configure it then there are some project-specific reason(s) to do so (e.g.: zephyr) and it should be obeyed. Otherwise they can just not use it.
There was a problem hiding this comment.
Thinking about this more, I am convinced this problem is specific to init -l.
The west.manifest module is intended to be used as a general purpose library for parsing and managing manifests, so putting it here is really telling the user what they ought to be doing with their data, rather than letting them choose.
Would anyone ever want an incorrect "self.path"?
They may be parsing a bunch of manifest files they have dumped in a temporary directory, and only be interested in the project list, for example. In this case, they do not care about self.path at all. Having this exception here prevents them from getting their job done in that case.
Or they might have a bunch of manifests which they are parsing and using to generate some other directory in the right place, by using self.path, even though the directory containing the manifest might not happen to be a valid manifest repository. That's fine, because they're just operating on manifests, not manifest repositories.
Also notice that you can create an instance using Manifest.from_data(), so there is no directory containing the manifest in general (what if we got the manifest data over the network?).
The MalformedManifest exception is for when the contents of the manifest file itself are malformed. It isn't about a context-specific issue in the environment in which the manifest is used.
How safe would that be? For instance letting west clone in self.path and manually renaming it later seems somewhat reckless.
In my opinion, error handling in that case should also be handled by the commands which operate on an installation, and not the routine whose only job it is to load manifest data.
There was a problem hiding this comment.
And yes, I'm aware that there is some code in here which violates the abstraction, e.g. by calling west_dir().
Things are actually cleaner than they used to be. We had arguments early on in the west design if we should be trying to make user-visible APIs or not. "Yes" won out, but code written by the "no" side had already been merged and has slowly been pulled apart into the current west.manifest etc. versus west.commands.xxx.
We have an issue open to finish the cleanup (#191); let's not make things worse :).
There was a problem hiding this comment.
Thanks for all the details, I'll try to find time to change this.
There was a problem hiding this comment.
Thanks again for your first west pull request! I'm always glad when we get submissions from other Zephyr developers; I see it as a sign of health.
If you don't find the time, let me know and I'll make the changes.
|
Thanks for the patch! I think this fix might belong in the init implementation in project.py, though. |
@mbolivar I won't have time to resume work on this particular one, not in the next couple months anyway. cc @Blacksilver42 |
OK, I'm closing out stale PRs for now, so please feel free to reopen if you have time again. Thanks for the work so far! |
As an example, the "Clone the Zephyr Repositories" section of Zephyr's
Getting Started Guide provides the following command to wrap a west
installation around an already cloned, standalone zephyr repository:
west init -l zephyr/
It feels natural to use the same command with some alternative
zephyr-blue/ or zephyr-red/ directory name, however that's not
compatible with the zephyr-blue/west.wml manifest which has its
self.path hardcoded to "zephyr". Hardcoding self.path is not a issue,
the problem is that the inconsistency is not caught and west pretends
the init command was successful. It's then much more difficult to
understand later why the build fails to find some file(s).
Implement a sanity check which make west init "fail fast" like this:
FATAL ERROR: Manifest self.path is 'zephyr' but located in
directory '/home/marc/work/zephyr-blue'
Fixes #267
Signed-off-by: Marc Herbert marc.herbert@intel.com