-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
doc: Repository structure and modules #20067
Conversation
|
||
#. Make a note of the current upstream revision that the module repository is | ||
currently upmerged to. | ||
#. Take the changes from the current to the latest upstream revision and apply |
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 might be controversial, but it's the only way I can think of to avoid the usual upmerge pitfalls.
Perhaps we should add a templated commit message here that says:
upmerge: Update from SHA1 to SHA2
Origin: project@SHA2
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 other option is to allow multiple upmerge strategies
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 say publicly what I've said before in slack/telecons: When I'm working on forks like those Zephyr requires I usually use a workspace with access to every upstream source. I find it extremely inconvenient to be guessing what upstream commit a particular fork commit is supposed to be equivalent to. A comment in a commit would at least provide a clue, but allowing real merge commits where you can see the upstream tag in its canonical form associated with the real SHA1 is the only sane way to be doing cross-repository comparisons.
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 agree that real merge commits (if merging) is the sane way to handle it.
I still think we should consider allowing sporadic rebasing, but that's a different question.
All checks are passing now. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
Replace the former "code flow and branches" section with a new one that covers the whole repository structure and the branches that each repository may have. Signed-off-by: Carles Cufi <[email protected]>
Rework some of the existing sections for clarity and extend the guide to cover upmerges. Signed-off-by: Carles Cufi <[email protected]>
* The `manifest repository`_ or also commonly named the "zephyr" repository | ||
contains the `west.yml`_ :ref:`manifest file <west-manifests>` that | ||
lists all other repositories that the manifest repository depends on. | ||
* The project repositories are the ones containing :ref:`west projects |
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 project repositories are set in the `projects:` part of :file:`west.yml`.
They aren't contained there as much as they are the same thing, no?
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.
In @mbolivar suggested edit, use double back-ticks around projects:
(not single back ticks)
lists all other repositories that the manifest repository depends on. | ||
* The project repositories are the ones containing :ref:`west projects | ||
<west-installation>`. Most (but not all) of those west projects are also | ||
:ref:`modules <modules>`. |
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'd say Zephyr modules just to disambiguate.
- Push changes frequently to upstream using the following methods: | ||
|
||
- GitHub pull requests: for example, when reviews have not been done in the local | ||
branch (one-man branch). |
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.
(only one person worked on the branch)
|
||
- GitHub pull requests: for example, when reviews have not been done in the local | ||
branch (one-man branch). | ||
- Merge requests: When a set of changes has been done in a local branch and |
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.
What is a merge request?
worry about how this variable is defined at all. The build system knows how to | ||
use west to set :makevar:`ZEPHYR_MODULES`. You can add additional modules to | ||
this list by setting the :makevar:`ZEPHYR_EXTRA_MODULES` CMake variable or by | ||
setting the :makevar:`ZEPHYR_EXTRA_MODULES` environment varialbe in |
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.
variable
|
||
.. _CMake list: https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#lists | ||
.. _add_subdirectory(): https://cmake.org/cmake/help/latest/command/add_subdirectory.html | ||
|
||
.. _GitHub issues: https://github.com/zephyrproject-rtos/zephyr/issues | ||
.. _zephyrproject-rtos: https://github.com/zephyrproject-rtos | ||
.. _west.yml: https://github.com/zephyrproject-rtos/zephyr/blob/master/west.yml |
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 are you removing the :zephyr_file:
uses?
That custom role has a hope of being extended to not go stale by emitting links for release documentation that don't point at master
. This doesn't.
See the discussion here for details on my concern: #19123 (comment)
local ones without having to modify the `manifest file <west.yml_>`_. | ||
|
||
It is also possible to avoid using west or modules at all, see the section below | ||
for more information. |
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 not just
please see :ref:`no-west` for details.
?
@@ -253,9 +263,43 @@ Where 23 in the example above indicated the pull request number submitted to the | |||
*my_module* repository. Once the module changes are reviewed and merged, the | |||
revision needs to be changed to the commit hash from the module repository. | |||
|
|||
|
|||
Upmerging forks |
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.
"upmerging" is an NCS term I would like to avoid in official upstream docs.
"Synchronizing forks" perhaps? Here and elsewhere in this section.
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.
agree, I do not really know what upmerging means in this context.
projects is constrained by the following two factors: | ||
|
||
* History must **never** be rewritten in any repository that is referenced in | ||
the `west.yml`_ manifest file, since that could break older revisions of the |
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? We could prevent the old commits being garbage collected using tags, as we do downstream.
reasons <west-why-multi-repo>`, the project's code and supporting files needed | ||
to be split out into a set of repositories instead. | ||
|
||
To that end a new tool, :ref:`west`, was developed and introduced to ease the |
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.
Better would be simply, A new tool, ...
(delete To that end
)
* The `manifest repository`_ or also commonly named the "zephyr" repository | ||
contains the `west.yml`_ :ref:`manifest file <west-manifests>` that | ||
lists all other repositories that the manifest repository depends on. | ||
* The project repositories are the ones containing :ref:`west projects |
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.
In @mbolivar suggested edit, use double back-ticks around projects:
(not single back ticks)
currently upmerged to. | ||
#. Take the changes from the current to the latest upstream revision and apply | ||
them to the current ``master`` branch of the module repository. There are | ||
several ways of achieving this It is important to note that the |
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.
missing a period at the end of the sentence this. It is
#. Submit the module content (code) to the new repository following the | ||
guidelines described :ref:`here <modules_using_west>`. | ||
#. Add a new entry to the :zephyr_file:`west.yml` with the following | ||
#. Add a new entry to the `west.yml`_ manifest file with the following |
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'd recommend leaving this as :zephyr_file:`west.yml`
(as noted below, you shouldn't lock it to master)
@@ -231,7 +241,7 @@ Changes to existing modules | |||
#. Submit the changes using a pull request to an existing repository following | |||
the :ref:`contribution guidelines <contribute_guidelines>`. | |||
#. Submit a pull request changing the entry referencing the module into the | |||
:zephyr_file:`west.yml` of the main Zephyr tree with the following | |||
`west.yml`_ of the main Zephyr tree with the following |
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'd recommend leaving this as :zephyr_file:`west.yml`
(as noted below, you shouldn't lock it to master)
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.
One minor suggestion below.
I guess the only really important comment I'd have is that you state now very categorically all repos linked from the manifest must be hosted in the zephyr-rtos org. Anyhow, I guess that if at some point it is necessary to do otherwise, then the paragraph can be just changed.
Which contains the latest state of development. In the case of :ref:`modules | ||
<modules>` that fork an external project this may not reflect the latest state | ||
of the upstream project's development, since projects are upmerged manually | ||
when required by Zephyr itself. |
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.
Bikeshedding a bit: Maybe you want to comment that they may contain patches needed by Zephyr? (meaning they are not just behind, but are actually forked)
not use modules at all if your application doesn't need them. | ||
|
||
|
||
Each module used by upstream Zephyr is contained in its own Git repository, and |
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.
s/upstream/mainline
and :ref:`west-multi-repo` for more details on how west manages multiple | ||
repositories. | ||
|
||
In that way, if you use upstream Zephyr and therefore west, you don't need to |
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.
s/upstream/mainline
@@ -253,9 +263,43 @@ Where 23 in the example above indicated the pull request number submitted to the | |||
*my_module* repository. Once the module changes are reviewed and merged, the | |||
revision needs to be changed to the commit hash from the module repository. | |||
|
|||
|
|||
Upmerging forks |
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.
agree, I do not really know what upmerging means in this context.
=============== | ||
|
||
When a module is in fact a fork of an existing, external project that is hosted | ||
on GitHub or elsewhere, it is often necessary to upmerge it: update the copy |
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.
avoid upmerge, maybe synchronize?
Extend the doc for repo structure and modules.