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

scripts: west_commands: add the west patch command #83243

Merged
merged 3 commits into from
Dec 24, 2024

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Dec 19, 2024

In smaller projects and organizations, forking Zephyr is a tenable solution for development continuity, in the case that bug-fixes or enhancements need to be applied to Zephyr to unblock things.

However, in other organizations, in the absence of healthy patch management, technical debt management, and open-source policies, and active scrutiny on the subject of internal forks, things can get out of hand quickly.

In other cases, it may just be preferable to have a zero forking / upstream first policy.

Regardless of the reason, this change adds a west patch command that enables users to manage patches locally in their modules, under version control, with complete transparence.

The format of the YAML file includes fields for filename, checksum, author, email, dates, pr and issue links. There are fields indicating whether the patch is upstreamble or whether it has been merged upstream already. Workflows can be created to notify maintainers when a merged patch may be discarded after a version or a commit bump,

In Zephyr modules, the YAML file resides conventionally under zephyr/patches.yml, and patch files reside under zephyr/patches/. Since the command to apply patches is flexible, patches can even come in the form of scripts.

Sample usage applying patches (the -v argument for additional detail):

west -v patch apply
reading patch file zephyr/run-tests-with-rtt-console.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/twister-rtt-support.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/multiple_icntl.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/move-bss-to-end.patch
checking patch integrity... OK
patching zephyr... OK
4 patches applied successfully \o/

Clean previously applied patches, leaving each module in a pristine state.

west patch clean

Behaviour after manually corrupting a patch file and passing the -r option is that all patches are automatically rolled-back.

west -v patch apply -r
reading patch file zephyr/run-tests-with-rtt-console.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/twister-rtt-support.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/multiple_icntl.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/move-bss-to-end.patch
checking patch integrity... FAIL
ERROR: sha256 mismatch for zephyr/move-bss-to-end.patch:
expect: 00e42e5d89f68f8b07e355821cfcf492faa2f96b506bbe87a9b35a823fd719cb
actual: b9900e0c9472a0aaae975370b478bb26945c068497fa63ff409b21d677e5b89f
Cleaning zephyr
FATAL ERROR: failed to apply patch zephyr/move-bss-to-end.patch

Fixes #71137

@cfriedt cfriedt force-pushed the west-patch-command branch 3 times, most recently from 0b72a15 to caa76a0 Compare December 19, 2024 21:35
@cfriedt
Copy link
Member Author

cfriedt commented Dec 19, 2024

need to write a couple of tests at some point

.ruff-excludes.toml Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Show resolved Hide resolved
@cfriedt cfriedt force-pushed the west-patch-command branch 2 times, most recently from a21bbd7 to 8dbefec Compare December 20, 2024 17:39
@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

  • improved patch-schema.yml with sub-schema and field descriptions

@cfriedt cfriedt requested a review from pdgendt December 20, 2024 17:40
scripts/schemas/patch-schema.yml Outdated Show resolved Hide resolved
scripts/west_commands/patch.py Outdated Show resolved Hide resolved
.ruff-excludes.toml Outdated Show resolved Hide resolved
@kartben
Copy link
Collaborator

kartben commented Dec 20, 2024

fixes #71137, I guess

@kartben
Copy link
Collaborator

kartben commented Dec 20, 2024

I have been using Golioth's west patch command quite successfully in the past
https://github.com/golioth/golioth-zephyr-sdk/blob/main/scripts/west_commands/patch.py

AFAIK it has a few downstream users already (, so might be worthwhile having a closer look / adopting if their approach makes sense. I quite like this doesn't require any custom yaml and "regular" patches can conveniently be dropped in a folder, but admittedly I haven't really looked in details at the implementation proposed in this PR :) Just thought I would mention of the existence of this alternative implementation that I know does the job :)
Note: Golioth SDK is Apache licensed.
cc @mniestroj @beriberikix @ubieda

@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

I have been using Golioth's west patch command quite successfully in the past golioth/golioth-zephyr-sdk@main/scripts/west_commands/patch.py

Oh wow - I didn't even realize that existed.

AFAIK it has a few downstream users already (, so might be worthwhile having a closer look / adopting if their approach makes sense. I quite like this doesn't require any custom yaml and "regular" patches can conveniently be dropped in a folder, but admittedly I haven't really looked in details at the implementation proposed in this PR :) Just thought I would mention of the existence of this alternative implementation that I know does the job :) Note: Golioth SDK is Apache licensed. cc @mniestroj @beriberikix @ubieda

I'll take a look. Maybe we can have the best of both worlds?

@kartben
Copy link
Collaborator

kartben commented Dec 20, 2024

I'll take a look. Maybe we can have the best of both worlds?

Deal! :)

@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

I'll push again momentarily, just running some tests locally.

@ubieda
Copy link
Member

ubieda commented Dec 20, 2024

We do use this downstream and it's pretty useful. Thanks for bringing this to Zephyr!

@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

  • updated to only execute checkout-command or clean-command if they are true-ish

@cfriedt cfriedt requested a review from pdgendt December 20, 2024 20:04
@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

  • corrected comment for top-level schema in patch-schema.yml

@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

  • added missing break

@cfriedt
Copy link
Member Author

cfriedt commented Dec 20, 2024

I still need to add some tests 😅 , lol

Might not get to that today because I have a few loose ends to tie up at the office still.

pdgendt
pdgendt previously approved these changes Dec 20, 2024
@kartben
Copy link
Collaborator

kartben commented Dec 21, 2024

cc @mbolivar (can't add you as reviewer yet until you're a collaborator)

ycsin
ycsin previously approved these changes Dec 21, 2024
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely helpful for orgs with access to west and have multiple teams sharing the same fork of Zephyr but need to apply different patches.

Glad to see that Golioth has something similar, so the use case is definitely there.

ubieda
ubieda previously approved these changes Dec 21, 2024
Add a pykwalify schema for patches.yml files, which are used by
the west patch command.

Signed-off-by: Chris Friedt <[email protected]>
In smaller projects and organizations, forking Zephyr is usually
a tenable solution for development continuity, in the case that
bug-fixes or enhancements need to be applied to Zephyr to
unblock development.

In larger organizations, perhaps in the absence of healthy patch
management, technical debt management, and open-source policies,
forking and in-tree changes can quickly get out of hand.

In other organizations, it may simply be preferable to have a
zero-forking / upstream-first policy.

Regardless of the reason, this change adds a `west patch`
command that enables users to manage patches locally in their
modules, under version control, with complete transparence.

The format of the YAML file (detailed in a previous comit)
includes fields for filename, checksum, author, email, dates,
along with pr and issue links. There are fields indicating
whether the patch is upstreamble or whether it has been merged
upstream already. There is a custom field that is not validated
and can be used for any purpose.

Workflows can be created to notify maintainers when a merged
patch may be discarded after a version or a commit bump.

In Zephyr modules, the file resides conventionally under
`zephyr/patches.yml`, and patch files reside under
`zephyr/patches/`.

Sample usage applying patches (the `-v` argument for additional
detail):
```shell
west -v patch apply
reading patch file zephyr/run-tests-with-rtt-console.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/twister-rtt-support.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/multiple_icntl.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/move-bss-to-end.patch
checking patch integrity... OK
patching zephyr... OK
4 patches applied successfully \o/
```

Cleaning previously applied patches
```shell
west patch clean
```

After manually corrupting a patch file (the `-r` option will
automatically roll-back all changes if one patch fails)
```shell
west -v patch apply -r
reading patch file zephyr/run-tests-with-rtt-console.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/twister-rtt-support.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/multiple_icntl.patch
checking patch integrity... OK
patching zephyr... OK
reading patch file zephyr/move-bss-to-end.patch
checking patch integrity... FAIL
ERROR: sha256 mismatch for zephyr/move-bss-to-end.patch:
expect: 00e42e5d89f68f8b07e355821cfcf492faa2f96b506bbe87a9b35a823fd719cb
actual: b9900e0c9472a0aaae975370b478bb26945c068497fa63ff409b21d677e5b89f
Cleaning zephyr
FATAL ERROR: failed to apply patch zephyr/move-bss-to-end.patch
```

Signed-off-by: Chris Friedt <[email protected]>
Add an entry to west-commands.yml for the west patch command.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt dismissed stale reviews from ubieda, ycsin, and pdgendt via d866323 December 21, 2024 15:32
@cfriedt
Copy link
Member Author

cfriedt commented Dec 21, 2024

  • switched merge-commitback to only matching 40-digit SHA-1 hashes in patch-schema.yml

@kartben kartben merged commit 64442ae into zephyrproject-rtos:main Dec 24, 2024
41 checks passed
@cfriedt cfriedt deleted the west-patch-command branch December 24, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

west patch: a unified way of handling patches
6 participants