batctl: add new package - #30163
Conversation
|
@simonwunderlich is the maintainer, not me. (see makefile) |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. The packaging itself looks sound: PKG_BUILD_DIR is variant-scoped, the config-n list is a superset of every option referenced by config-settings/config-tables/config-json/config-tools/config-extratools (so nothing is left at its upstream default), gc-sections/lto are valid PKG_BUILD_FLAGS, and BuildPackage is called with literal names rather than $(PKG_NAME).
None of the three inline findings block a merge — two are cosmetic and the third is a CI-harness question to confirm once the test build reports on this head. Note I could not diff this against the routing feed's copy (only openwrt/packages is reachable from this session), so the "content matches the current routing feed master" claim in the PR body is unverified here.
Generated by Claude Code
| endef | ||
|
|
||
| define Package/batctl/description | ||
| batctl is a more intuitive managment utility for B.A.T.M.A.N.-Advanced. |
There was a problem hiding this comment.
nit: typo — "managment" should be "management". This string is user-visible in the package description of all three variants.
| batctl is a more intuitive managment utility for B.A.T.M.A.N.-Advanced. | |
| batctl is a more intuitive management utility for B.A.T.M.A.N.-Advanced. |
Generated by Claude Code
| LIBNL_NAME="libnl-tiny" \ | ||
| LIBNL_GENL_NAME="libnl-tiny" |
There was a problem hiding this comment.
Optional/cosmetic: these continuation lines are indented with 8 spaces, while every other multi-line list in this new file (config-n, config-settings, config-tables, config-json, config-tools, config-extratools, config-y) uses a single tab. Nothing breaks — these are plain variable assignments, not recipes — but it is an inconsistency inside a file being added fresh.
| LIBNL_NAME="libnl-tiny" \ | |
| LIBNL_GENL_NAME="libnl-tiny" | |
| LIBNL_NAME="libnl-tiny" \ | |
| LIBNL_GENL_NAME="libnl-tiny" |
Same applies to line 79 (the REVISION= continuation under MAKE_FLAGS +=).
Generated by Claude Code
| LIBNL_GENL_NAME="libnl-tiny" | ||
|
|
||
| MAKE_FLAGS += \ | ||
| REVISION="$(PKG_VERSION)-openwrt-$(PKG_RELEASE)" |
There was a problem hiding this comment.
This PR adds only the Makefile, with no test.sh / test-version.sh. This repo's CI forces a generic runtime version check on package executables (run with version/help flags, output expected to contain PKG_VERSION), which the routing feed did not apply — so a Makefile that was fine there can still trip the check here.
Concretely: REVISION here expands to 2026.2-openwrt-1, so batctl's version banner does contain PKG_VERSION, but batctl prints it via -v, and its usage output (-h) does not carry the version. If the generic check's flag set doesn't cover -v, all three variants will fail it. Since CI hasn't reported on this head yet, worth confirming against the actual run rather than taking my word for it — but if it does fail, a small net/batctl/test.sh in the same shape as net/ampr-ripd/test.sh is the fix:
#!/bin/sh
"$1" -v 2>&1 | grep "$PKG_VERSION"(Note grep without -q, per the feed's testing guidance, so the matched version line shows up in the CI log.)
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (1649fac, 49784c1). Nothing here blocks a merge — the only new remark is a stale sentence in the PR body.
I re-derived both commit claims rather than taking them at face value:
1649fac—PKG_BUILD_DIR ?=in include/package.mk:11 expands to exactly the dropped override wheneverBUILD_VARIANTis set, and the override sat before theinclude, so?=honoured it. Removal is a no-op for all three variants (and incidentally drops a straybatctl-/path component on theBUILD_VARIANT-empty metadata pass). Message matches the diff.49784c1— I expanded the oldConfigVars/shexportstring and the newpatsubstpair side by side under GNU make for each variant; the resulting assignment lists are identical, same order: 66 (tiny), 95 (default), 96 (full), so the "96 assignments" in the message checks out. Last-command-line-assignment-wins also verified — that ordering is load-bearing here, sinceconfig-nand the fullconfig-yare the same 48 options, so every single switch depends on the n-then-y sequence.
Still open from my previous review (unchanged on this head): the managment typo in Package/batctl/description, the 8-space continuation lines under MAKE_VARS/MAKE_FLAGS (cosmetic), and the test.sh / generic version-check question. CI on this head is still queued, so I could not confirm the last one against an actual run.
Generated by Claude Code
| MAKE_FLAGS += \ | ||
| $(patsubst %,CONFIG_%=n,$(config-n)) \ | ||
| $(patsubst %,CONFIG_%=y,$(config-y)) |
There was a problem hiding this comment.
nit: the PR body still says "The content matches the current routing feed master", but this hunk (49784c1) and the PKG_BUILD_DIR removal (1649fac) deliberately deviate from it — the routing feed copy still carries the ConfigVars/shexport indirection and the explicit PKG_BUILD_DIR. Worth updating the body so the reviewer of the coordinated removal PR in openwrt/routing knows the move is verbatim only in the first commit, with two cleanups on top.
(The refactor itself checks out: I expanded the old $(call ConfigVars,n)$(call ConfigVars,y) string and the new patsubst pair side by side with GNU make for all three variants and got identical word lists in identical order — 66 / 95 / 96 assignments for tiny / default / full.)
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (f79a31a); no new issues found. Its message matches the diff — both hunks are exactly the typo and the indentation it describes, and nothing else rides along.
Verified on this head:
managment→managementis the only occurrence in the file; no stragglers remain.- The
MAKE_VARS/MAKE_FLAGScontinuation lines are now single-tab, matching theconfig-*lists and thepatsubstblock at the bottom. The remaining space-indented lines are all insidedefine Package/…metadata blocks, where two spaces is the correct convention — so the file is now internally consistent rather than uniformly tabbed, which is the right end state. No trailing whitespace anywhere.
Withdrawing my earlier test.sh / generic-version-check question. CI has now reported on this head and the generic check passes — no test-version.sh override is needed. From the x86_64 job:
batctl-tiny: Use generic tests
batctl 2026.2-openwrt-1 [batman-adv: module version unknown]
batctl-tiny: [pass] Version check (/usr/libexec/batctl-tiny)
batctl-tiny: Generic tests passed
The harness reaches the version banner on its own, so my concern about -v not being in the generic flag set was unfounded. Nine of the ten build targets are green, powerpc_464fp still running.
Still open (existing thread, not repeated inline): the PR body's "The content matches the current routing feed master" is now three commits stale rather than two — f79a31a is a further deliberate deviation from the feed copy. The layering itself is good practice, and I'd keep it: a verbatim move in f945ae1 with cleanups stacked on top is far easier to audit against the feed than one squashed commit, so I am explicitly not asking for a squash of the typo fix into the initial commit. It just wants a sentence in the body saying so, for whoever reviews the coordinated removal PR in openwrt/routing.
One caveat carried over from my previous reviews: only openwrt/packages is reachable from this session, so I still cannot diff the initial commit against the routing feed's copy. The "matches routing feed master" claim, and consequently whether the managment typo originated there or was introduced here, remains unverified on my side — worth a maintainer eyeball since it is the one load-bearing claim in this PR I have no way to check.
Generated by Claude Code
The package has been moved to the openwrt/packages feed, as discussed in openwrt#184. See openwrt/packages#30163. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push. The four commits were rewritten (f945ae1→2bc7999, 1649fac→3bf2bba, 49784c1→10118bb, f79a31a→4675f3b) but the base is unchanged (ec4ca5c) and the tree delta against my last review is exactly one line: SUBMENU:=Wireless, amended into the first commit. I diffed each old/new commit pair individually to confirm nothing else rode along — commits 2–4 are byte-identical apart from that inherited line, so my earlier verification of them still stands and I have not re-litigated it here.
Nothing blocks a merge. The single inline remark is a question about which submenu batctl belongs in, not an assertion that the current one is wrong.
CI is fully green on this head now: all ten build targets plus the three FormalityCheck jobs passed, so the generic version check I withdrew last time is confirmed good on the rebased series as well.
Commit checks
2bc7999"batctl: add new package" — the body presents this commit as the routing feed's copy moved across unchanged, but it now also introducesSUBMENU:=Wireless, which is not part of the move and is not mentioned anywhere in the message. The three follow-up commits each document their one deviation from the feed copy, which is what made this series easy to audit; this one currently hides a deviation inside the "verbatim" commit. Either stack it as a fifth commit or add a sentence to the body. The PR body's "The content matches the current routing feed master" is stale on this count too.
Same caveat as my previous reviews: only openwrt/packages is reachable from this session, so I still cannot diff the initial commit against the routing feed's copy. Whether the feed's batctl already carries a SUBMENU (and if so, which one) is the one thing here I have no way to check — worth a maintainer eyeball before the coordinated removal PR lands.
Generated by Claude Code
| define Package/batctl/Default | ||
| SECTION:=net | ||
| CATEGORY:=Network | ||
| SUBMENU:=Wireless |
There was a problem hiding this comment.
This is the only content change since my last review (f79a31a), and it went in as an amend to the initial "move from the routing feed" commit rather than as a fifth commit on top of it.
On the placement itself: in-tree, Wireless collects radio/PHY-level tooling — net/wavemon/Makefile:32, plus aircrack-ng, hcxtools, hcxdumptool, pixiewps, reaver, dawn, wifischedule. Routing daemons go under Routing and Redirection — net/frr/Makefile:75, net/yggdrasil/Makefile:33, smcroute, bfdd, mwan3. batctl configures and debugs batman-adv, a layer-2 mesh routing protocol that runs over any interface including wired ones, so Routing and Redirection reads like the closer fit to me.
Is Wireless deliberate — e.g. to keep batctl next to where kmod-batman-adv is expected to land when the rest of the routing feed moves across? If so it's worth a line in the commit message, since nothing in the diff explains the choice.
Generated by Claude Code
There was a problem hiding this comment.
I was going to mention that as well, I run batman-adv over both wireless and wired links. I wouldn't necessarily go to look for it under the wireless submenu.
There was a problem hiding this comment.
Fwiw, if you want to move batctl and alfred into a submenu, Routing and Redirection seems more appropriate to me. It has survived for a long time in the general Network menu, so it would be reasonable to leave it there too, imo. batman-adv has always been a bit weird from a user perspective because the basic support comes from a kernel module which lives in a different menu altogether, "Kernel modules" / "Network support".
There was a problem hiding this comment.
@DamisDorgon
batctl and alfred are without doubt mesh orientated, but I don't see how they fit into Routing and Redirection.
There are mesh packages scattered all over, a submenu indicating "Mesh" to bring them all together makes a lot of sense, at least in my opinion.
Although a new SECTION would be more useful as it would be searchable by the apk package manager...
There was a problem hiding this comment.
^^ potentially searchable by apk package manager....
There was a problem hiding this comment.
@BKPepe I'm indifferent to this. Yes, batman-adv/alfred/batctl can be used over wired links - and some people do that, e.g. Freifunk communities also use Ethernet and L2-VPNs with batman-adv on top. Practically, though, batman-adv is pretty much always used on top of wireless routers for wireless applications (I've seen some niche counter examples though). Technically, it's mostly independent of the wireless stack, practically it's typically used together. I'm used to having it in the network menu (where it was for years), but wireless would be okay too. Routing/Redirection is something I read as layer 3, while batman-adv is layer 2 only, so I'd lean more to wireless if you want to move it at all.
batctl is the user space configuration and debugging tool for the B.A.T.M.A.N. Advanced kernel module. It is packaged in three variants (tiny, default, full) with different sets of subcommands. Moved from the openwrt/routing feed, as discussed in openwrt/routing#184. The one deviation from the feed copy is SUBMENU:=Wireless, which the feed copy does not set at all: without it the three variants sit ungrouped at the top of the Network menu. Wireless keeps them next to mesh11sd and alfred, which move across with the same value. A dedicated Mesh submenu is the better home for all three and is worth adding once enough of the mesh packages have landed to justify it. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
PKG_BUILD_DIR was set to exactly the value package.mk derives by default for a package with build variants, so the override had no effect. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The CONFIG_* switches were assembled into a newline separated string
and smuggled to the build through a shell variable via shexport, so
the Makefile read:
MAKE_FLAGS += $$$$$(call shvar,batctl_config)
They can simply be listed on the make command line instead. The
result is identical, since later assignments on the command line
override earlier ones, exactly as the "n first, then y" order of the
old string relied on. The longest variant expands to 96 assignments,
about 2 KB, so there is no length concern that would justify the
indirection.
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
"managment" is user visible in the description of all three variants. The MAKE_VARS and MAKE_FLAGS continuation lines were indented with spaces while every other multi-line list in the file uses a tab. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 4 rewritten commits; no new issues found.
The force-push rewrote all four SHAs (2bc7999→7407e00, 3bf2bba→3a4134b, 10118bb→c5a852b, 4675f3b→67111dd), but git diff 4675f3b..67111dd is empty — the tree is byte-identical to my last review. I confirmed per-pair that this is a message-only rewrite: each old/new pair has the same git patch-id, and the only textual difference across the four messages is the new paragraph in 7407e00 documenting SUBMENU:=Wireless. Commits 2–4 are unchanged in both content and message, so my earlier verification of them stands and I have not re-litigated it.
That paragraph closes the one finding from my previous review — the initial commit no longer presents itself as a verbatim move while silently carrying a deviation. The two factual claims it makes are checkable from this repo and both hold:
- alfred —
net/alfred/Makefile:33in #30161 setsSUBMENU:=Wireless. - mesh11sd —
net/mesh11sd/Makefile:22in #30168 setsSUBMENU:=Wireless.
So the three mesh packages do land in the same submenu, as the message says. The PR body carries the same paragraph now, so the "content matches the current routing feed master" sentence is no longer stale — it is explicitly qualified by the deviation list.
I am withdrawing my Routing and Redirection question from the previous review. My ask there was for the rationale to appear in the commit message rather than only in the diff, and it now does; the "keep the mesh packages together, revisit with a dedicated Mesh submenu once more have landed" reasoning is a legitimate call for the maintainer to make, and consistent placement across the three moves is worth more than my read of which existing submenu is the better taxonomic fit.
CI is fully green on this head: all ten build targets plus the three FormalityCheck jobs passed.
One caveat carried over unchanged from every previous review: only openwrt/packages is reachable from this session (openwrt/routing is explicitly blocked), so I still cannot diff the initial commit against the routing feed's copy. Whether the feed's batctl already carries a SUBMENU, and whether the managment typo fixed in 67111dd originated there, remain the two things here I have no way to check — worth a maintainer eyeball before the coordinated removal PR lands in openwrt/routing.
Generated by Claude Code
Adds batctl from the openwrt/routing feed — the routing packages are being moved into openwrt/packages one by one, as discussed in openwrt/routing#184.
The content matches the current routing feed master, with one deliberate deviation:
SUBMENU:=Wireless, which the feed copy does not set at all. Without it the three variants sit ungrouped at the top of the Network menu. Wireless keeps them next to mesh11sd (#30168) and alfred (#30161), which move across with the same value. A dedicated Mesh submenu is the better home for all three and is worth adding once enough of the mesh packages have landed to justify it.Once this is merged, the package will be removed from the routing feed (a coordinated removal PR is prepared there).
Maintainer: @simonwunderlich