patching: Rewriter: actually delete empty patch files on rewrite #9209
+5
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThe patching module now logs failed or unrewritten patches at warning level and removes the corresponding patch files from disk after attempting to apply them. (49 words) Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/tools/patching.py:
- Around line 377-381: The loop over FAILED_PATCHES may call
os.remove(failed_patch.parent.full_file_path()) multiple times for the same file
and has no error handling; change the logic to collect unique parent paths (use
failed_patch.parent.full_file_path() or a set of parent identifiers) and iterate
once per unique path, call log.warning once per path (including the failing
patch counters if desired), and wrap os.remove(...) in a try/except that catches
OSError/FileNotFoundError to log an error via log.exception or log.error instead
of letting the script crash.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/tools/patching.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.
Learnt from: igorpecovnik
Repo: armbian/build PR: 8705
File: .github/workflows/rewrite-kernel-configs.yml:96-169
Timestamp: 2025-10-03T11:42:46.660Z
Learning: In the Armbian build repository's kernel config rewrite workflow (.github/workflows/rewrite-kernel-configs.yml), deletion of kernel config files does not need to be handled. The workflow intentionally only captures ACMR (added/copied/modified/renamed) changes and does not need to propagate deletions to the aggregator.
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:65-72
Timestamp: 2025-09-14T11:37:35.089Z
Learning: In the Armbian build system, patch directories referenced in BOOTPATCHDIR and KERNELPATCHDIR configurations can be non-existent without causing build failures. Empty patch directories are also ignored by git, so missing patch directories should not be flagged as errors during code review.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: igorpecovnik
Repo: armbian/build PR: 8705
File: .github/workflows/rewrite-kernel-configs.yml:217-220
Timestamp: 2025-10-04T17:59:19.369Z
Learning: In the Armbian build repository's kernel config rewrite workflow (.github/workflows/rewrite-kernel-configs.yml), the workflow is designed to rewrite existing kernel configs only. New/untracked files can be safely ignored, so using `git diff --quiet` (which ignores untracked files) is the correct approach for detecting changes rather than `git status --porcelain`.
Learnt from: leggewie
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-29T18:44:47.732Z
Learning: When creating GitHub issues for code improvements in the Armbian build project, include the relevant patches/diffs in the issue description or open a PR directly instead of just describing the changes. This makes it more efficient for the assignee to handle the improvements.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: rpardini
Repo: armbian/build PR: 9180
File: patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml:4-8
Timestamp: 2026-01-07T18:21:35.944Z
Learning: In Armbian patch/kernel/archive/**/0000.patching_config.yaml files, the version metadata fields (name, branch, last-known-good-tag, kind, type) under the "info stuff" comment are not used by the patching scripts. When reviewing these files, do not suggest updating or fixing these metadata fields. Instead, suggest removing them entirely as they serve no functional purpose.
Learnt from: EvilOlaf
Repo: armbian/build PR: 9096
File: patch/kernel/archive/sunxi-6.18/patches.armbian/arm64-dts-sun50i-h616-add-overlays.patch:4-5
Timestamp: 2025-12-17T15:46:22.709Z
Learning: In Armbian kernel patches, when patches are renamed or reorganized, the old Subject line may be intentionally preserved in the patch header (as a second Subject line) for archival/historical purposes to document the previous patch name. This is an Armbian-specific practice for tracking patch evolution and should not be flagged as an error when the maintainer has intentionally included it for documentation.
Comment on lines
377
to
+381
| for failed_patch in FAILED_PATCHES: | ||
| log.info( | ||
| f"Consider removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); " | ||
| f"it was not applied successfully.") | ||
| log.warning( | ||
| f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); " | ||
| f" it failed to apply/was not rewritten.") | ||
| os.remove(failed_patch.parent.full_file_path()) |
Contributor
There was a problem hiding this comment.
Critical: Duplicate file deletion attempts will crash the script.
This code will attempt to delete the same patch file multiple times if that file contains multiple patches (mbox format) that both fail to apply or weren't rewritten. The second os.remove() call will raise a FileNotFoundError, terminating the script.
Additionally, there's no error handling around os.remove(), so any deletion failure (permissions, file locked, etc.) will also crash the script.
🐛 Proposed fix: deduplicate parent files and add error handling
FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)]
+ # Deduplicate parent files to avoid attempting to delete the same file multiple times
+ failed_patch_files_to_delete = set()
for failed_patch in FAILED_PATCHES:
- log.warning(
- f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
- f" it failed to apply/was not rewritten.")
- os.remove(failed_patch.parent.full_file_path())
+ failed_patch_files_to_delete.add(failed_patch.parent.full_file_path())
+
+ for patch_file_path in failed_patch_files_to_delete:
+ log.warning(f"Removing {patch_file_path}; it failed to apply/was not rewritten.")
+ try:
+ os.remove(patch_file_path)
+ except FileNotFoundError:
+ log.debug(f"File {patch_file_path} already removed.")
+ except OSError as e:
+ log.error(f"Failed to remove {patch_file_path}: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for failed_patch in FAILED_PATCHES: | |
| log.info( | |
| f"Consider removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); " | |
| f"it was not applied successfully.") | |
| log.warning( | |
| f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); " | |
| f" it failed to apply/was not rewritten.") | |
| os.remove(failed_patch.parent.full_file_path()) | |
| FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)] | |
| # Deduplicate parent files to avoid attempting to delete the same file multiple times | |
| failed_patch_files_to_delete = set() | |
| for failed_patch in FAILED_PATCHES: | |
| failed_patch_files_to_delete.add(failed_patch.parent.full_file_path()) | |
| for patch_file_path in failed_patch_files_to_delete: | |
| log.warning(f"Removing {patch_file_path}; it failed to apply/was not rewritten.") | |
| try: | |
| os.remove(patch_file_path) | |
| except FileNotFoundError: | |
| log.debug(f"File {patch_file_path} already removed.") | |
| except OSError as e: | |
| log.error(f"Failed to remove {patch_file_path}: {e}") |
🤖 Prompt for AI Agents
In @lib/tools/patching.py around lines 377 - 381, The loop over FAILED_PATCHES
may call os.remove(failed_patch.parent.full_file_path()) multiple times for the
same file and has no error handling; change the logic to collect unique parent
paths (use failed_patch.parent.full_file_path() or a set of parent identifiers)
and iterate once per unique path, call log.warning once per path (including the
failing patch counters if desired), and wrap os.remove(...) in a try/except that
catches OSError/FileNotFoundError to log an error via log.exception or log.error
instead of letting the script crash.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Member
Author
There was a problem hiding this comment.
I don't see it like that.
Contributor
|
not used folders in
|
- so patches-to-git filtering is actually useful
fc25c17 to
0f7c467
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/tools/patching.py:
- Around line 376-382: The deletion logic is unsafe: it builds FAILED_PATCHES
from VALID_PATCHES and calls os.remove(failed_patch.parent.full_file_path()) for
each failed patch, causing duplicate removes, deleting files that were actually
rewritten, and treating skipped patches (rewritten_patch is None when
rewrite_only_patches_needing_rebase=True) as failures; fix by computing a unique
set of parent paths that truly need deletion by (a) tracking which parent paths
were successfully rewritten (e.g., collect parent.full_file_path() when
Patch.rewritten_patch is not None and the rewrite completed successfully), (b)
excluding patches skipped due to rewrite_only_patches_needing_rebase (do not
treat rewritten_patch==None as failure if the patch was intentionally skipped),
(c) building a set of failed_paths = {parent.full_file_path() for patches that
applied_ok is False and were not skipped} and then iterating over the sorted
unique failed_paths to remove each once with os.remove inside a try/except
FileNotFoundError to avoid crashing; use the symbols VALID_PATCHES,
FAILED_PATCHES, Patch.rewritten_patch, Patch.applied_ok,
parent.full_file_path(), and rewrite_only_patches_needing_rebase to locate and
implement the change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/tools/patching.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 8968
File: patch/u-boot/u-boot-sunxi/arm64-dts-sun50i-h6-orangepi.dtsi-Rollback-r_rsb-to-r_i2c.patch:36-36
Timestamp: 2025-11-20T18:20:11.985Z
Learning: The rewrite-patches tool (REWRITE_PATCHES=yes) in the Armbian build system can inadvertently introduce semantic changes when the u-boot/kernel git base revision differs from expected state. The tool applies patches, commits them, and re-exports them using git format-patch, which can cause the re-exported patch to reflect the base revision's state rather than preserving the original patch intent. This is particularly problematic for device tree changes like interrupt specifications. The tool currently lacks validation mechanisms to detect such semantic drift, and affected patches must be manually corrected after rewriting.
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:65-72
Timestamp: 2025-09-14T11:37:35.089Z
Learning: In the Armbian build system, patch directories referenced in BOOTPATCHDIR and KERNELPATCHDIR configurations can be non-existent without causing build failures. Empty patch directories are also ignored by git, so missing patch directories should not be flagged as errors during code review.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-17T05:09:02.306Z
Learning: In the Armbian build system, kernel patches (e.g., in patch/kernel/archive/sunxi-6.18/patches.armbian/) contain device tree overlays and other code that have existed for years and are maintained/ported across kernel versions. When reviewing PRs that modify these patch files, focus on the actual changes being made (e.g., Makefile fixes, new additions) rather than reviewing the entire existing content within the patch as if it were new code. The patch file contents are existing, stable code unless explicitly modified in the PR diff.
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-12-19T13:56:45.124Z
Learning: When reviewing kernel or u-boot version bump PRs in the Armbian build system, check if patches existed in previous kernel version directories (e.g., sunxi-6.12, sunxi-6.13) before describing them as new features. If a patch and the majority of its contents existed previously with no major functionality changes, focus the review on the actual changes: the version bump itself and patch compatibility adjustments. Don't describe existing patches being ported/maintained across versions as new features or drivers—this is misleading. The patches are existing code being re-aligned to work with the new upstream version.
Learnt from: rpardini
Repo: armbian/build PR: 9180
File: patch/kernel/archive/sm8250-6.18/0000.patching_config.yaml:4-8
Timestamp: 2026-01-07T18:21:41.085Z
Learning: In Armbian patch/kernel/archive/**/0000.patching_config.yaml files, the version metadata fields (name, branch, last-known-good-tag, kind, type) under the "info stuff" comment are not used by the patching scripts. When reviewing these files, do not suggest updating or fixing these metadata fields. Instead, suggest removing them entirely as they serve no functional purpose.
Learnt from: igorpecovnik
Repo: armbian/build PR: 8705
File: .github/workflows/rewrite-kernel-configs.yml:96-169
Timestamp: 2025-10-03T11:42:46.660Z
Learning: In the Armbian build repository's kernel config rewrite workflow (.github/workflows/rewrite-kernel-configs.yml), deletion of kernel config files does not need to be handled. The workflow intentionally only captures ACMR (added/copied/modified/renamed) changes and does not need to propagate deletions to the aggregator.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
Learnt from: rpardini
Repo: armbian/build PR: 8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Comment on lines
376
to
+382
| FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)] | ||
| for failed_patch in FAILED_PATCHES: | ||
| log.info( | ||
| f"Consider removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); " | ||
| f"it was not applied successfully.") | ||
| log.warning( | ||
| f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); " | ||
| f" it failed to apply/was not rewritten.") | ||
| os.remove(failed_patch.parent.full_file_path()) | ||
|
|
Contributor
There was a problem hiding this comment.
Critical: File deletion logic has multiple bugs that can cause data loss and runtime errors.
Several issues with this implementation:
-
Duplicate
os.remove()calls: If a patch file contains multiple patches and several fail, each failed patch triggersos.remove()on the same path. The first succeeds; subsequent calls raiseFileNotFoundError. -
Deletes successfully rewritten files: If a file has 3 patches where 2 succeed and 1 fails, the file is first rewritten with the 2 successful patches (line 375), then deleted here because of the 1 failed patch.
-
Deletes files skipped intentionally: When
rewrite_only_patches_needing_rebase=True(lines 364-367), patches not needing rebase are skipped and haverewritten_patch=None. These would incorrectly be treated as failed and trigger file deletion.
Proposed fix: Track parents that were successfully rewritten and use unique paths
+ # Get parents that were successfully rewritten
+ successful_parents = set(patch_files_by_parent.keys())
+
+ # Collect unique parent file paths that should be deleted (only if ALL patches failed)
+ parents_to_delete = set()
FAILED_PATCHES = [one_patch for one_patch in VALID_PATCHES if (not one_patch.applied_ok) or (one_patch.rewritten_patch is None)]
for failed_patch in FAILED_PATCHES:
- log.warning(
- f"Removing {failed_patch.parent.full_file_path()}(:{failed_patch.counter}); "
- f" it failed to apply/was not rewritten.")
- os.remove(failed_patch.parent.full_file_path())
-
+ # Only delete if this parent had no successfully rewritten patches
+ if failed_patch.parent not in successful_parents:
+ parents_to_delete.add(failed_patch.parent)
+
+ for parent in parents_to_delete:
+ log.warning(f"Removing {parent.full_file_path()}; all patches failed to apply/were not rewritten.")
+ os.remove(parent.full_file_path())🤖 Prompt for AI Agents
In @lib/tools/patching.py around lines 376 - 382, The deletion logic is unsafe:
it builds FAILED_PATCHES from VALID_PATCHES and calls
os.remove(failed_patch.parent.full_file_path()) for each failed patch, causing
duplicate removes, deleting files that were actually rewritten, and treating
skipped patches (rewritten_patch is None when
rewrite_only_patches_needing_rebase=True) as failures; fix by computing a unique
set of parent paths that truly need deletion by (a) tracking which parent paths
were successfully rewritten (e.g., collect parent.full_file_path() when
Patch.rewritten_patch is not None and the rewrite completed successfully), (b)
excluding patches skipped due to rewrite_only_patches_needing_rebase (do not
treat rewritten_patch==None as failure if the patch was intentionally skipped),
(c) building a set of failed_paths = {parent.full_file_path() for patches that
applied_ok is False and were not skipped} and then iterating over the sorted
unique failed_paths to remove each once with os.remove inside a try/except
FileNotFoundError to avoid crashing; use the symbols VALID_PATCHES,
FAILED_PATCHES, Patch.rewritten_patch, Patch.applied_ok,
parent.full_file_path(), and rewrite_only_patches_needing_rebase to locate and
implement the change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.