luci-app-qos: mark the old app more clearly as ancient - #8904
Conversation
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. The commit message matches the diff; qos-scripts is indeed still the iptables-based package in openwrt/openwrt (package/network/config/qos-scripts) and both luci-app-sqm and luci-app-qosify exist in this tree, so the new wording is factually accurate. One nit inline about the reach of the changed strings.
Generated by Claude Code
| LUCI_TITLE:=Configuration for the ancient qos-scripts package | ||
| LUCI_DESCRIPTION:=Configuration for the ancient qos-scripts package. (Note: SQM or qosify are more modern QoS packages) |
There was a problem hiding this comment.
nit: LUCI_TITLE/LUCI_DESCRIPTION only reach package-selection surfaces (menuconfig, apk/opkg metadata, the LuCI Software page) — they are not used anywhere in the web UI. A user who already has the app installed and lands on the page still sees the neutral form.Map('qos', _('Quality of Service'), …) header at qos.js:24-25 and the plain "title": "QoS" menu entry, with no hint that this is the superseded tool.
Since the linked forum thread is about confusion in the running UI rather than at install time, was leaving the view text untouched intentional (e.g. to avoid churning the 40+ po/ catalogues), or would you also want the note in the form.Map description?
Generated by Claude Code
|
I wonder if this package should retain its prominent name, given that it essentially is defunct. Maybe we should rename to luci-app-qos-legacy, and assign luci-app-qos as PROVIDES to qosify. |
|
Agree. I can modify the commit. And the actual qos-scripts in the main OpenWrt repo should similarly be somehow marked deprecated/defunct. I think that it is still iptables based, so not really for firewall4 etc. |
a038375 to
0939daf
Compare
|
@jow- Thinking about this, and it might actually be easier in the long run to just delete this old defunct package, and just set the PROVIDES for luci-app-qosify. Would that be ok? |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits (the branch was force-pushed; the previously reviewed 9cf83d2 is gone and the rename from jow-'s suggestion replaces it).
Both commit messages match their diffs. The rename itself looks complete and self-consistent: Makefile, menu.d, acl.d, ucitrack, the view and all 44 po/ catalogues moved to the qos-legacy basename, the .pot/.po source references and obsoleted QoS / Quality of Service msgids were regenerated correctly, LUCI_BASENAME resolves to qos-legacy so the generated luci-i18n-qos-legacy-* packages line up with the new po/*/qos-legacy.po names, and no reference to the old luci-app-qos path remains anywhere in this tree (nor in openwrt/openwrt or the packages feed).
One inline issue on the PROVIDES commit — the variable used there is not the one luci.mk consumes.
Generated by Claude Code
| LUCI_DESCRIPTION:=Web UI for the qosify CAKE/eBPF traffic shaping daemon | ||
| LUCI_DEPENDS:=+qosify | ||
| LUCI_PKGARCH:=all | ||
| PROVIDES:=luci-app-qos |
There was a problem hiding this comment.
A bare PROVIDES:= at the top level of a LuCI package Makefile has no effect — the emitted package will not provide luci-app-qos, so the migration path this commit intends does not happen.
luci.mk generates the Package/$(PKG_NAME) block itself and only emits a PROVIDES: field from PKG_PROVIDES: $(if $(PKG_PROVIDES),PROVIDES:=$(PKG_PROVIDES)) at luci.mk:168. BuildPackage evaluates $(Package/Default) then $(Package/$(1)) (package.mk:324-325), and Package/Default contains a bare PROVIDES:= (package-defaults.mk:20) that clears whatever was assigned earlier in the Makefile.
In-tree precedent uses PKG_PROVIDES, e.g. applications/luci-app-openlist/Makefile:13 and applications/luci-app-package-manager/Makefile:16.
| PROVIDES:=luci-app-qos | |
| PKG_PROVIDES:=luci-app-qos |
(Unrelated to this PR, but applications/luci-app-radicale3/Makefile:11 has the same latent bug with PROVIDES:=luci-app-radicale2.)
Generated by Claude Code
There was a problem hiding this comment.
Hm... I think this should be implemented in our webhook tool.
|
I am positively surprised by the quality of the feedback from the AI. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 2 new commits; no new issues found.
47285a9 was amended and d37cbcd added since the last review; the only change against ee772a9 is PROVIDES:= → PKG_PROVIDES:= in the two Makefiles, which resolves the issue from the previous round. Both are now correct: PKG_PROVIDES is set before include ../../luci.mk, so $(if $(PKG_PROVIDES),PROVIDES:=$(PKG_PROVIDES)) at luci.mk:168 picks it up inside the generated Package/$(PKG_NAME) block, past the bare PROVIDES:= in Package/Default. Placement and spelling match the in-tree precedent in luci-app-openlist/Makefile:13 and luci-app-package-manager/Makefile:16, and those three are now the only PROVIDES-style assignments in the tree. Both commit messages match their diffs, and the radicale3 fix is correctly split into its own commit rather than folded into the qos work.
Generated by Claude Code
Rename luci-app-qos to luci-app-qos-legacy. Adjust acl and .po files. Also tell users that the underlying qos-scripts is ancient and that SQM & qosify are the more modern QoS tools currently supported from the LuCI repo. Signed-off-by: Hannu Nyman <hannu.nyman@iki.fi>
Set the package to provide luci-app-qos, so that users of that ancient semi-defunct package could find this new implementationthat is compatible with the current firewall. (The old luci-app-qos has been renamed as luci-app-qos-legacy.) Signed-off-by: Hannu Nyman <hannu.nyman@iki.fi>
Correct the PROVIDES line by adding the PKG_ prefix to it. Signed-off-by: Hannu Nyman <hannu.nyman@iki.fi>
Yeah, Anthropic is doing a pretty amazing job with Claude. 👍 Much better than Google. |
|
@jow- https://github.com/openwrt/luci/blame/73adbd5e3a499e28fe7a59df1ff707e3ee7211c4/luci.mk#L164-L168 I didn't at a quick look figure any real need for the prefixes. |
4 participants
Tell the users that the underlying qos-scripts is ancient and that SQM & qosify are the more modern QoS tools currently supported from the LuCI repo.
cc @systemcrash
Reference to forum discussion in https://forum.openwrt.org/t/qos-disabled-by-luci-still-impacts-performance-bug/251783
The old qos-scripts may look like the main QoS tool for OpenWrt, although it has been superseded and practically deprecated by SQM and later also by qosify (by @nbd168 ). Now, when also qosify has a LuCI app in master, we might change the title & description of the ancient qos LuCI package to more clearly point out that it is not the preferred tool any more.
It might also be good if @nbd168 would change the description of the qos-scripts package itself, so that the old iptables based app would more clearly look outdated (in the current nftables firewall4 world).