doc: improve west's repo-tool.rst #31119
Conversation
doc/guides/west/repo-tool.rst
Outdated
| as project revisions. | ||
|
|
||
| Although ``manifest-rev`` is a normal Git branch, west will recreate and/or | ||
| reset it on the next update. For this reason, it is normally a **bad idea** |
There was a problem hiding this comment.
I would remove normally.
It should be considered a bad idea to do so.
Maybe there could be rare cases when it is needed, but it's still a bad idea.
| reset it on the next update. For this reason, it is normally a **bad idea** | |
| reset it on the next update. For this reason, it is a **bad idea** |
There was a problem hiding this comment.
How about we say it is **dangerous** instead?
I think there are legitimate, if advanced, use cases for manipulating it directly.
There was a problem hiding this comment.
**dangerous** sounds good.
There was a problem hiding this comment.
Great, that's pushed.
Various improvements to the west repo-tool.rst page and pages that link to it: Rename the page title to "Basics", since it documents, well the basics, including built-in commands like "help" an "config" that are not directly related to multiple repositories. The "multi-repo" term was invented before we started using "workspace". Drop it from the text and rework things using words like "workspace", "basics", or "built-ins" instead, which read better. We've been using west for a long enough time that justifying its existence prominently at the start of this page is no longer necessary; move that to the "dustbin of history" page (why.rst). Try harder to clarify exactly what a "project" is, along with other workspace related clarifications. Improve the documentation for the west init and west update commands, and create :ref: targets for linking directly to them for convenience elsewhere in the docs. Slim down the usages for other built-in commands, so we don't have to keep those up to date as carefully each release. This is about to be important for west 0.9, which is going to change the detailed semantics for the "[PROJECT ...]" part of each command synopsis in a way that will be inconvenient to duplicate for each of these. Move the "Topologies supported" content lower down. Try to help the reading flow by letting people get familiar with a workspace and what you can do with it using the Zephyr GSG workspace that they probably already have before overwhelming them with other possibilities and choices. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic
force-pushed
the
west-0.9-doc-prep
branch
from
ff3bb24
to
c769e5c
Compare
| A workspace can contain additional Git repositories or other files and | ||
| directories not managed by west. West basically ignores anything in the | ||
| workspace except :file:`.west`, the manifest repository, and the projects | ||
| specified in the manifest file. |
There was a problem hiding this comment.
I find this clarification and the similar one about paths above extremely useful, thanks.
On the other hand I don't see anything about nesting one west project inside another (assuming of course git does not mind, pretty sure it does not).
There was a problem hiding this comment.
On the other hand I don't see anything about nesting one west project inside another (assuming of course git does not mind, pretty sure it does not).
I think this is implied by:
Projects can be located anywhere inside the workspace, but they may not
"escape" it. Project repositories need not be located in subdirectories of
the manifest repository or as immediate subdirectories of the topdir.
However, projects must have paths inside the workspace.Nested projects obey these rules.
There was a problem hiding this comment.
It is indeed implied by this but I think it should be explicit because nesting projects is not "obviously OK", far from it IMHO. See most recent and "scary" exemple at zephyrproject-rtos/west#459 (comment)
Adding "including nested projects" somewhere in the middle of that paragraph would be enough. That's just 3 words.
| This command creates a west workspace. It can be used in two ways: | ||
|
|
||
| 1. Cloning a new manifest repository from a remote URL | ||
| 2. Creating a workspace around an existing local manifest repository |
There was a problem hiding this comment.
To me this introduction sounds like 1. and 2. are alternatives, however in the longer description below Option 2 does a subset of what Option 1 does.
So I would prefer something like: "This command creates west workspace from a manifest repository. west init can clone the manifest repository from a remote URL or it can use one that has already been cloned locally."
There was a problem hiding this comment.
To me this introduction sounds like 1. and 2. are alternatives, however in the longer description below Option 2 does a subset of what Option 1 does.
They are mutually exclusive alternatives. Option 2. requires using west init -l, and 1. does not allow using -l. You'll see 1. and 2. referred to as "bootstrap" and "local" initializations in the west init source code respectively.
There was a problem hiding this comment.
Option 1 does A+B while Option 2 performs only B. I understand Option 1 and 2 are mutually exclusive in the user interface, I was merely suggesting a different presentation focusing more on A and B than on the user interface, I think it would have been easier to read. Never mind, the current text has no ambiguity
| #. Fetches any project revisions which are not already pulled from the | ||
| remote | ||
| #. Sets each project's :ref:`manifest-rev <west-manifest-rev>` branch to the | ||
| revision specified for that project in the manifest file |
There was a problem hiding this comment.
Or rather:
#. If missing locally, fetches the revision specified for each project in the manifest file
#. Sets each project's :ref:`manifest-rev <west-manifest-rev>` branch to that revision
The suggested change is to move the "revision specified for each project in the manifest file" one item up where it is used first.
There was a problem hiding this comment.
The suggested change is to move the "revision specified for each project in the manifest file" one item up where it is used first.
I'll fold this idea into the 0.9 docs PR I'm preparing today, thanks.
| For safety, ``west update`` uses ``git checkout --detach`` to check out a | ||
| detached ``HEAD`` at the manifest revision for each updated project, | ||
| leaving behind any branches which were already checked out. This is | ||
| typically a safe operation that will not modify any of your local branches. |
There was a problem hiding this comment.
"If you were already using a detached HEAD and no branch, either because you forgot to create a branch before committing, or because upstream performed the west equivalent of a git force-push, or for any other reason, then git will warn you that it will be garbage-collected and lost at some point in the future."
There was a problem hiding this comment.
Will address in follow up
| typically a safe operation that will not modify any of your local branches. | ||
|
|
||
| If you would rather rebase any locally checked out branches instead, use | ||
| the ``-r`` (``--rebase``) option. |
There was a problem hiding this comment.
Sorry I forgot what happens with detached HEADs in this case: --rebase (silently?) ignored?
Maybe I'm part of the "careless and detached HEADs" minority that performs "impulse commits" while forgetting to create a branch. Even if we're a minority, I think we deserve a tiny bit of documentation because losing commits is really, really not cool. Having this documented in west help update would work too.
There was a problem hiding this comment.
Sorry I forgot what happens with detached HEADs in this case:
--rebase(silently?) ignored?
That's deliberately undefined behavior for now because I think we may want to change it at some point. The current implementation just ignores. See zephyrproject-rtos/west#381 for our existing discussion on the subject.
There was a problem hiding this comment.
Ha! I tend to forget "undefined" a lot, that explains.
Still, it's very scary how much @marc-hb commented on zephyrproject-rtos/west#381 just a year ago and forgot all about it :-(
While waiting for 381 I think the docs should mention this as "explicitly undefined" because I suspect committing on detached HEADs is relatively common for the simple reason that it is "what's there".
|
|
||
| If you would like ``west update`` to keep local branches checked out as | ||
| long as they point to commits that are descendants of the new | ||
| ``manifest-rev``, use the ``-k`` (``--keep-descendants``) option. |
There was a problem hiding this comment.
I never used this update -k option and only now I'm realizing that a simple git rebase can also achieve the exact same effect because git rebase is in such a case smart enough to do absolutely nothing. Demonstration:
$ git status; git rev-parse HEAD
"Your branch is ahead of 'origin/master' by 1 commit."
2f47b177d12
# Go back in time
$ git update-ref refs/remotes/origin/master origin/master~2
$ git status
"Your branch is ahead of 'origin/master' by 3 commits."
# Go forward in time
$ git fetch
$ git rebase
"Current branch master is up to date."
$ git status; git rev-parse HEAD
"Your branch is ahead of 'origin/master' by 1 commit."
2f47b177d12Now I understand this does not mean west update --rebase is globally equivalent to west update --keep-descendants, however it means the difference between the two is actually quite subtle and could use an example or two (not necessarily in this page)
cc: @kkasperczyk-no
-k and -r were added in zephyrproject-rtos/west#165
There was a problem hiding this comment.
Now I understand this does not mean west update --rebase is globally equivalent to west update --keep-descendants, however it means the difference between the two is actually quite subtle and could use an example or two (not necessarily in this page)
Submitted at: #31201 (could not add @kkasperczyk-no as a reviewer, not enough github power?)
There was a problem hiding this comment.
I'll consider this addressed by your PR. Thanks for sending that.
Various improvements to the west repo-tool.rst page and pages that
link to it:
Rename the page title to "Basics", since it documents, well the
basics, including built-in commands like "help" an "config" that are
not directly related to multiple repositories.
The "multi-repo" term was invented before we started using
"workspace". Drop it from the text and rework things using words like
"workspace", "basics", or "built-ins" instead, which read better.
We've been using west for a long enough time that justifying its
existence prominently at the start of this page is no longer
necessary; move that to the "dustbin of history" page (why.rst).
Try harder to clarify exactly what a "project" is, along with other
workspace related clarifications.
Improve the documentation for the west init and west update commands,
and create :ref: targets for linking directly to them for convenience
elsewhere in the docs.
Slim down the usages for other built-in commands, so we don't have to
keep those up to date as carefully each release. This is about to be
important for west 0.9, which is going to change the detailed
semantics for the "[PROJECT ...]" part of each command synopsis in a
way that will be inconvenient to duplicate for each of these.
Move the "Topologies supported" content lower down. Try to help the
reading flow by letting people get familiar with a workspace and what
you can do with it using the Zephyr GSG workspace that they probably
already have before overwhelming them with other possibilities and
choices.
Signed-off-by: Martí Bolívar marti.bolivar@nordicsemi.no