From 17a0fb448b4f18ce23731e3b46337f93c1563f2c Mon Sep 17 00:00:00 2001 From: AI Dev Date: Sat, 19 Sep 2026 06:39:51 +0000 Subject: [PATCH 1/3] review: ten rules a board-support PR walked straight through #2446 (Imou Cue 2 board support) is the shape of PR these files exist to catch, and the automated review caught eight of twelve problems in it. The four it missed, and the five it half-caught, are all gaps in what we wrote down rather than gaps in the reviewer. The binaries gate enumerated .ko/.so/.a/.bin, so an executable with no extension at all -- general/overlay/etc/ir/nrxset -- was reported back as "contains no textual changes", which is exactly what a blob looks like to anything reading a diff as text. The gate now keys on the "Binary files ... differ" marker, which is the one signal that survives, and says outright that general/overlay/ is never the place for a compiled artefact. A register table copied out of a vendor's driver gets the same treatment: it is the same provenance failure written in C. Three things the tree already owns had no rule at all. ipctool is not on a camera -- the package installs ipcinfo, and /usr/sbin/ipctool is an extutils arm that curls the tool from a "latest" release into tmpfs at first use -- so calling it from rc.local twelve seconds into boot on a WiFi-only board is four bad properties at once. An overlay file needs no Config.in, so nothing caught /etc/ir/nrxset and nrx_night_06.txt, which nothing in firmware or builder reads and which the same PR's rc.local never invokes, whatever its comment says. And S95majestic carries an -s argument, a start-stop-daemon lookup, a ten-second wait for the sensor HAL and a trap '' HUP that its own comment measured at 25 deaths out of 25 on a hi3516ev200 -- all of which a hand-rolled killall-and-relaunch throws away, on that same SoC. Every patch in this tree targets an upstream we cannot commit to. A patch against hisilicon-opensdk does not: its SITE is openipc/openhisilicon, and the file being patched is checked in there. That repository and OpenIPC/sensors join the redirect table, which listed only linux, builder, ipctool and majestic. The rest is telling contributors what already exists. customizer.sh, muxes.sh, gpio.conf, late-overlays.list and the excludes lists are a complete per-device mechanism, shipped per device in builder at the same paths -- and documented nowhere, which is why somebody writes an S01leds with GPIO 0, 4 and 9 typed into it instead. "Move it to builder" is half an answer; say which seam it becomes. Two smaller notes go with it: the "additive, so it changes nothing" exemption belongs to load_ case arms and not to the overlay, where etc/wireless/usb has quietly reached 46 retail-model arms; and an excludes list added here is keyed _, which is the generic board's key too, so #2446's list took libsns_imx307.so away from hi3516ev200_lite itself. Lastly, devmem in a shipped script is now a question rather than a given. It is the worst way to record what is soldered to a pad -- undone by anything that later exports it, lost at the next reflash -- and the camera can be told instead, through the /api/v1/pinmux surface the pins page in OpenIPC/majestic-webui draws. --- .pr_agent.toml | 64 +++++++++- CLAUDE.md | 26 ++++- best_practices.md | 219 +++++++++++++++++++++++++++++++++-- pr_compliance_checklist.yaml | 105 +++++++++++++++-- 4 files changed, 395 insertions(+), 19 deletions(-) diff --git a/.pr_agent.toml b/.pr_agent.toml index cf27cd623e..3fcb2493c0 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -49,6 +49,18 @@ Prioritise, in order: family. A sensor, I2C address, GPIO number, or resolution that is true only of the contributor's bench must not become the default for everyone else. This breaks already deployed cameras at upgrade time, which is the worst failure mode this project has. + "Additive, so it changes no existing board" is the right exemption for a load_ + sensor case arm and is not a licence for the shared overlay: etc/wireless/usb has + accumulated 46 arms keyed on retail model names while 18 builder devices override the + whole file, and those 46 are grandfathered, not precedent. + Question a devmem write in a shipped script. It used to be the only way to tell a + camera something is soldered to a pad, and it is the worst way: undone by anything + that later exports the pad, and lost at the next reflash. The camera can be told + instead - /api/v1/pinmux reports and sets what each pad is wired to, which is what + the pins page in OpenIPC/majestic-webui draws, and nightMode drives an IR + illuminator from configuration. A devmem line under general/overlay/ has to say why + configuration cannot express it and - since rc.local is shared - why every other + camera should execute it. 2. Toolchain-wide flags added without measurement. BR2_TARGET_OPTIMIZATION is appended to TOOLCHAIN_WRAPPER_OPTS in Buildroot's toolchain/toolchain-wrapper.mk, so it is baked @@ -65,14 +77,40 @@ Prioritise, in order: URL the .mk actually fetches. Binaries lifted out of a camera's factory firmware are not a source of supply: there is no source, no vendor SDK, and no way to rebuild them for the next kernel. + Detect a binary by the DIFF MARKER, never the extension. A hunk rendered as + "Binary files a/... and b/... differ" is a blob whatever it is named - an executable + with no extension at all reads to a text-diff summariser as "contains no textual + changes", and that is how one shipped past review into general/overlay/, which is + copied verbatim into every image. Report it, and say you cannot see its contents. + A register initialisation table copied out of a camera vendor's own driver is the same + provenance failure written in C: unverifiable against a datasheet, and replacing a + whole table rather than adding the deltas a board needs changes every other board + using that sensor. + A new *.patch is fine against a third-party upstream (every patch in the tree today is + - ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, libwebsockets, f2fs-tools, the + Realtek drivers) and is a finding when the package's *_SITE resolves to an openipc + repository, because there the fix has somewhere better to go and a *_VERSION bump + silently drops it. 4. Changes that belong to a different repository. Kernel code and kernel patches belong to OpenIPC/linux. Support for one retail camera model belongs to OpenIPC/builder, under devices/common/br-ext-chip-/ - general/overlay/usr/sbin/sysupgrade already encodes this split, routing lite|ultimate|neo to firmware and everything else to - builder. Hardware probing and bring-up tools belong to OpenIPC/ipctool. Bugs in the - majestic streamer belong to majestic's maintainers. Redirecting a contributor is a + builder. Hardware probing and bring-up tools belong to OpenIPC/ipctool. HiSilicon + sensor drivers and open SDK code belong to OpenIPC/openhisilicon, which is what + hisilicon-opensdk fetches; Sigmastar sensor drivers belong to OpenIPC/sensors. Bugs in + the majestic streamer belong to majestic's maintainers. Redirecting a contributor is a normal, useful review outcome; say which repo and why. + Say which SEAM too, not only which repo. The tree already has one for every + board-specific thing, and a contributor who has not been shown it writes a new init + script with the pin numbers typed in: /usr/share/openipc/customizer.sh and + /usr/share/openipc/muxes.sh (both run by S30customizer), the declarative pin map + /usr/share/openipc/gpio.conf, late-overlays.list, and + general/scripts/excludes/_.list. Builder ships all of them per device + at those same paths under devices//. An excludes list added to THIS repo is not + per-board - the key is _, which the generic board answers to as well. + A comment explaining why a workaround is needed has usually already named the + repository that owns the fix. Read it as a redirect and quote it back. 5. Monkey-patching in place of a fix. Dynamic-linker injection shims - a library forced ahead of the real one through the linker's LD_* environment knob, spelled out in @@ -109,6 +147,28 @@ Prioritise, in order: builds is not compiled by CI, so nothing proves it even builds. And a diff must do what its title says: a PR adding one sensor has no business repointing a SITE that every board of that vendor consumes. + The overlay's mirror image of this needs saying separately, because general/overlay/ + needs no Config.in and no .mk - anything dropped there ships, so a file nothing on the + camera ever opens passes every dead-code check while costing every board of every + vendor, several of which sit within 32 KB of their squashfs cap. Grep the installed + path of an added overlay file; if nothing reads it, that is a finding. + +8. Commands a shipped script calls must exist on the image. A camera has no package + manager and often no network, and a missing command does not fail loudly - sh prints + "not found" to a console nobody reads and carries on, so the script reports success + having done nothing. Busybox applets and binaries installed by a package the board's + defconfig selects are the whole set. ipctool is the trap that looks like an exception: + general/package/ipctool/ipctool.mk installs ipcinfo and nothing else, so + BR2_PACKAGE_IPCTOOL=y puts no "ipctool" on the image; /usr/sbin/ipctool is a symlink to + extutils, whose ipctool arm downloads the tool from the ipctool repository's "latest" + release into /tmp on first use. That is a convenience for a person at a shell, and in + a boot script it is a network fetch during boot, from a floating tag, repeated every + boot because /tmp is tmpfs, and a silent no-op until the network is up. + Relatedly, a shipped script bounces the streamer with /etc/init.d/S95majestic restart. + A hand-rolled killall-and-relaunch loses the -s argument, the start-stop-daemon -x + lookup, the ten-second wait for the old process to release the sensor HAL, and the + trap '' HUP that S95majestic carries because the disposition survives exec() where a + handler does not. Do not report: speculation about whether a change was written with AI assistance - judge the diff on blast radius, provenance, evidence, and repo ownership, never on writing diff --git a/CLAUDE.md b/CLAUDE.md index a44e267342..d516dc0210 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,6 +35,8 @@ else. Check this first — it costs a minute and can save an evening. | Kernel source and kernel patches | [OpenIPC/linux][linux] | | Support for one specific retail camera model | [OpenIPC/builder][builder] | | Probing, identification, bring-up tooling | [OpenIPC/ipctool][ipctool] | +| HiSilicon sensor drivers and open SDK code | [OpenIPC/openhisilicon][openhisilicon] | +| Sigmastar sensor drivers | [OpenIPC/sensors][sensors] | | A bug in the video stream itself | majestic's maintainers, not a shim here | | Documentation, device notes, how-tos | [OpenIPC/wiki][wiki], [docs][docs] | @@ -148,15 +150,31 @@ Summarised from `pr_compliance_checklist.yaml`; the reasoning is in `best_practi every caller, is invisible to anyone debugging the process, and freezes the real bug in place. - **No binaries without buildable source.** Nothing lifted out of a factory image, nothing a script in the same pull request generated. A `PROVENANCE.md` documents the problem; it does not - solve it. + solve it. The test is the diff marker — anything rendered `Binary files ... differ` — not the + extension; an executable with no extension is still a blob, and `general/overlay/` is never the + place for one. A register table copied out of a camera vendor's driver is the same problem + written in C. +- **No patches against OpenIPC's own packages.** Every `*.patch` in the tree targets a third-party + upstream. If a package's `*_SITE` is an `openipc` repository, the fix is a pull request there + and a `*_VERSION` bump here. +- **Nothing ships that nothing runs.** An overlay file needs no `Config.in`, so nothing catches a + file the camera never opens. Grep the installed path before adding one. +- **Shipped scripts call only what the image contains.** Busybox applets, or binaries a package in + the board's defconfig installs. `ipctool` looks installed and is not: the package ships + `ipcinfo`, and `/usr/sbin/ipctool` is an `extutils` arm that downloads the tool at first use. - **No runtime patching of vendor blob memory**, and no `kallsyms` address hooking. - **`*_SITE` points at an OpenIPC-org repository or a documented upstream**, never a personal fork, and a `*_VERSION` bump never becomes less specific than the pin it replaces — a full 40-character SHA for anything new. `Config.in` help text must name the URL the `.mk` fetches. - **No board-specific value in shared files.** A sensor name, I2C address, GPIO number, resolution, MAC prefix or IP literal does not belong in `general/overlay/` or in a shared - `load_` default. Extending a case arm, or adding a sensor to a package's list, is - additive and fine. + `load_` default. Extending a `load_` case arm, or adding a sensor to a + package's list, is additive and fine; that exemption does not extend to the overlay. + Per-board values have their own seams — `/usr/share/openipc/customizer.sh` and + `/usr/share/openipc/muxes.sh`, both run by `S30customizer`; the declarative pin map + `/usr/share/openipc/gpio.conf`; `late-overlays.list`; and + `general/scripts/excludes/_.list`. All of them live per-device in + OpenIPC/builder under `devices//`, at the same paths. ## Verifying a change @@ -341,4 +359,6 @@ Maintainers append the pull request number at merge time; you do not need to. [docs]: https://docs.openipc.org [ipctool]: https://github.com/OpenIPC/ipctool [linux]: https://github.com/OpenIPC/linux +[openhisilicon]: https://github.com/OpenIPC/openhisilicon +[sensors]: https://github.com/OpenIPC/sensors [wiki]: https://github.com/OpenIPC/wiki diff --git a/best_practices.md b/best_practices.md index 287b37e372..3a5a0ddebe 100644 --- a/best_practices.md +++ b/best_practices.md @@ -6,8 +6,9 @@ are shared, so one file there reaches every camera of a family. Those cameras si places nobody can physically reach, there is no staged rollout, and a bad change is found only after `sysupgrade` has already written it to flash. -Each rule below was written from a pull request that was actually closed. The -referenced PR is the one that motivated it. +Each rule below was written from a pull request this project actually received. The +referenced PR is the one that motivated it — usually one that was closed, occasionally +one still open where the pattern is clear enough to write down now. --- @@ -100,6 +101,65 @@ Flag any added or changed `BR2_TARGET_OPTIMIZATION`, `BR2_TARGET_LDFLAGS`, or `BR2_GLOBAL_PATCH_DIR` without a named symptom and a before/after image-size and boot check on the affected board. If the flag fixes one package, fix that package. +### 1.4 The per-device seam already exists — name it, do not just say "move it" + +"This is board-specific, take it to OpenIPC/builder" is the correct verdict and half an +answer. The tree already carries a mechanism for every board-specific thing a retail +camera needs, and a contributor who has not been shown it invents a new init script +with the pin numbers typed into it. + +`general/overlay/etc/init.d/S30customizer` is the entry point. On first boot it runs +`/usr/share/openipc/customizer.sh` once, guarded by `/etc/custom.ok`; on every boot it +runs `/usr/share/openipc/muxes.sh`, which is where a board's pinmux and GPIO presets +go. `/usr/share/openipc/gpio.conf` is the declarative pin map beside them — `button`, +`ircut1`, `ircut2`, `led1`, `led2`, `light_ir`, `light_wl`, `light_sensor`, `speaker`, +`usb`, `-1` for a pin the board does not have — read by consumers such as +`general/package/quirc-openipc/files/qrscan.sh`. Per-image pruning has a seam too: +`general/scripts/rootfs_script.sh` applies `general/scripts/excludes/_.list`, +and `late-overlays.list` ships a file only when a config symbol is set. + +All of it is per-device *in OpenIPC/builder*, at the same paths under +`devices//`: 96 devices ship a `customizer.sh`, 18 a `gpio.conf`, 15 a +`muxes.sh`, and the excludes lists live there too — this repository deliberately ships +no `general/scripts/excludes/` directory at all. The exclusion key is +`_`, which is the *generic* board's key as well, so a list added here +prunes the family board and not only the contributor's camera. + +`#2446` added `S01leds` and `S99leds` driving GPIO 0, 4 and 9 on every camera the tree +builds, a `customizer.sh` in the shared overlay branding every image an Imou Cue 2, and +a `general/scripts/excludes/hi3516ev200_lite.list` that took `libsns_imx307.so` and +`default.ini` away from the generic `hi3516ev200_lite` board. + +When redirecting, say which of these the work becomes. Note also that none of +`gpio.conf`, `muxes.sh` or `customizer.sh` is documented in OpenIPC/wiki — +`en/gpio-settings.md` is a human-readable pin table only — so the contributor has had +no way to find them. + +### 1.5 `devmem` in a shipped script is now a question, not a given + +Writing an SoC register from `rc.local` used to be the only way to tell a camera that +something is soldered to a pad. It no longer is. A `devmem` line is also the worst +available way to do it: it is undone by anything that later exports the pad, and it is +lost at the next reflash, because nothing in the camera has been told the pad exists. + +The camera can be told now. `/api/v1/pinmux` reports what every pad can be, what it is +currently, and which pads are already driven, and takes a selection back — that is what +the pins page in OpenIPC/majestic-webui draws (`www/a/mj-pins.js`), and the choice is +remembered rather than replayed from a boot script. `nightMode` drives an IR +illuminator, PWM lamp included, from configuration. `#2446` added a `devmem` block to +the shared `rc.local` programming PWM1 for its IR LED. + +So ask what owns the pad before accepting a register write. A `devmem` line under +`general/overlay/` needs to say why configuration cannot express it, and — because +`rc.local` is shared — why every other camera should execute it. + +A related trap: "additive, so it changes no existing board" is the right exemption for a +`load_` sensor case arm, and it is not a licence for the shared overlay. +`general/overlay/etc/wireless/usb` has accumulated 46 arms keyed on retail model names +in 327 lines, while 18 builder devices override the whole file. Treat those 46 as +grandfathered, not as precedent: a new arm is a board-specific value in +`general/overlay/` and belongs in the device's own copy of the file. + --- ## 2. Provenance of sources and binaries @@ -169,6 +229,37 @@ Flag any added `.ko`, `.so`, `.bin`, or firmware image that cannot be traced to vendor SDK release or a buildable source tree. A `PROVENANCE.md` documents the problem; it does not solve it. +Do not read that list of extensions as the definition. `#2446` added +`general/overlay/etc/ir/nrxset`, an executable with no extension at all, and it went +unremarked through an automated review that reported it as "the supplied patch contains +no textual changes" — which is exactly what a binary looks like to anything reading the +diff as text. The signal that always survives is the diff marker itself: + +``` +Binary files /dev/null and b/general/overlay/etc/ir/nrxset differ +``` + +Any hunk rendered that way is a binary, whatever it is called and wherever it sits, and +`general/overlay/` is never the right place for a compiled artefact — the overlay is +copied verbatim into every image, so a blob there ships to every camera of every vendor. + +### 2.5 A register table lifted from a vendor's driver is the same problem as a binary + +§2.4 is about what a file *is*; this is about what it *contains*. A sensor init sequence +copied out of a vendor's shipped driver has no more provenance than the driver did — it +cannot be corrected against a datasheet nobody has, and the reasoning behind any one +register is gone. Written as C it passes every check aimed at blobs. + +`#2446` replaced the SC2235 init table with a "Dahua DVP register sequence (114 +entries)" whose only stated origin was Dahua's own firmware, dropping about forty +registers the OpenSDK driver sets and adding others, for every Hi3516EV200 board using +that sensor. + +Flag an added or replaced register table that names a camera vendor rather than a +datasheet as its source. Ask for the deltas the board actually needs — here the PCLK +output-enable and pad-drive registers the PR itself identifies — rather than a wholesale +swap, and ask which other boards were retested. + --- ## 3. Repo boundaries @@ -227,6 +318,25 @@ Flag changes to `general/package/majestic/files/*` that alter how majestic runs order to compensate for how majestic behaves. Redirect the contributor to file the underlying issue with the majestic maintainers. +### 3.5 SoC driver behaviour belongs to the SDK repository + +`hisilicon-opensdk` fetches **OpenIPC/openhisilicon**, and the Sigmastar sensor drivers +come from **OpenIPC/sensors**. Both are OpenIPC-org repositories that take pull +requests. A sensor that mis-detects, an ISP that does not track gain, a driver that +leaves a pad unconfigured — those are changes to the driver, made once, for every tree +that consumes it. + +`#2446` is the shape to recognise. Its `rc.local` waited twelve seconds, wrote five +SC2235 registers over I²C, then killed and relaunched majestic, and the comment above it +said what it was for: the sensor's DVP pad enables, and majestic not re-reading +orientation. Both halves name an owner. The pad enables are a two-line change to +`libraries/sensor/hi3516ev200/smart_sc2235/sc2235_sensor_ctl.c` in +**OpenIPC/openhisilicon**; the orientation re-read is an issue for **OpenIPC/majestic**. +Neither is a shell script that runs on every camera the tree builds. + +A comment that explains *why* a workaround is needed has usually named the repository +the work belongs to. Read it as a redirect and quote it back. + --- ## 4. No monkey-patching @@ -271,6 +381,27 @@ reviewable — nobody rebuilds it, and the committed `.so` is what actually ship Flag any committed binary produced by a script in the same PR. If it is generated, the build system generates it; if the build system cannot, the change needs the real SDK. +### 4.4 A patch against an OpenIPC package is a pull request to that repository + +Patches in a package directory are normal here, and every one of them targets code +this project cannot commit to: ffmpeg, mbedTLS, vtund, baresip's libre, siproxd, +ZeroTier, libwebsockets, f2fs-tools, and the Realtek WiFi drivers. Not one patches a +repository under the OpenIPC organisation, because for those the fix has somewhere +better to go. + +A downstream patch against our own code is monkey-patching with extra steps. It is +invisible to anyone reading the SDK, it is silently dropped the moment someone bumps +`*_VERSION`, and every other consumer of that repository keeps the bug. + +`#2446` added `general/package/hisilicon-opensdk/0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch`. +`HISILICON_OPENSDK_SITE` is `$(call github,openipc,openhisilicon,...)`, and the file it +patches is checked in there. + +Flag a new `*.patch` in a package whose `*_SITE` resolves to an `openipc` repository. +Redirect to that repository; once it lands, bump `*_VERSION` here and the patch is not +needed. Where the change must ride ahead of the bump, say so explicitly and keep the +patch to the delta, not a wholesale replacement (§2.5). + --- ## 5. Evidence @@ -379,6 +510,28 @@ which no reviewer reading the title would look for. Flag files in the diff that the stated purpose does not explain, especially shared `.mk`, defconfig, and overlay files. Ask for them to be split into their own PR. +### 6.3 Nothing ships that nothing runs + +§6.1 catches source nobody compiles. Its mirror image is a file that reaches the rootfs +perfectly well and that nothing on the camera ever opens. `general/overlay/` needs no +`Config.in` and no `.mk` — anything dropped in it ships — so the usual dead-code check +never fires, and the cost lands on every board of every vendor, several of which sit +within 32 KB of their squashfs cap. + +`#2446` added `general/overlay/etc/ir/nrxset` and +`general/overlay/etc/ir/nrx_night_06.txt`. Nothing in firmware or builder reads either +path, and the `rc.local` the same PR ships never invokes `nrxset` — while the comment +above that code says it does. Its own two comments also disagree with each other about +whether the files are needed at all: `rc.local` says the 3DNR parameters are applied +from them and not from the IQ profile, the IQ profile says the opposite and that the +parameters were baked into it instead. Both cannot be true, and either way one of the +two is dead. + +Grep the tree for the installed path of any added overlay file. If nothing reads it, +ask what does; "the vendor's firmware had it" is not an answer. Where a comment asserts +a consumer, check that the consumer is actually called — a stale comment is how a file +keeps looking justified. + --- ## 7. Shipped shell scripts @@ -448,6 +601,50 @@ whether a script has a good reason to signal by hand. Lifecycle signalling is ne sysupgrade's SIGQUIT, and SIGTERM to stop the daemon, are not reload attempts and are not in scope for either. +### 7.3 A shipped script may only call commands the image contains + +A script under `general/overlay/` runs on a camera with no package manager, no +`$PATH` beyond what the rootfs holds, and frequently no network. A command that is not +there does not fail loudly: `sh` prints "not found" to a console nobody is reading and +carries straight on to the next line, so the script reports success having done nothing. + +`ipctool` is the trap worth knowing by name, because a defconfig line looks like it +supplies it and does not. `general/package/ipctool/ipctool.mk` installs `ipcinfo` and +nothing else, so `BR2_PACKAGE_IPCTOOL=y` puts no binary called `ipctool` on the image. +What answers to that name is `/usr/sbin/ipctool`, a symlink to +`general/overlay/usr/sbin/extutils`, whose `ipctool)` arm curls the tool from +`https://github.com/OpenIPC/ipctool/releases/download/latest/` into `/tmp` the first +time somebody asks for it — "installed as remote GitHub plugin", as it says. That is a +debugging convenience for a person at a shell, and it is four bad properties in a boot +script: a network fetch during boot, from a floating `latest` tag, repeated every boot +because `/tmp` is tmpfs, and a silent no-op until the network is up. + +`#2446` used `ipctool i2cset --bus 0 0x60 ...` from `rc.local`, twelve seconds into +boot, on a WiFi-only camera, to apply the sensor registers the whole change depends on. + +Flag a call in `general/overlay/` or `general/package/*/files/` to anything that is not +a busybox applet and not installed by a package the board's defconfig selects. Reach for +`ipcinfo` where the information is what is wanted, and for the vendor `load_` +script or the SDK driver where hardware must actually be programmed (§3.5). + +### 7.4 Restart majestic through its init script, not by hand + +`general/package/majestic/files/S95majestic` is not a thin wrapper. It re-reads `/etc/TZ` +so a restart picks up a zone change; it passes `-s`; it finds the daemon with +`start-stop-daemon -x` rather than a pidfile, because a stale pidfile makes `-S` start a +*second* majestic that dies on the busy sensor HAL; it waits up to ten seconds for the +old process to actually exit; and it starts the new one under `trap '' HUP`, because the +disposition survives `exec()` and a handler does not — the script's own comment records +25 out of 25 deaths without it, measured on a hi3516ev200. + +A hand-rolled `killall majestic; sleep 3; majestic &` re-opens every one of those, which +is what `#2446` shipped, on a hi3516ev200. + +Flag a shipped script that stops or starts majestic directly. `/etc/init.d/S95majestic +restart` does it correctly; `reload` is the SIGHUP path in §7.2. Lifecycle signalling by +other consumers — sysupgrade's SIGQUIT to make majestic release the SDK while staying +alive, or SIGTERM to stop it — is a different thing and is not in scope here. + --- ## 8. Things that must not reach `master` @@ -456,15 +653,23 @@ These are hard gates rather than judgement calls; `pr_compliance_checklist.yaml` enforces them. Summarised here because they are the most common review findings: - `LD_PRELOAD` in any shipped script, package file, or overlay. -- Binaries extracted from a camera's factory firmware, or any `.ko`/`.so`/`.bin` with - no vendor SDK or buildable source behind it. +- Binaries extracted from a camera's factory firmware, or anything the diff renders as + `Binary files ... differ`, with no vendor SDK or buildable source behind it — + extension and path do not matter, and `general/overlay/` is never the place for one. - New kernel patches under `general/package/all-patches/linux/` — those go to OpenIPC/linux. +- A new `*.patch` against a package whose `*_SITE` is an `openipc` repository — that + goes to the repository itself. - A `*_SITE` pointing at a personal fork, or a `*_VERSION` that is an abbreviated SHA. - A sensor, GPIO, I2C address, or other board-specific value written into `general/overlay/` or into a shared `load_` default. -- Single-board scripts and packages in the shared tree — those go to OpenIPC/builder. -- New sources that no `Config.in` selects and no defconfig builds. -- `insmod` where the tree uses `modprobe`, or an OpenSDK module not named `open_*`. +- Single-board scripts and packages in the shared tree — those go to OpenIPC/builder, + through the per-device seams in §1.4. +- New sources that no `Config.in` selects and no defconfig builds, and overlay files + that nothing on the camera reads. +- A shipped script calling a command the image does not contain — `ipctool` is the one + that looks installed and is not. +- `insmod` where the tree uses `modprobe`, an OpenSDK module not named `open_*`, or a + hand-rolled majestic restart in place of `/etc/init.d/S95majestic restart`. - A test plan whose boxes are unchecked, or a description stating the change was not tested on hardware. diff --git a/pr_compliance_checklist.yaml b/pr_compliance_checklist.yaml index 52b8b9e44e..a9114fa459 100644 --- a/pr_compliance_checklist.yaml +++ b/pr_compliance_checklist.yaml @@ -27,13 +27,25 @@ pr_compliances: cannot be fixed, and is not known to work on any board other than the one it came off. Firmware built that way cannot be supported. success_criteria: > - Any added .ko, .so, .a, .bin, or firmware image traces to a vendor SDK release or - to a source tree the build system compiles. Existing vendor blobs may be moved or - bumped. + Any added binary traces to a vendor SDK release or to a source tree the build + system compiles. Existing vendor blobs may be moved or bumped. failure_criteria: > The diff adds a binary extracted from factory firmware, a binary generated by a - script in the same PR, or any .ko/.so/.bin whose origin the PR cannot name. A + script in the same PR, or any binary whose origin the PR cannot name. A PROVENANCE-style file documenting the absence of source is itself a failure. + Judge this on the diff marker, never on the extension: a hunk rendered as + "Binary files a/... and b/... differ" IS a binary, whatever the file is called and + wherever it sits. .ko/.so/.a/.bin are the common spellings, not the definition — + #2446 added general/overlay/etc/ir/nrxset, an executable with no extension, and an + automated review reported it as "contains no textual changes", which is precisely + what a blob looks like to anything reading the diff as text. A binary anywhere + under general/overlay/ fails outright: the overlay is copied verbatim into every + image, so it ships to every camera of every vendor. + A register table copied out of a camera vendor's own driver is the same failure + wearing C: it cannot be checked against a datasheet, and the reason for any one + register is gone. #2446 replaced the SC2235 init sequence with a 114-entry "Dahua + DVP register table" for every Hi3516EV200 board using that sensor. The deltas a + board needs are reviewable; a wholesale swap sourced from a vendor image is not. - title: "Package SITE is an OpenIPC or upstream repository" compliance_label: true @@ -80,10 +92,24 @@ pr_compliances: script is expected and passes as long as it is additive — extending an existing case arm, adding a new .ini, or adding an entry to a package's sensor list, none of which change an existing board's behaviour. + Per-board values have their own seams, and naming the right one is part of the + finding: /usr/share/openipc/customizer.sh and /usr/share/openipc/muxes.sh (both run + by S30customizer), the declarative pin map /usr/share/openipc/gpio.conf, the + late-overlays.list symbol gate, and general/scripts/excludes/_.list. + All of them live per-device in OpenIPC/builder under devices//, at the same + paths. failure_criteria: > The diff writes a sensor name, I2C address, GPIO number, resolution, MAC prefix, or IP literal into general/overlay/, or changes an existing SNS_TYPE*, default resolution, or default profile in a shared *-osdrv-*/files/script/load_. + The additive exemption above is for load_ sensor case arms; it is not a + licence for the shared overlay. general/overlay/etc/wireless/usb already carries 46 + arms keyed on retail model names while 18 builder devices override the whole file — + those are grandfathered, and a new one fails like any other board-specific value. + An excludes list added HERE is also not per-board: the key is _, + which is the generic board's key too, so #2446's hi3516ev200_lite.list took + libsns_imx307.so and default.ini away from the generic hi3516ev200_lite image. This + repository ships no general/scripts/excludes/ directory; the lists belong in builder. - title: "Toolchain-wide flags are justified and measured" compliance_label: true @@ -143,6 +169,63 @@ pr_compliances: states the package must not be enabled on generic SoC defconfigs, or it collides with an existing first-class package at the same installed path. + - title: "No patches against OpenIPC's own packages" + compliance_label: true + objective: > + A patch in a package directory is the sanctioned way to carry a fix for code this + project cannot commit to. Against code it CAN commit to it is monkey-patching with + extra steps: invisible to anyone reading the SDK, silently dropped at the next + *_VERSION bump, and leaving the bug in place for every other consumer. + success_criteria: > + A new *.patch sits in a package whose *_SITE names a third-party upstream. Every + patch in the tree today does — ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, + libwebsockets, f2fs-tools, webrtc-audio-processing and the Realtek drivers. + A fix to OpenIPC-org code lands in that repository and arrives here as a + *_VERSION bump. + failure_criteria: > + The diff adds a *.patch to a package whose *_SITE resolves to a repository in the + openipc organisation. hisilicon-opensdk fetches openipc/openhisilicon, and #2446 + added 0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch against a file + checked in there. Redirect to that repository. Sensor and SDK work for HiSilicon + goes to OpenIPC/openhisilicon; Sigmastar sensor drivers go to OpenIPC/sensors. + + - title: "Shipped files are reachable from something that runs" + compliance_label: true + objective: > + general/overlay/ needs no Config.in and no .mk — anything dropped there ships — so + the dead-code gate above never fires on it. A file nothing opens still costs every + board of every vendor, and several sit within 32 KB of their squashfs cap. + success_criteria: > + Every added file under general/overlay/ or a package's files/ directory is read at + its installed path by a script, init file, package, or configuration somewhere in + firmware or builder. Documentation and license text shipped on purpose are fine. + failure_criteria: > + The diff adds an overlay file whose installed path appears nowhere else in the + tree. #2446 added /etc/ir/nrxset and /etc/ir/nrx_night_06.txt, which nothing reads + — and its own two comments contradict each other about whether they are used, one + claiming nrxset applies the VPSS 3DNR parameters and the other saying they were + baked into the IQ profile instead. "The vendor's firmware had it" is not a reason. + + - title: "A shipped script calls only commands the image contains" + compliance_label: true + objective: > + A camera has no package manager and frequently no network. A missing command does + not fail loudly — sh prints "not found" to a console nobody reads and continues to + the next line, so the script reports success having done nothing. + success_criteria: > + Commands invoked from general/overlay/ or general/package/*/files/ are busybox + applets, or binaries installed by a package the board's defconfig selects. + failure_criteria: > + The diff calls, from a shipped script, a command no package installs. ipctool is + the specific trap: general/package/ipctool/ipctool.mk installs ipcinfo and nothing + else, so BR2_PACKAGE_IPCTOOL=y puts no "ipctool" on the image. /usr/sbin/ipctool + is a symlink to extutils, which curls the tool from the ipctool repository's + "latest" release into /tmp on first use — an interactive convenience, and in a + boot script a network fetch during boot, from a floating tag, repeated every boot + because /tmp is tmpfs, and a silent no-op before the network is up. #2446 ran + `ipctool i2cset` from rc.local twelve seconds into boot on a WiFi-only camera. + Do not raise this for .github/ or contrib/, which run off-device. + - title: "No runtime patching of vendor code" compliance_label: true objective: > @@ -164,14 +247,22 @@ pr_compliances: naming for OpenSDK modules that replace vendor-prefixed blobs. Both are mechanical and checkable from the diff. success_criteria: > - Shell scripts that ship in the image load kernel modules with modprobe, and OpenSDK - modules are named open_* rather than with a vendor-specific prefix. This gate - applies only to files installed onto a camera — general/overlay/ and + Shell scripts that ship in the image load kernel modules with modprobe, OpenSDK + modules are named open_* rather than with a vendor-specific prefix, and a script + that needs to bounce the streamer calls `/etc/init.d/S95majestic restart`. This + gate applies only to files installed onto a camera — general/overlay/ and general/package/*/files/. Scripts under .github/ and contrib/ run on CI runners or a developer's machine under bash and are out of scope. failure_criteria: > The diff calls insmod where the surrounding tree uses modprobe, or introduces an OpenSDK module under a vendor-prefixed name such as gk7205v200_* or hi3516ev200_*. + Or it stops and starts majestic by hand instead of through S95majestic: #2446 + shipped `killall majestic; sleep 3; majestic &`, which drops the -s argument, + replaces start-stop-daemon -x with a pidfile-free killall, replaces a ten-second + wait for exit with a fixed sleep into the window where a second majestic dies on + the busy sensor HAL, and loses the `trap '' HUP` that S95majestic carries because + the disposition survives exec() where a handler does not — 25 deaths out of 25 + without it, measured on a hi3516ev200, which is the SoC that PR targets. Do not raise this for anything under .github/ or contrib/. Do not raise shell portability here: syntax is already enforced by .github/workflows/shell-tests.yml against busybox ash, and "avoid bashisms" is not binary on this target — the shipped From 9170160c335e37058e73bed390e399a7015d2411 Mon Sep 17 00:00:00 2001 From: AI Dev Date: Sat, 19 Sep 2026 06:57:47 +0000 Subject: [PATCH 2/3] review: two of the new rules would have generated false findings Both come from the bot review on the pull request that added them, and both were right. The patch rule claimed no patch in the tree targets an OpenIPC-org repository. That is false: libevent-openipc carries 0001-CMakeLists-remove-epoll_pwait2-check.patch against github.com/OpenIPC/libevent. My own check missed it because the SITE grep matched LIBEVENT_OPENIPC_SITE_METHOD first and took that line. Counted properly it is 22 of 23, not 23 of 23 -- and the one exception is the legitimate shape the rule should have described from the start: one hunk, a build fix, obviously temporary, riding ahead of a bump. So the gate no longer refuses every such patch. It asks whether the patch is a bridge or a substitute -- is the upstream pull request named, is it the minimal delta, does it go at the next *_VERSION bump -- which still catches #2446, whose 114-entry table replacement is none of those. The reachability rule was worse than the bot said. It asked that an added file be "read at its installed path", but most of this tree's data is never named by anyone: hisilicon-osdrv-hi3516ev200 installs files/sensor/config/*.ini by wildcard and the one that gets used is chosen at runtime from the configured sensor, a module is reached by `modprobe `, an IQ profile through isp.iqProfile. A new sensor ini would have failed the gate while working perfectly. It also contradicted a gate five entries above it in the same file, which already exempts a data file landing under a path an existing install rule globs. Both now say the same thing: trace the mechanism -- path, basename, a name built at runtime, or an install glob a convention selects from -- and raise only the residue, which is what /etc/ir/ actually is. A rule that fires on correct work costs more than the rule is worth, and best_practices.md already carries one warning about a false finding this project has seen made. These two would have been the next. --- .pr_agent.toml | 23 +++++++++---- CLAUDE.md | 11 +++--- best_practices.md | 67 ++++++++++++++++++++++++------------ pr_compliance_checklist.yaml | 54 ++++++++++++++++++++--------- 4 files changed, 105 insertions(+), 50 deletions(-) diff --git a/.pr_agent.toml b/.pr_agent.toml index 3fcb2493c0..8ebc44f602 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -86,11 +86,15 @@ Prioritise, in order: provenance failure written in C: unverifiable against a datasheet, and replacing a whole table rather than adding the deltas a board needs changes every other board using that sensor. - A new *.patch is fine against a third-party upstream (every patch in the tree today is - - ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, libwebsockets, f2fs-tools, the - Realtek drivers) and is a finding when the package's *_SITE resolves to an openipc - repository, because there the fix has somewhere better to go and a *_VERSION bump - silently drops it. + A new *.patch is fine against a third-party upstream (22 of the tree's 23 patched + packages - ffmpeg, mbedtls, vtund, libre, baresip, siproxd, zerotier, libwebsockets, + f2fs-tools, mini-snmpd, uacme, nabto, mavlink-router, the Realtek drivers). Against a + package whose *_SITE resolves to an openipc repository it is a finding ONLY when it + substitutes for the upstream fix rather than bridging to it: no pull request named + against the owning repo, or a wholesale replacement rather than a minimal delta, or + nothing saying it goes at the next *_VERSION bump. libevent-openipc is the tree's one + legitimate instance - a single hunk dropping a build-time feature check - so do not + report this as an absolute prohibition. 4. Changes that belong to a different repository. Kernel code and kernel patches belong to OpenIPC/linux. Support for one retail camera model belongs to OpenIPC/builder, under @@ -150,8 +154,13 @@ Prioritise, in order: The overlay's mirror image of this needs saying separately, because general/overlay/ needs no Config.in and no .mk - anything dropped there ships, so a file nothing on the camera ever opens passes every dead-code check while costing every board of every - vendor, several of which sit within 32 KB of their squashfs cap. Grep the installed - path of an added overlay file; if nothing reads it, that is a finding. + vendor, several of which sit within 32 KB of their squashfs cap. But reachability is + SEMANTIC: a sensor .ini installed by a files/sensor/config/*.ini wildcard and chosen + at runtime from the configured sensor name is reachable, as is a module loaded by + `modprobe ` and an IQ profile named through isp.iqProfile. Trace the mechanism + before raising anything - path, basename, a name built at runtime, or an install glob + a documented convention selects from all count. The finding is the residue: a file + whose name appears nowhere, in a directory no consumer knows about. 8. Commands a shipped script calls must exist on the image. A camera has no package manager and often no network, and a missing command does not fail loudly - sh prints diff --git a/CLAUDE.md b/CLAUDE.md index d516dc0210..ffb1ce2ced 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -154,11 +154,14 @@ Summarised from `pr_compliance_checklist.yaml`; the reasoning is in `best_practi extension; an executable with no extension is still a blob, and `general/overlay/` is never the place for one. A register table copied out of a camera vendor's driver is the same problem written in C. -- **No patches against OpenIPC's own packages.** Every `*.patch` in the tree targets a third-party - upstream. If a package's `*_SITE` is an `openipc` repository, the fix is a pull request there - and a `*_VERSION` bump here. +- **A patch against an OpenIPC-owned package is a bridge, not a fix.** 22 of the tree's 23 patched + packages target third-party upstreams. If a package's `*_SITE` is an `openipc` repository the fix + belongs there; a patch here passes only when the pull request against the owning repository is + named, the patch is the minimal delta, and it goes at the next `*_VERSION` bump — + `libevent-openipc` is the one instance and the model. - **Nothing ships that nothing runs.** An overlay file needs no `Config.in`, so nothing catches a - file the camera never opens. Grep the installed path before adding one. + file the camera never opens. Trace the consumer before adding one — reachability is semantic, so + a basename, a name built at runtime, or an install glob a convention selects from all count. - **Shipped scripts call only what the image contains.** Busybox applets, or binaries a package in the board's defconfig installs. `ipctool` looks installed and is not: the package ships `ipcinfo`, and `/usr/sbin/ipctool` is an `extutils` arm that downloads the tool at first use. diff --git a/best_practices.md b/best_practices.md index 3a5a0ddebe..99a9a0c30f 100644 --- a/best_practices.md +++ b/best_practices.md @@ -383,24 +383,34 @@ build system generates it; if the build system cannot, the change needs the real ### 4.4 A patch against an OpenIPC package is a pull request to that repository -Patches in a package directory are normal here, and every one of them targets code -this project cannot commit to: ffmpeg, mbedTLS, vtund, baresip's libre, siproxd, -ZeroTier, libwebsockets, f2fs-tools, and the Realtek WiFi drivers. Not one patches a -repository under the OpenIPC organisation, because for those the fix has somewhere -better to go. - -A downstream patch against our own code is monkey-patching with extra steps. It is -invisible to anyone reading the SDK, it is silently dropped the moment someone bumps -`*_VERSION`, and every other consumer of that repository keeps the bug. - -`#2446` added `general/package/hisilicon-opensdk/0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch`. -`HISILICON_OPENSDK_SITE` is `$(call github,openipc,openhisilicon,...)`, and the file it -patches is checked in there. - -Flag a new `*.patch` in a package whose `*_SITE` resolves to an `openipc` repository. -Redirect to that repository; once it lands, bump `*_VERSION` here and the patch is not -needed. Where the change must ride ahead of the bump, say so explicitly and keep the -patch to the delta, not a wholesale replacement (§2.5). +Patches in a package directory are normal here. Twenty-two of the twenty-three +patched packages target code this project cannot commit to — ffmpeg, mbedTLS, vtund, +baresip and its libre, siproxd, ZeroTier, libwebsockets, f2fs-tools, mini-snmpd, uacme, +nabto, mavlink-router, onvif-simple-server, gst-plugins-bad, and the Realtek WiFi +drivers — because for those a downstream patch is the only route there is. + +A patch against code the project *does* own is different. It is invisible to anyone +reading that repository, it is dropped the moment someone bumps `*_VERSION`, and every +other consumer of the code keeps the bug. So the default is a pull request to the owning +repository, and a `*_VERSION` bump here once it lands. + +**The default has one legitimate exception, and the tree contains exactly one instance +of it.** `libevent-openipc` carries `0001-CMakeLists-remove-epoll_pwait2-check.patch` +against `https://github.com/OpenIPC/libevent`. That is the shape the exception should +take: one hunk, a build fix, obviously temporary, riding ahead of a bump. Do not read +this section as "never" — read it as "not instead of the pull request". + +`#2446` is the other shape. +`general/package/hisilicon-opensdk/0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch` +rewrites a 114-entry sensor init table for every Hi3516EV200 board using that sensor. +`HISILICON_OPENSDK_SITE` is `$(call github,openipc,openhisilicon,...)`, the file it +patches is checked in there, and nothing about it is temporary. + +So the question to ask of a new `*.patch` in a package whose `*_SITE` resolves to an +`openipc` repository is not whether it exists but whether it is a bridge: does the PR +name the pull request opened against the owning repository, is the patch the minimal +delta rather than a wholesale replacement (§2.5), and will it be deleted at the next +bump? Three yeses and it is the libevent case. Any no and it belongs upstream first. --- @@ -527,10 +537,23 @@ from them and not from the IQ profile, the IQ profile says the opposite and that parameters were baked into it instead. Both cannot be true, and either way one of the two is dead. -Grep the tree for the installed path of any added overlay file. If nothing reads it, -ask what does; "the vendor's firmware had it" is not an answer. Where a comment asserts -a consumer, check that the consumer is actually called — a stale comment is how a file -keeps looking justified. +**Reachability is semantic, not a literal path match, and getting that wrong turns this +rule into a false-finding generator.** Most of the tree's data files are never named by +a consumer. `hisilicon-osdrv-hi3516ev200.mk` installs `files/sensor/config/*.ini` into +`/etc/sensors/` by wildcard, and the file that gets used is chosen at runtime from the +configured sensor name; a kernel module is reached by `modprobe `, not by path; an +IQ profile is named through `isp.iqProfile`. All of those are reachable. The compliance +checklist already says as much under "New sources are wired into the build" — a data +file landing under a path an existing install rule globs needs no `.mk` change and +passes — and this rule must not contradict it. + +So trace the mechanism before raising anything. A file is reachable if something names +its path, or its basename, or constructs its name at runtime, or picks it up through an +install glob that a documented convention then selects from. The finding is for the +residue: a file whose name appears nowhere, that no convention selects, and that sits in +a directory no consumer knows about — `/etc/ir/` being the case in hand. Where a comment +asserts a consumer, check that the consumer is actually called; a stale comment is how a +dead file keeps looking justified. "The vendor's firmware had it" is not an answer. --- diff --git a/pr_compliance_checklist.yaml b/pr_compliance_checklist.yaml index a9114fa459..b75eca5fa7 100644 --- a/pr_compliance_checklist.yaml +++ b/pr_compliance_checklist.yaml @@ -177,17 +177,26 @@ pr_compliances: extra steps: invisible to anyone reading the SDK, silently dropped at the next *_VERSION bump, and leaving the bug in place for every other consumer. success_criteria: > - A new *.patch sits in a package whose *_SITE names a third-party upstream. Every - patch in the tree today does — ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, - libwebsockets, f2fs-tools, webrtc-audio-processing and the Realtek drivers. - A fix to OpenIPC-org code lands in that repository and arrives here as a - *_VERSION bump. + A new *.patch sits in a package whose *_SITE names a third-party upstream — + 22 of the tree's 23 patched packages do, among them ffmpeg, mbedtls, vtund, libre, + baresip, siproxd, zerotier, libwebsockets, f2fs-tools, mini-snmpd, uacme, nabto, + mavlink-router, onvif-simple-server and the Realtek drivers. + A patch against OpenIPC-org code passes when it is a NAMED BRIDGE rather than a + substitute for the upstream fix: the PR cites the pull request opened against the + owning repository, the patch is the minimal delta, and it goes away at the next + *_VERSION bump. libevent-openipc is the tree's one instance and the model for it — + 0001-CMakeLists-remove-epoll_pwait2-check.patch against github.com/OpenIPC/libevent + is a single hunk removing one build-time feature check. failure_criteria: > The diff adds a *.patch to a package whose *_SITE resolves to a repository in the - openipc organisation. hisilicon-opensdk fetches openipc/openhisilicon, and #2446 - added 0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch against a file - checked in there. Redirect to that repository. Sensor and SDK work for HiSilicon - goes to OpenIPC/openhisilicon; Sigmastar sensor drivers go to OpenIPC/sensors. + openipc organisation AND does not meet the bridge conditions above — no upstream + pull request named, or a wholesale replacement rather than a delta, or nothing + saying it is temporary. #2446 is the case: hisilicon-opensdk fetches + openipc/openhisilicon, and 0001-sc2235-replace-init-table-with-dahua-dvp-sequence.patch + rewrites a 114-entry sensor init table, permanently, for every Hi3516EV200 board + using that sensor. Redirect to the owning repository. Sensor and SDK work for + HiSilicon goes to OpenIPC/openhisilicon; Sigmastar sensor drivers go to + OpenIPC/sensors. Do not raise this for an existing patch the diff merely touches. - title: "Shipped files are reachable from something that runs" compliance_label: true @@ -196,15 +205,26 @@ pr_compliances: the dead-code gate above never fires on it. A file nothing opens still costs every board of every vendor, and several sit within 32 KB of their squashfs cap. success_criteria: > - Every added file under general/overlay/ or a package's files/ directory is read at - its installed path by a script, init file, package, or configuration somewhere in - firmware or builder. Documentation and license text shipped on purpose are fine. + Every added file under general/overlay/ or a package's files/ directory can be + traced to a consumer. Reachability is SEMANTIC, not a literal path match, and most + of this tree's data files are never named by anyone: a sensor .ini is installed by + the wildcard files/sensor/config/*.ini and selected at runtime from the configured + sensor name, a kernel module is reached by `modprobe ` and not by path, an IQ + profile is named through isp.iqProfile. A file passes if something names its path, + its basename, a name built at runtime, or an install glob that a documented + convention then selects from. This is the same allowance the "New sources are wired + into the build" gate already makes for a data file landing under a path an existing + install rule globs, and the two must not contradict each other. Documentation and + license text shipped on purpose are fine. failure_criteria: > - The diff adds an overlay file whose installed path appears nowhere else in the - tree. #2446 added /etc/ir/nrxset and /etc/ir/nrx_night_06.txt, which nothing reads - — and its own two comments contradict each other about whether they are used, one - claiming nrxset applies the VPSS 3DNR parameters and the other saying they were - baked into the IQ profile instead. "The vendor's firmware had it" is not a reason. + The diff adds a file whose name appears nowhere, that no runtime convention + selects, and that sits in a directory no consumer knows about. #2446 is the case: + /etc/ir/nrxset and /etc/ir/nrx_night_06.txt, in a directory nothing in firmware or + builder reads, and the rc.local in the same PR never invokes nrxset although its + comment says it does — while the IQ profile it ships says the opposite, that the + parameters were baked into the profile instead. Trace the mechanism before raising + this; an asset reached by basename or convention is reachable, and a finding that + says otherwise is a false one. "The vendor's firmware had it" is not a reason. - title: "A shipped script calls only commands the image contains" compliance_label: true From 49d2b082d0ef1008b1e420d796673b0cd6f4c43d Mon Sep 17 00:00:00 2001 From: AI Dev Date: Sat, 19 Sep 2026 06:59:21 +0000 Subject: [PATCH 3/3] review: the patch gate's title said the thing the gate no longer means The rule now asks whether a patch against an OpenIPC-owned package is a bridge to an upstream pull request or a substitute for one, but it was still titled "No patches against OpenIPC's own packages". A title is what gets quoted back in a finding, so on a legitimate libevent-shaped patch this one would have delivered exactly the message the rule was just corrected to stop delivering. --- best_practices.md | 6 +++--- pr_compliance_checklist.yaml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/best_practices.md b/best_practices.md index 99a9a0c30f..82b379c9e2 100644 --- a/best_practices.md +++ b/best_practices.md @@ -381,7 +381,7 @@ reviewable — nobody rebuilds it, and the committed `.so` is what actually ship Flag any committed binary produced by a script in the same PR. If it is generated, the build system generates it; if the build system cannot, the change needs the real SDK. -### 4.4 A patch against an OpenIPC package is a pull request to that repository +### 4.4 A patch against an OpenIPC package is a bridge, not a substitute Patches in a package directory are normal here. Twenty-two of the twenty-three patched packages target code this project cannot commit to — ffmpeg, mbedTLS, vtund, @@ -681,8 +681,8 @@ enforces them. Summarised here because they are the most common review findings: extension and path do not matter, and `general/overlay/` is never the place for one. - New kernel patches under `general/package/all-patches/linux/` — those go to OpenIPC/linux. -- A new `*.patch` against a package whose `*_SITE` is an `openipc` repository — that - goes to the repository itself. +- A new `*.patch` against a package whose `*_SITE` is an `openipc` repository, unless it + is a named, minimal, temporary bridge to a pull request already open there (§4.4). - A `*_SITE` pointing at a personal fork, or a `*_VERSION` that is an abbreviated SHA. - A sensor, GPIO, I2C address, or other board-specific value written into `general/overlay/` or into a shared `load_` default. diff --git a/pr_compliance_checklist.yaml b/pr_compliance_checklist.yaml index b75eca5fa7..e6f9dcec85 100644 --- a/pr_compliance_checklist.yaml +++ b/pr_compliance_checklist.yaml @@ -169,7 +169,7 @@ pr_compliances: states the package must not be enabled on generic SoC defconfigs, or it collides with an existing first-class package at the same installed path. - - title: "No patches against OpenIPC's own packages" + - title: "A patch against an OpenIPC package is a bridge, not a substitute" compliance_label: true objective: > A patch in a package directory is the sanctioned way to carry a fix for code this