allow extending or overriding the patching config from the userpatche… #8690
Conversation
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
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}
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)
| self.yaml_config = {} | ||
| for p in yaml_config_file_paths: | ||
| self.yaml_config.update(self.read_yaml_config(p)["config"]) |
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.
| 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().
igorpecovnik
removed
the
Needs Documentation
igorpecovnik
force-pushed
the
allow-patching-config-extensions
branch
from
9bf6062 to
caac53d
Compare
3 participants
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: