-
Couldn't load subscription status.
- Fork 67
[zep noup] document the additional patches #77
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: zephyr
Are you sure you want to change the base?
[zep noup] document the additional patches #77
Conversation
095bd82 to
caf6c4a
Compare
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.
Few questions:
- Does this mean every commit in the modules should have 1 of the 4 mentioned tags?
- Do we expect the
fromlistcommits to be reverted and swapped withfromtreecommits once they are merged upstream? - Could we consider no tag as
fromtreeto avoid editing the upstream cherry-picked commits?
|
@wearyzen this tagging process is something Marti came up with in Founderies and brought to Nordic. The general objective though should be a minimal amount of downstream patches being maintained and managed. To answer your questions though> Few questions:
Yes, all commits that are not part of sync/merge from the upstream shall have a tag to identify them as "Zephyr Project" additions.
These questions are actually a larger question of what process is used for cleanup. When you have fromlist/fromtree tags, they imply commits that will eventually be pulled into the Zephyr fork with some later update. So how we clean these up are important. I will also add that @tomi-font do you want to articulate the synchronization and clean up process? I know that @d3zd3z does not want a revert/reapply process. I can think of other methods we can do for a module that doesn't include that flow which might be similar to how we do it now, although I'm not aware of how we have been doing it? |
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.
Just some minor comments, otherwise looks good.
README.md
Outdated
|
|
||
| Compiler options can be set using conventional environment variables such as `CC` and `CFLAGS` when using the Make and CMake build system (see below). | ||
| It shall be cherry picked with the `-x` flag so that the hash of the upstream commit is present in the commit message for tracking purposes. | ||
| However, no sign-off shall be added by the person cherry picking the commit and the original commit message shall be left unmodified, only the commit title edited to preprend `[zep fromtree]` to it. |
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.
So, technically, at least per Linux rules, a sign-off is required by anyone who cherry picks the patch. Given the overall project is under the LF, I suspect this rule would still apply here.
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 it necessary applies just by inheritance from Linux. I proposed no sign-off because to me it doesn't add any value, but all we really care about here is to agree on and enforce one way of doing, so I'm good with either.
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 tend to agree with @tomi-font here. We can set our own rules here and refer to other project for guidance/inspiration.
README.md
Outdated
| The main Mbed TLS documentation is available via [ReadTheDocs](https://mbed-tls.readthedocs.io/). | ||
| We have an upstream-first approach, so any changes that can be upstream should first be submitted upstream. | ||
| A `fromlist` comes in handy for cherry picking changes that have been submitted but not yet merged upstream. | ||
| They should however have reasonable expectations to be merged with few modifications upstream, and it is preferable to get at least a first round of reviews on the upstream PR before cherry picking it to ensure that. |
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 might be worth noting that 'fromlist' is historical, and this applies to changes living as PRs. It might be worth spelling out that Gerrit review is also a possibility.
README.md
Outdated
| Documentation for the PSA Cryptography API is available [on GitHub](https://arm-software.github.io/psa-api/crypto/). | ||
| `fromlist` commits, just as `fromtree` ones, shall have no added sign-off. | ||
| In contrast, however, they must not be cherry picked with the `-x` flag. This is because the commit hash most likely won't point to the upstream commit once the upstream PR is merged. | ||
| Instead, the following line shall be added at the bottom of the commit message (after the sign-off of the upstream author): `Upstream PR #: <PR number>` |
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.
Perhaps describe how to reference in-review Gerrit patches as well, that would let us just use this same document across tf-m, etc.
To me no, except in the exceptional case where the commit merged upstream would turn out to have differences with the fromlist that was cherry picked and for some reason those changes are needed before the next update. In that case you would open a PR to do that but otherwise it's just additional unneeded burden.
I'm not very fond of that idea. The tags make it very easy and fast to recognize patches (and their types), for commits without tags I would feel the need to check the commit message and maybe even more. Also, how about the case where the first commits in this repo are fromtrees (which is how we typically order them when updating)? Now you have to chase down what is the latest upstream commit and what is the first fromtree. Plus taking into consideration that upstream commits might themselves be cherry picks (from other branches)... |
I could take a stab at it, I'm also in favor of dropping the revert approach. |
|
Thanks for the clarification! I initially misunderstood and thought this tagging process applied to all commits, including the initial ones used to sync with the latest upstream. Apart from adding some guidance for sync/cleanup process, the rest looks looks good to me as well. |
I'm actually a bit undecided on the toup. The reason I included it is for legit cases where the commit might be available upstream (either in a PR or merged) but cannot be cherry picked as-is because of conflicts that cannot be resolved by just cherry picking some more upstream commits (or can but which would bring unacceptable changes). What do y'all think about this? |
caf6c4a to
d13d488
Compare
|
Updated according to comments and replaced |
Turns out @valeriosetti will propose something as 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.
Generally LGTM
|
d13d488 to
81812de
Compare
|
According to feedback I replaced |
|
|
||
| There are currently three active build systems used within Mbed TLS releases: | ||
| In those cases it is acceptable to resolve the conflicts manually and/or remove the undesired changes. | ||
| However make sure to document what you did and the rationale at the very bottom of the commit message, right before your own sign-off. |
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.
So the structure here will be:
Original commit message
Original signoff
Commit message regarding the dirty cherry pick
Cherry pick signoff?
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.
Correct.
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.
Then its all good, I was thinking if it makes sense to add something to distinguish the cherry pick message but since it will be placed after the original signoff it should be visible enough.
Replace the upstream README with guidelines for the additional patches. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>
81812de to
ecbc60e
Compare
|
I see that toup went completely away. I think you should consider still having it but document it as "to be used sparingly... try posting a PR upstream and cherry-picking it back". But there might be cases where we need to apply a patch that we want to submit upstream but for undetermined reasons we can't yet... |
I tend to agree here. I can see a hot fix needed being submitted here before making it upstream. There should be all sorts of discouragements to this type of commit, but we should still cover it IMHO. |
|
So what you both are talking about would now be covered by a |
I think |
Replace the upstream README with guidelines for the additional patches.
Documentation preview: https://github.com/tomi-font/zephyr-mbedtls/tree/repo_maintenance_guidelines?tab=readme-ov-file#additional-patches