-
Notifications
You must be signed in to change notification settings - Fork 126
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
app: more versatile initialization for path in config #775
base: main
Are you sure you want to change the base?
Conversation
bebc716
to
dbb2fdc
Compare
IMO to go even further, I think
but
the command would become
this is a breaking change though, thoughts? |
dbb2fdc
to
8631ad2
Compare
Verify that |
in case a manifest file is contained into some sub directories, the command to init a workspace locally should be ``` west init -l sub1/sub2 --mf west.yml ``` but in that case, the west top dir is going to be into `sub1` which is an issue. one workaround is to use: ``` west init -l sub1 --mf sub2/west.yml ``` but in that case, the config is initialized incorrectly: `manifest.path` contains only `sub1` and `manifest.file` is `sub2/west.yml`. `manifest.path` should contain a path, even if `--mf` is a relative path. `manifest.file` should only contain the filename. this change allows any usage so that the workspace top dir is created next to `sub1` and the manifest repo can be located into nested directories Signed-off-by: Cyril Fougeray <cyril.fougeray@toolsforhumanity.com>
8631ad2
to
92629a8
Compare
@pdgendt tox fails with errors even on |
@@ -287,7 +287,7 @@ def local(self, args) -> Path: | |||
os.chdir(topdir) | |||
self.config = Configuration(topdir=topdir) | |||
self.config.set('manifest.path', os.fspath(rel_manifest)) | |||
self.config.set('manifest.file', manifest_filename) | |||
self.config.set('manifest.file', manifest_file.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't allow a nested manifest file, I think? It should be relative to the manifest path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this issue and submitting a PR. I haven't time to look at it in detail yet but I already know we definitely need additional test coverage for this. Either adding some new test(s) or tweaking existing tests or a combination of both.
Again, thanks for the excellent report.
Interestingly, even your very first I'm afraid we need to clearly and formally define the user interface before even looking at the code. EDIT: see rsync-based suggestion in #774 (comment)
Please file a separate bug for this. Don't forget the OS and OS version. |
in case a manifest file is contained into some sub directories, the command to init a workspace locally should be
but in that case, the west top dir is going to be into
sub1
which is an issue.one workaround is to use:
but in that case, the config is initialized incorrectly:
manifest.path
contains onlysub1
andmanifest.file
issub2/west.yml
.manifest.path
should contain a path, even if--mf
is a relative path.manifest.file
should only contain the filename.this change allows any usage so that the workspace top dir is created next to
sub1
and the manifest repo can be located into nested directoriesfixes #774