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

manifest: error if self.path doesn't match directory name #268

Closed
wants to merge 1 commit into from

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented May 24, 2019

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

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 '{}' \
Copy link
Contributor

@mbolivar mbolivar May 24, 2019

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?

Copy link
Collaborator Author

@marc-hb marc-hb May 24, 2019

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.

Copy link
Contributor

@mbolivar mbolivar May 24, 2019

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.

Copy link
Contributor

@mbolivar mbolivar May 24, 2019

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 :).

Copy link
Collaborator Author

@marc-hb marc-hb May 24, 2019

Thanks for all the details, I'll try to find time to change this.

Copy link
Contributor

@mbolivar mbolivar May 25, 2019

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.

@mbolivar
Copy link
Contributor

@mbolivar mbolivar commented May 24, 2019

Thanks for the patch! I think this fix might belong in the init implementation in project.py, though.

@marc-hb
Copy link
Collaborator Author

@marc-hb marc-hb commented Jul 11, 2019

If you don't find the time, let me know and I'll make the changes.

@mbolivar I won't have time to resume work on this particular one, not in the next couple months anyway.

cc @Blacksilver42

@mbolivar-nordic
Copy link
Member

@mbolivar-nordic mbolivar-nordic commented Aug 13, 2020

@mbolivar I won't have time to resume work on this particular one, not in the next couple months anyway.

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!

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

Successfully merging this pull request may close these issues.

3 participants