-
Notifications
You must be signed in to change notification settings - Fork 150
support 'west init --topdir' #854
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
base: main
Are you sure you want to change the base?
support 'west init --topdir' #854
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #854 +/- ##
==========================================
+ Coverage 85.95% 86.03% +0.08%
==========================================
Files 11 11
Lines 3453 3474 +21
==========================================
+ Hits 2968 2989 +21
Misses 485 485
|
mbolivar
left a comment
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.
Seems reasonable to me
pdgendt
left a comment
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.
Looks good!
652408c to
8a2e15e
Compare
|
I really want to review this one because I already spent a lot of time on #774 and #775 before but @thorsten-klein is just submitting way too much code, I can't keep up!
Can you give me a few more days? |
Sorry 🤞
Yes, sure 👍🏻 Thank you in advance for your time and input! |
a08dec0 to
99377aa
Compare
Seconded, and I'll take this opportunity to thank you for all your contributions @thorsten-klein. |
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.
Even though I'd prefer --topdir was only compatible with -l, this is not a blocking issue for me. Thanks!
|
Let's wait for feedback from @marc-hb, as he requested some more time. |
marc-hb
left a comment
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 is particularly useful when using --local
I downloaded this PR and tested the other, --manifest-url option and it does not seem to work for me. I ran out of time and did not try --local yet. I didn't look at the code either.
Describing directory nesting clearly and accurately is not easy - it is basically a "specification" except it has to be user-friendly too. So I will look at the code only when that "specification" is complete and unambiguous.
This help looks like the two last ones can be both used at the same time:
west init [-m URL] [--mr REVISION] [--mf FILE] [-o=GIT_CLONE_OPTION] [-t WORKSPACE_DIR] [directory]
But it's not actually possible:
FATAL ERROR: --topdir cannot be combined with positional argument [directory]
When using either separately, each one seems to have the same effect. So, there does not seem to be any new feature in the remote case, which means it's still not possible to west init a manifest two levels down or more in the remote case? Is this an oversight? The help does not seem to say. I don't see why --local would be more flexible than --manifest-url. I would hope the --manifest-url and --local options work the same immediately after the git clone operation.
|
|
Thank you for the review 🙏
Yes, exactly. Nevertheless, in remote case those arguments that do the same are mutually exclusive (see error message you got).
This has already worked before in remote case, since the positional argument Thank you 👍🏻 |
99377aa to
552f964
Compare
|
I think I have resolved all review comments in the code👍🏻 |
Has it? Please share one example of a |
dd0e9e3 to
8e07584
Compare
8e07584 to
0c4ecf8
Compare
|
@marc-hb could you please follow-up on this PR? |
|
Sorry I was on vacation and then it fell through the cracks. Will take another look ASAP |
marc-hb
left a comment
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.
Partial review, I didn't get a chance to look at the test code yet, sorry. Later this week.
I’ve tried with the code to fail if the topdir is already initialized (when using --local). However, it seems that some tests (e.g. test_update_some_with_imports) rely on the current behavior and perform a re-initialization of some already initialized workspace.
What shall I do? It looks like this behavior might have been intentional, so I make it fail only when the new argument --topdir is used.
I'm amazed that such "reinit" situations exist, is it still the case? If yes, can you elaborate and provide a simple reproduction? This looks like a serious bug. Does it predate this PR? Does it simply deletes the existing .west/ directory? I can't believe there is code doing that.
You requested that west is able to create such a layout.
I only requested feature parity between --local and not --local. This is not "my" use case, this is the same use case than --local but when cloning.
Is this correct behavior or what would be your expectation when both args.topdir and directory are provided?
If they are mutually exclusive (as requested above), your requested feature will not be possible.
I don't remember requesting these they should be mutually exclusive, did I? I only remember experiencing that they were mutually exclusive in a previous version while the --help presented them as not mutually exclusive. So I requested to correct the help: #854 (comment)
Obviously, that's not relevant any more.
I find the current user interface confusing in the cloning case but:
- it seems consistent with --local
- it seems backwards-compatible
- I can't think of a better one at this point.
src/west/app/project.py
Outdated
| `.west/config`. | ||
| The West manifest can come from either a remote repository (that will be cloned | ||
| during workspace initialization) or from an already existing local directory. | ||
| Config option `manifest.path` is set accordingly (see below). |
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 is too early for that implementation detail, you can drop that line here.
To be honest I'm not sure this name and implementation detail should be mentioned at all in the help text. But for sure not that early.
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.
To me it was a helpful detail since I can directly see as a user what's the impact of this command.
Especially it helped to point out the difference between local and remote.
But I will remove 👍
src/west/app/project.py
Outdated
| parser = self._parser( | ||
| parser_adder, | ||
| usage=''' | ||
| init around a remote repository: |
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.
| init around a remote repository: | |
| init after cloning a manifest repository: |
"around" something that does not exist yet sounds weird. "remote" sounds like it has to go over the network. I don't think it has.
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.
I think it was some naming I have read anywhere and took over.
I would prefer having some shrt preposition.
Maybe one of those?
init workspace FROM an existing local manifestinit workspace WITH an existing local manifestinit workspace FOR an existing local manifest
I prefer WITH.
src/west/app/project.py
Outdated
| '--manifest-url', | ||
| help='''manifest repository URL to clone; | ||
| metavar='URL', | ||
| help='''remote manifest repository URL to clone; |
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.
"remote" sounds like this must go over the network. I don't think it has to. I don't think this line needs to change. It already says "clone" so it's not ambiguous.
src/west/app/project.py
Outdated
| in this case, and manifest.path points at | ||
| "directory"''', | ||
| help='''initialize workspace around an already existing local | ||
| manifest instead of cloning a remote manifest; |
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.
| manifest instead of cloning a remote manifest; | |
| manifest instead of cloning a manifest; |
src/west/app/project.py
Outdated
| metavar='FILE', | ||
| help=f'''manifest file to use. It is the relative | ||
| path of the manifest file within the remote repository | ||
| respectively the local manifest_directory. |
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.
I don't think both cases need to be listed, it's the same thing in both cases. This just the "relative path TO the manifest file within the manifest directory" and that's IMHO enough. It does not matter whether that manifest directory is remote, local, or being cloned from a local disk. All that dilutes the information.
| If no `-m / --manifest-url` is provided, west uses Zephyr URL by default: | ||
| {MANIFEST_URL_DEFAULT}. | ||
|
|
||
| The topdir (where `.west` is created) is determined as follows: |
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.
(It really sucks we have to keep such a complicated logic for backwards compatibility)
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.
To simplify the logic, we can just deprecate using a positional argument instead of the new --topdir. This will make the local and cloning case more consistent with each other.
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.
I would propose to add the --topdir feature here, and do this deprecation in a separate PR.
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.
Agreed for a separate PR, glad you're ok with the deprecation idea. Can you please add a comment in the code now? "# This case will be deprecated later". And in the corresponding test code if any.
src/west/app/project.py
Outdated
| 2. Local Manifest | ||
| ----------------- | ||
| If `-l / --local` is given, west initializes a workspace around an already | ||
| existing `west.yml`. Its location can be specified in `manifest_directory` |
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.
| existing `west.yml`. Its location can be specified in `manifest_directory` | |
| existing `west.yml`. Its location can be specified in the `manifest_directory` positional argument |
src/west/app/project.py
Outdated
| ----------------- | ||
| If `-l / --local` is given, west initializes a workspace around an already | ||
| existing `west.yml`. Its location can be specified in `manifest_directory` | ||
| (defaults to current working directory). Config option `manifest.path` is set |
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.
| (defaults to current working directory). Config option `manifest.path` is set | |
| It defaults to current working directory. |
No parenthesis.
Is mentioning manifest.path really required at all in this help text? I think the west documentation should be enough.
src/west/app/project.py
Outdated
| # is PurePosixPath. | ||
| manifest_path = PurePath(urlparse(manifest_url).path).name | ||
|
|
||
| manifest_path = str(Path(subdir) / 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.
Why does need to be converted to a string? The comment just above warns about this needing to be a PurePath in some cases...
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.
For setting the option the value have to be str: TypeError: option values must be strings
I have moved to conversion to the function call now 👍
src/west/app/project.py
Outdated
| if args.topdir: | ||
| topdir = Path(args.topdir).resolve() | ||
| try: | ||
| already = util.west_topdir(topdir, fall_back=False) |
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.
| already = util.west_topdir(topdir, fall_back=False) | |
| already = util.west_topdir(manifest_dir, fall_back=False) |
We want to maximize the chances of finding an existing workspace, not minimize them.
Also: move the manifest_file.is_relative_to(topdir): check first.
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.
I think this is not correct.
E.g. --topdir workspace --local /path/to/my/manifest should only fail if topdir is already a workspace, not if any parent of the manifest is already a workspace.
Ok.
0c4ecf8 to
c3674bf
Compare
c3674bf to
5d88b67
Compare
marc-hb
left a comment
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.
manifest_path: Path = Path(subdir) / mf_path
This line does not make sense to me, see below.
src/west/app/project.py
Outdated
| --mf | ||
| The relative path to the manifest file within the manifest repository | ||
| (remote or local). Config option manifest.file will be set to this value. | ||
| Defaults to `{_WEST_YML}` if not provided. |
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.
Backquotes are useful in .rst docs but they look weird. Not important.
src/west/app/project.py
Outdated
|
|
||
| If both `-t / --topdir` and `directory` are provided, `-t / --topdir` | ||
| specifies the workspace directory, while `directory` specifies the subfolder | ||
| within the workspace where the repository is cloned during initialization |
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.
| within the workspace where the repository is cloned during initialization | |
| within the workspace where the manifest repository is cloned during initialization |
src/west/app/project.py
Outdated
|
|
||
| The topdir (where `.west` is created) is determined as follows: | ||
| - argument `-t / --topdir` if provided | ||
| - otherwise: argument `directory` if provided |
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.
| - otherwise: argument `directory` if provided | |
| - otherwise: the positional `directory` argument if provided without --topdir. Deprecated: prefer --topdir. |
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.
I think if provided without --topdir is not relevant (same as your comment above where I removed if -m is used).
Also here I would propose to do the Deprecation in a separate PR.
src/west/app/project.py
Outdated
| - otherwise: the current working directory | ||
|
|
||
| If both `-t / --topdir` and `directory` are provided, `-t / --topdir` | ||
| specifies the workspace directory, while `directory` specifies the subfolder |
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.
| specifies the workspace directory, while `directory` specifies the subfolder | |
| specifies the workspace directory, while the positional directory argument specifies the manifest subfolder |
| If no `-m / --manifest-url` is provided, west uses Zephyr URL by default: | ||
| {MANIFEST_URL_DEFAULT}. | ||
|
|
||
| The topdir (where `.west` is created) is determined as follows: |
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.
To simplify the logic, we can just deprecate using a positional argument instead of the new --topdir. This will make the local and cloning case more consistent with each other.
src/west/app/project.py
Outdated
| metavar='directory', | ||
| help='''With --local: the path to a local directory which contains | ||
| a west manifest (west.yml); | ||
| Otherwise (if -m is used and no --topdir is provided): |
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.
Wait: -m is not relevant here, is it?
| Otherwise (if -m is used and no --topdir is provided): | |
| Otherwise, if no --topdir is provided: |
src/west/app/project.py
Outdated
| topdir = Path(args.topdir or manifest_dir.parent).resolve() | ||
|
|
||
| if not manifest_file.is_relative_to(topdir): | ||
| self.die(f'{manifest_file} must be relative to west topdir') |
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.
| self.die(f'{manifest_file} must be relative to west topdir') | |
| self.die(f'{manifest_file} must be relative to west topdir {topdir}') |
src/west/app/project.py
Outdated
| ) | ||
| subdir = os.path.relpath(args.directory, topdir) | ||
| elif args.directory: | ||
| topdir = Path(abspath(args.directory)) |
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.
| topdir = Path(abspath(args.directory)) | |
| self.wrn(f"DEPRECATED, use --topdir {args.directory} instead") | |
| topdir = Path(abspath(args.directory)) |
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.
I would propose to do the deprecation in a separate PR.
src/west/app/project.py
Outdated
| if args.topdir: | ||
| topdir = Path(abspath(args.topdir)) | ||
| if args.directory: | ||
| if not Path(abspath(args.directory)).is_relative_to(topdir): |
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.
abspath(args.directory) does not make sense because args.directory is meant to be appended to topdir first. And then the former is automatically relative to the latter. So this check does not make sense to me.
src/west/app/project.py
Outdated
| manifest_path = PurePath(urlparse(manifest_url).path).name | ||
| mf_path = PurePath(urlparse(manifest_url).path).name | ||
|
|
||
| manifest_path: Path = Path(subdir) / mf_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.
This looks like a misunderstanding... manifest.path comes either from the command line, or from self.path in the manifest file, but it's never a combination of both. If both are defined, the code should fail if they don't match.
If none is defined, keeping falling back on basename(url) like before.
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.
Without prepending subdir the test fails when topdir is Path('subdir') and the positional argument directory is Path('subdir') / 'extra' / 'level')
As per definition:
If both `-t / --topdir` and `directory` are provided, `-t / --topdir`
specifies the workspace directory, while positional argument `directory`
specifies the subfolder within the workspace where the manifest repository is
cloned during initialization
Therefore the manifest will be located under <topdir>/<subdir>/<yml_path>
UPDATE: I have locally merged your PR (#906) and all tests still pass 👍🏻
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.
Without prepending subdir the test fails...
Tests are only a means to an end, if they don't reflect the desired behavior then they need to change.
As per definition
In the --local case, the positional argument becomes manifest.path[*]. For consistency, I think we want the same in the cloning case. If that clashes with self.path then we should either error or warn, not sure which.
[*] Not "literally" because absolute/relative but they do point at the same directory.
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.
I’m not sure we’re talking about the same thing. Let’s align.
When west init --topdir subdir subdir/extra/level is invoked, my expectation is that:
- The workspace is created under
./subdir, i.e../subdir/.westis created. - The Zephyr repository is cloned to
<topdir>/extra/level/zephyr/.git(which is./subdir/extra/level/zephyr/.git). - The
manifest.pathconfig option points toextra/level/zephyr. - The
manifest.fileconfig option is set towest.yml.
Does this match your expectations? Am I incorrect about any of these points, or missing something important?
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.
The Zephyr repository is cloned to /extra/level/zephyr/.git
No this does not match my expectations. For consistency with west init -l topdir/extra/level/zephyr, I'm expecting west init --topdir topdir topdir/extra/level/zephyr + an error or a warning when self.path is present and different from extra/level/zephyr. Long story short, all these arguments should always point at the manifest directory for consistency. Some arguments may be absolute, others like self.path must be relative but they should all point to the same directory in the end for simplicity. In case of a failure becaus of a conflict with self.path, the error message could require the users to 1. clone 2. west init in two steps.
Please prove me wrong and demonstrate that there is a more user-friendly UI design, but again it would have been more efficient to have this very discussion about the user interface and design first in #774 BEFORE writing the corresponding code, rather then "reserve-engineering" the code at code review time to discover that the UI design was not formalized.
This PR is 100% about nesting subdirectories, an area where "off by one" and similar errors are extremely common[*]. So it is critical to pay great attention to such nesting details. This is also why I repeatedly warned about phrasing like "thing gets created inside dir0" in the help text: does it mean "thing=dir0" or "dir0/thing"? No one knows. Less ambiguous language is required.
[*] examples:
- https://en.wikipedia.org/wiki/Tar_(computing)#Tarbomb
- https://unix.stackexchange.com/questions/402555/why-add-a-trailing-slash-after-an-rsync-destination
- Run
cp -r dir0/ dir1/twice, starting with non-existent dir1. Results depend on the operating system!
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.
Let's try to figure out the actual disagreement here, step by step. First of all, let's use proper full commands. @marc-hb and @thorsten-klein in the examples you are providing above with west init —topdir you are not including -l nor -m. Can we start by defining the full command-line arguments of the command that you @marc-hb think should work differently?
5d88b67 to
9647d77
Compare
9647d77 to
88035ae
Compare
88035ae to
553ce1a
Compare
Added new argument for west init to specify the west topdir when a workspace is inizialized.
553ce1a to
482e8d4
Compare
|
From #854 (comment)
Also, it's very time-consuming and error prone to have high-level design discussions buried in in tens of comments about typos and other, low-level implementation details. Let's discuss user interface and other high-level design points in issue #774 rather than in this PR. #774 is a bit noisy because of some initial misunderstandings but it's not as noisy as here. |
multiple combinations of arguments are tested as they can be provided to 'west init'.
482e8d4 to
bb01594
Compare
In an ideal world, sure. But I think in this particular case we are already too far ahead in the implementation and design to change the location of the discussions in my opinion. Reading your comments, I don't think we are that far from agreeing to a solution here, so @thorsten-klein perhaps we can try to resolve the last points (specifically this comment). |
It's not really a "change" because all the BTW I'm writing "high-level design" as in "looking at the --help, not the code". This is only about defining what the command-line parameters mean, it's not advanced maths! It's just faster to discuss and define this before writing and reviewing the code, that's all.
Agreed, and I also love being proven wrong. I'm only upset about the time-consuming way to get there. EDIT, trying to be fair: @thorsten-klein did not initially intend to implement |
Fixes #774 (Successor of #775)
This PR introduces a new
--topdiroption forwest init, allowing users to explicitly specify the west workspace directory (topdir) during initialization.This is particularly useful when using
--local, as the current behavior always sets the parent of the given directory as the topdir. In addition, west is currently unable to determine bothmanifest.pathandmanifest.filefrom a singledirectoryargument as the path can be interpreted in multiple ways (e.g. it can be split at various places).With the new
--topdirargument, users can now directly control where the west workspace is created.Parameterized tests for
west init --topdirare added as well.Moreover, the help texts and descriptions are (hopefully) simplified.