Skip to content

allow extending or overriding the patching config from the userpatche… #8690

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.

Sign up for GitHub

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

Open
bertouttier wants to merge 1 commit into
base: main
Choose a base branch
from
Open

allow extending or overriding the patching config from the userpatche… #8690

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions lib/tools/common/patching_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,9 @@ def __str__(self):
class PatchingConfig:
def __init__(self, yaml_config_file_paths: list[str]):
self.yaml_config_file_paths = yaml_config_file_paths
if len(yaml_config_file_paths) == 0:
self.yaml_config = {}
else:
# I'm lazy, single one for now.
self.yaml_config = self.read_yaml_config(yaml_config_file_paths[0])["config"]
self.yaml_config = {}
for p in yaml_config_file_paths:
self.yaml_config.update(self.read_yaml_config(p)["config"])
Comment on lines +56 to +58
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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().

Copy link
Copy Markdown
Collaborator

@tabrisnet tabrisnet Apr 1, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 1, 2026

Choose a reason for hiding this comment

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

@tabrisnet Fully agreed — the merge semantics are subtle enough that they really need explicit documentation. At minimum, the docs should cover:

  1. Where to place the file: userpatches/kernel/<variant>/0000.patching_config.yaml
  2. Merge semantics: which keys are merged (e.g., lists like overlay-directories, dts-directories, auto-patch-dt-makefile are appended), which are overridden (scalar/non-list values), and how to explicitly suppress a base setting (e.g., setting a key to null).
  3. Load order: the base patch/kernel/<variant>/0000.patching_config.yaml is loaded first, then the userpatches file is applied on top.
  4. 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.


self.patches_to_git_config: PatchingToGitConfig = PatchingToGitConfig(self.yaml_config.get("patches-to-git", {}))

Expand Down
Loading