allow extending or overriding the patching config from the userpatche…#8690
Open
bertouttier wants to merge 1 commit intoarmbian:mainfrom
Open
allow extending or overriding the patching config from the userpatche…#8690bertouttier wants to merge 1 commit intoarmbian:mainfrom
bertouttier wants to merge 1 commit intoarmbian:mainfrom
Conversation
Contributor
WalkthroughConstructor in lib/tools/common/patching_config.py was changed to remove special-case handling for zero-length yaml paths. It now initializes self.yaml_config as an empty dict and iterates over all provided yaml_config_file_paths, merging each file's ["config"] into self.yaml_config. Downstream computed values (e.g., patches_to_git_config, autopatch_makefile_dt_configs, dts_directories, overlay_directories) are derived from the merged configuration. No public/exported signatures were changed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)lib/tools/common/patching_config.py (1)
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 |
github-actions
Bot
added
size/small
Contributor
Hey @bertouttier! 👋Thanks for submitting your first pull request to the Armbian project — we're excited to have you contributing! 🧡 If you'd like to stay informed about project updates or collaborate more closely with the team, Also, don’t forget to ⭐ star the repo if you haven’t already — and welcome aboard! 🚀 |
github-actions
Bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
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/common/patching_config.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: amazingfate
PR: armbian/build#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
PR: armbian/build#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
PR: armbian/build#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.
🧬 Code graph analysis (1)
lib/tools/common/patching_config.py (1)
lib/tools/common/dt_makefile_patcher.py (2)
auto_patch_all_dt_makefiles(281-305)__init__(22-37)
Comment on lines
+55
to
+57
| self.yaml_config = {} | ||
| for p in yaml_config_file_paths: | ||
| self.yaml_config.update(self.read_yaml_config(p)["config"]) |
Contributor
There was a problem hiding this comment.
Current merge logic overwrites list settings, defeating the “extend” goal
Using dict.update() per fragment means any list-valued key from the Armbian-supplied config is wholesale replaced once the user fragment defines the same key. For example, if the base 0000.patching_config.yaml already ships with an overlay-directories entry and the user file only adds a new overlay, the final config will contain only the user-supplied list, silently discarding the base overlays. That directly contradicts the PR’s objective of letting userpatches extend the default config, and it will purge existing overlay/autopatch directives as soon as the user defines that section. Please switch to a merge routine that appends list values (and merges nested dicts) while still allowing explicit overrides when the user provides a non-list (e.g. null) value.
@@
- self.yaml_config = {}
- for p in yaml_config_file_paths:
- self.yaml_config.update(self.read_yaml_config(p)["config"])
+ self.yaml_config: dict = {}
+ for p in yaml_config_file_paths:
+ self._merge_config(self.read_yaml_config(p)["config"])
+
+ def _merge_config(self, fragment: dict):
+ for key, value in fragment.items():
+ if key not in self.yaml_config or value is None:
+ self.yaml_config[key] = value
+ continue
+ current = self.yaml_config[key]
+ if isinstance(current, list) and isinstance(value, list):
+ self.yaml_config[key] = [*current, *value]
+ elif isinstance(current, dict) and isinstance(value, dict):
+ merged = current.copy()
+ merged.update(value)
+ self.yaml_config[key] = merged
+ else:
+ self.yaml_config[key] = value📝 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
| self.yaml_config = {} | |
| for p in yaml_config_file_paths: | |
| self.yaml_config.update(self.read_yaml_config(p)["config"]) | |
| # in __init__ of PatchingConfig or similar class | |
| self.yaml_config: dict = {} | |
| for p in yaml_config_file_paths: | |
| self._merge_config(self.read_yaml_config(p)["config"]) | |
| # Add this method to the class | |
| def _merge_config(self, fragment: dict): | |
| for key, value in fragment.items(): | |
| # If new key, or explicit null override, just assign | |
| if key not in self.yaml_config or value is None: | |
| self.yaml_config[key] = value | |
| continue | |
| current = self.yaml_config[key] | |
| # Extend lists rather than replace | |
| if isinstance(current, list) and isinstance(value, list): | |
| self.yaml_config[key] = [*current, *value] | |
| # Shallow-merge dicts | |
| elif isinstance(current, dict) and isinstance(value, dict): | |
| merged = current.copy() | |
| merged.update(value) | |
| self.yaml_config[key] = merged | |
| # Fallback to override for other types | |
| else: | |
| self.yaml_config[key] = value |
🤖 Prompt for AI Agents
In lib/tools/common/patching_config.py around lines 55-57, the current loop uses
dict.update() which replaces list-valued keys instead of extending them; replace
that with a deep-merge routine: for each fragment, recursively merge into
self.yaml_config by merging nested dicts key-by-key, concatenating lists
(appending fragment list items to existing list) and only performing a full
override when the fragment value is not a list/dict (including explicit
null/None), so user fragments can extend default lists but still explicitly
override by providing non-list values; implement a small helper
merge_dicts(dest, src) and call it for each read_yaml_config(p)["config"]
instead of update().
Collaborator
There was a problem hiding this comment.
the 🐰 is not wrong. but the main thing IMO, whichever path of overlay/merge you take, this should be documented, including the semantics of overlay/merge.
Contributor
There was a problem hiding this comment.
@tabrisnet Fully agreed — the merge semantics are subtle enough that they really need explicit documentation. At minimum, the docs should cover:
- Where to place the file:
userpatches/kernel/<variant>/0000.patching_config.yaml - Merge semantics: which keys are merged (e.g., lists like
overlay-directories,dts-directories,auto-patch-dt-makefileare appended), which are overridden (scalar/non-list values), and how to explicitly suppress a base setting (e.g., setting a key tonull). - Load order: the base
patch/kernel/<variant>/0000.patching_config.yamlis loaded first, then the userpatches file is applied on top. - A minimal example showing a userpatches config that adds a new overlay directory without removing the base ones.
Without this, users will likely hit the silent-discard issue described above without knowing why their base overlays stopped working.
🧠 Learnings used
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: 9483
File: patch/kernel/archive/rockchip64-6.19/board-sakurapi-rk3308b-fix-wlan-broken.patch:23-24
Timestamp: 2026-03-04T17:34:23.706Z
Learning: When reviewing patches in the Armbian build system (especially in patch/kernel/archive/**), always carefully distinguish between nested patch context lines (i.e., unchanged surrounding lines in an inner/embedded diff) and actual changes introduced by the PR. Lines inside a nested diff that begin with a space are context — not modifications. Before flagging anything, verify that the change being flagged is a genuine addition or removal in the inner patch content, not just context lines that shifted due to hunk offset changes. Failing to do so leads to false positives where existing, unmodified inner patch content is incorrectly treated as a new change.
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: 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: rpardini
Repo: armbian/build PR: 9159
File: patch/u-boot/u-boot-genio/0026-dts-configs-add-Grinn-GenioSBC-510.patch:161-161
Timestamp: 2026-01-03T20:46:29.189Z
Learning: For the Armbian genio family (config/sources/families/genio.conf and patch/u-boot/u-boot-genio/), when reviewing PRs that include vendor U-Boot patches from Collabora, avoid flagging potential issues in board configurations that are out of scope for the PR's primary focus (e.g., don't flag Genio 510/700 board issues when the PR is focused on radxa-nio-12l/Genio 1200). The maintainer prioritizes keeping vendor patches close to upstream for easier re-copying and maintenance, even if secondary board configs have potential mismatches.
<!-- </add_learning>
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2026-03-18T16:31:36.035Z
Learning: In patch files for the Armbian build system (e.g., patch/kernel/archive/sunxi-6.18/patches.armbian/), lines visible in a diff hunk that are prefixed with a space are context lines — they are unchanged surrounding content, not modifications introduced by the PR. Never describe context lines as additions or new code in summaries or reviews. Only lines with `+` or `-` prefixes represent actual changes. Misidentifying context lines as additions leads to incorrect summaries (e.g., falsely claiming a closing brace or `#endif` was "added" when it was already present).
Learnt from: EvilOlaf
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2026-03-25T17:36:06.301Z
Learning: In the Armbian build system, when a PR title indicates a "rewrite" of kernel patches against a new kernel version (e.g., "rewrite kernel patches against 6.18.20" in patch/kernel/archive/rockchip64-6.18/), only the patch context lines change — the surrounding unchanged lines in the patch hunks are updated to match the new kernel base. The actual code changes introduced by the patches remain identical. Do NOT describe context-line adjustments as additions, removals, or fixes in summaries or reviews. The correct description is: "patches rewritten against kernel X.Y.Z; only patch context lines updated, no functional changes."
Learnt from: tabrisnet
Repo: armbian/build PR: 9136
File: lib/functions/compilation/armbian-kernel.sh:529-537
Timestamp: 2025-12-28T01:49:34.661Z
Learning: In lib/functions/compilation/armbian-kernel.sh's opts_m handling, checking .config (kernel defconfig) is problematic because savedefconfig elides options matching defconfig, making it impossible to distinguish overlay silence from defconfig agreement. The correct approach is to check only kernel_config_source_filename (the overlay) for explicit =y, otherwise default to =m, letting savedefconfig handle deduplication with defconfig automatically.
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: 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: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T04:13:16.457Z
Learning: Armbian kernel configuration files like linux-filogic-current.config are autogenerated overlays on top of arch defconfig. Comments added manually will be lost during future updates by maintainers, and explicit "CONFIG_OPTION is not set" statements aren't needed for mutually exclusive options since these are overlay configs that only specify changes from the base configuration.
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-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: tabrisnet
Repo: armbian/build PR: 9275
File: patch/kernel/archive/filogic-6.18/frank-w/0053-net-phy-as21-try-the-driver-from-mtk-sdk.patch:1298-1326
Timestamp: 2026-01-19T22:46:53.605Z
Learning: In the Armbian build system, patches located under `patch/kernel/archive/filogic-6.18/frank-w/` are sourced from frank-w's out-of-tree BPI-Router-Linux repository (https://github.com/frank-w/BPI-Router-Linux) and are out of scope for code review. These patches are maintained externally and should not be reviewed in Armbian build PRs.
Learnt from: HackingGate
Repo: armbian/build PR: 9535
File: patch/kernel/archive/rockchip64-6.18/rk3576-0007-counter-rockchip-pwm-capture-driver.patch:287-305
Timestamp: 2026-03-21T07:37:45.706Z
Learning: In the Armbian build system, kernel patches under `patch/kernel/archive/` that originated from the Linux kernel mailing list (identifiable by upstream authorship, `Signed-off-by` from known upstream contributors/companies like Collabora, and a `From <hash> Mon Sep 17...` git format-patch header) should NOT be reviewed. They are upstream patches being ported/maintained in Armbian's kernel archive directories (e.g., `patch/kernel/archive/rockchip64-6.18/`) and are out of scope for code review. Only Armbian-specific integration changes around these patches are in scope.
igorpecovnik
removed
the
Needs Documentation
igorpecovnik
force-pushed
the
allow-patching-config-extensions
branch
from
9bf6062 to
caac53d
Compare
igorpecovnik
added
02
bertouttier
force-pushed
the
allow-patching-config-extensions
branch
from
caac53d to
b00c6fb
Compare
Author
|
@rpardini is it possible to do a quick review here? Thank you in advance! |
tabrisnet
requested changes
Apr 1, 2026
Comment on lines
+55
to
+57
| self.yaml_config = {} | ||
| for p in yaml_config_file_paths: | ||
| self.yaml_config.update(self.read_yaml_config(p)["config"]) |
Collaborator
There was a problem hiding this comment.
the 🐰 is not wrong. but the main thing IMO, whichever path of overlay/merge you take, this should be documented, including the semantics of overlay/merge.
tabrisnet
added
05
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Description
Motivation: allow adding extra device tree overlays in the user patches folder, similar to the patch folder.
Summary: with this change it becomes possible to add a
userpatches/kernel/<variant>/0000.patching_config.yamlfile in theuserpatchesdirectory which extends/overrides settings in the Armbian-build providedpatch/kernel/<variant>/0000.patching_config.yaml.Documentation summary for feature / change
I'm not sure if documentation is really needed, since this change makes the build system more consistent with respect to adding patches in the userpatches folder.
How Has This Been Tested?
Add a
userpatches/kernel/<variant>/0000.patching_config.yamlfile and make sure that it changes/overrides settings from the originalpatch/kernel/<variant>/0000.patching_config.yaml.Checklist: