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

app: more versatile initialization for path in config #775

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

Conversation

fouge
Copy link

@fouge fouge commented Dec 18, 2024

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

fixes #774

@fouge
Copy link
Author

fouge commented Dec 18, 2024

IMO to go even further, I think directory should point to the location of the west top dir
not:

.west is created next to "directory"

but

.west is created in "directory"

the command would become

west init -l . --mf sub1/sub2/west.yml

this is a breaking change though, thoughts?

@fouge fouge force-pushed the fouge/west-manifest-path branch from dbb2fdc to 8631ad2 Compare December 18, 2024 14:19
@pdgendt pdgendt requested review from pdgendt and marc-hb December 18, 2024 14:21
@pdgendt
Copy link
Collaborator

pdgendt commented Dec 18, 2024

Verify that tox runs on your local machine before pushing :)

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>
@fouge fouge force-pushed the fouge/west-manifest-path branch from 8631ad2 to 92629a8 Compare December 18, 2024 14:52
@fouge
Copy link
Author

fouge commented Dec 18, 2024

@pdgendt tox fails with errors even on main on my machine 😞

@@ -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)
Copy link
Collaborator

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.

Copy link
Collaborator

@marc-hb marc-hb left a 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.

@marc-hb
Copy link
Collaborator

marc-hb commented Dec 18, 2024

Again, thanks for the excellent report.

this is a breaking change though, thoughts?

Interestingly, even your very first west init -l development/west_project1/ --mf west.yml suggestion at the top would already be a backwards incompatible change.

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)

@pdgendt tox fails with errors even on main on my machine 😞

Please file a separate bug for this. Don't forget the OS and OS version.

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.

west init gives wrong config path for a manifest file in nested directories
3 participants