k3screenctrl: add new package - #30281
Conversation
dc60772 to
5557a0c
Compare
5557a0c to
e83f3b6
Compare
| PKG_LICENSE:=GPL-2.0-only | ||
| PKG_LICENSE_FILES:=COPYING | ||
| PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz | ||
| PKG_SOURCE_URL:=https://github.com/kazutoiris/k3screenctrl/archive/refs/tags/v$(PKG_VERSION).tar.gz? |
There was a problem hiding this comment.
Why not use https://github.com/lwz322/k3screenctrl ? Your repository is only 2 commits ahead of that repository and there is no pull request to the upstream repository.
Also ping @zxlhhyccc who created https://github.com/zxlhhyccc/Hill-98-k3screenctrl
There was a problem hiding this comment.
Since most of the authors/modifiers are no longer reachable, I currently use the Phicomm K3 and will continue to maintain it.
There was a problem hiding this comment.
4 days ago. Hmm, still I would like to see pull request there.
There was a problem hiding this comment.
Since the most recent commit for lwz322/k3screenctrl was made 3 years ago, and zxlhhyccc/Hill-98-k3screenctrl was made 7 years ago, many maintainers have abandoned the Phicomm K3. Of course, if you are willing to wait, I don't mind.
| PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz | ||
| PKG_SOURCE_URL:=https://github.com/kazutoiris/k3screenctrl/archive/refs/tags/v$(PKG_VERSION).tar.gz? | ||
| PKG_HASH:=cc9927f324ce7e221a2ac3351ec78156719195662cc5bafd5424d340c170cd39 | ||
| PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)-$(PKG_VERSION) |
There was a problem hiding this comment.
This is already default, so it can be dropped.
There was a problem hiding this comment.
It seems that only PKG_BUILD_DIR matches the default value.
While PKG_SOURCE, PKG_SOURCE_URL, and PKG_HASH all differ from it.
Am I misunderstanding something?
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
e83f3b6"feat: add k3screenctrl" — the subject uses a Conventional Commits type prefix instead of the package name.CONTRIBUTING.mdasks for "a useful commit subject prefixed with the package name (E.g.:foopkg: ...)". The PR title (k3screenctrl: add new package) already has the right shape; the commit subject should match it.
Nothing here looks like a build-breaker. The one finding I would resolve before merge is the DEPENDS device gate — if I read scripts/target-metadata.pl correctly, it keeps the package out of every buildbot/SDK config, which would defeat the purpose of adding it to the feed. Details inline; I flagged it as a question because the single in-tree precedent uses the same form.
The rest ($(INSTALL_BIN) on oui.txt, the non-numeric UCI clamp) are small correctness items with one-click suggestions, and the nit:-prefixed comments are optional.
I have not commented on the upstream-repository discussion in @BKPepe's thread, nor on PKG_BUILD_DIR — both are already open above. I was also unable to verify the contents of the release tarball (PKG_HASH, presence of COPYING, the lib/k3screenctrl/*.sh layout referenced by the install recipe) from this session, so those are unchecked.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the 2 new commits since e83f3b6.
Commit checks
2df0f80"chore: modify Makefile and config" — two issues. The subject uses a Conventional Commits type prefix instead of the package name;CONTRIBUTING.mdasks for "a useful commit subject prefixed with the package name (E.g.:foopkg: add libzot dependency)". And the subject understates the scope: the diff also rewrites the UCI value validation infiles/k3screenctrl.init, which neither "Makefile and config" nor the body ("modify for review.") mentions.3556b6f"chore: modify Makefile" — same prefix issue;k3screenctrl: ...rather thanchore: ....
Note this is the same point I raised on e83f3b6 last round, so all three commits currently carry it. The FormalityCheck jobs are green on 3556b6f, so the bot is not going to catch this one for you.
The code in both new commits is correct as far as I can check:
- Dropping
+coreutilsis right —GenPlugininutils/coreutils/MakefilesetsDEPENDS:=+coreutils $(DEPENDS_$(2))on every generated sub-package, socoreutils-odalready pulls it in. $(INSTALL_DATA)onoui.txtand thecase-based validation in the init script both address what I flagged.
Still open, and the one thing I'd resolve before merge: the @TARGET_bcm53xx_generic_DEVICE_phicomm_k3 gate on line 24. Re your reply that TARGET_DEVICE_ is not listed under dependency_types — that page enumerates the common forms, but an @ dependency is a plain Kconfig expression and any symbol is valid there, which is why utils/mox-pkcs11 can name per-device symbols at all. My concern was not the spelling but which symbol is actually set: config TARGET_$conf_$id at target-metadata.pl:255 is emitted only for the single-device profile choice, while multi-profile builds — the buildbot/SDK default — emit menuconfig TARGET_DEVICE_$conf_$id at line 314 instead. If that reading is right, the symbol is n in the feed builds and the package never ships as a prebuilt .ipk, which is the opposite of the motivation in your PR description. Worth a maintainer's call — I can't test a buildbot config from here.
I have not re-reviewed the tarball contents (PKG_HASH, presence of COPYING, the lib/k3screenctrl/*.sh layout the install recipe expects), and @BKPepe's upstream-repository question is still unanswered above.
Generated by Claude Code
k3screenctrl is the package that drives the screen of Phicomm K3. Signed-off-by: Kazuto Iris <hitomi@apache.org>
3556b6f to
5d4b244
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the squash to 5d4b244. git diff 3556b6f..5d4b244 is empty — the three commits were collapsed with no content change, so everything I checked last round still holds and I have no new code findings.
Commit checks
5d4b244"feat: add k3screenctrl" — the squash kept the Conventional Commits type prefix.CONTRIBUTING.md:76asks for "a useful commit subject prefixed with the package name (E.g.:foopkg: ...)", sok3screenctrl: add new package— which is already the PR title — rather thanfeat: .... This is now the only commit in the PR, so it is a one-linegit commit --amendaway. Third round for this point; the FormalityCheck jobs are green on5d4b244and will not catch it.
The squash itself resolves the introduce-then-fix hygiene point, and the $(INSTALL_DATA) fix, the +coreutils drop and the case-based UCI validation all carried through intact.
Still open, and the one item I would settle before merge: the @TARGET_bcm53xx_generic_DEVICE_phicomm_k3 gate. The force-push marked that thread outdated and collapsed it, so I have re-anchored it inline on the current head.
Unchanged from last round: I still cannot inspect the release tarball from this session (PKG_HASH, presence of COPYING for PKG_LICENSE_FILES, the lib/k3screenctrl/*.sh layout the install recipe expects) — the connector is scoped to openwrt/packages only, so those are unverified rather than verified-good. @BKPepe's upstream-repository question also remains unanswered.
Generated by Claude Code
| define Package/k3screenctrl | ||
| SECTION:=utils | ||
| CATEGORY:=Utilities | ||
| DEPENDS:=+coreutils-od +bash +curl @TARGET_bcm53xx_generic_DEVICE_phicomm_k3 |
There was a problem hiding this comment.
Re-anchoring this one because the squash marked the earlier thread outdated and collapsed it — the code is unchanged and the question is still open, so I don't want it to disappear from the page.
Short version: TARGET_bcm53xx_generic_DEVICE_phicomm_k3 is emitted only by the single-device profile choice (config TARGET_$conf_$id at target-metadata.pl:255). Multi-profile builds — the buildbot/SDK default — emit menuconfig TARGET_DEVICE_$conf_$id at line 314 instead. If that reading holds, the symbol is n in feed builds and no prebuilt .ipk is ever produced, which is the opposite of the motivation in the PR body ("all users have to search and compile it on their own").
I can't test a buildbot config from here, so this is a maintainer call rather than an assertion. If the gate is only meant to keep the package out of menuconfig on other targets, @TARGET_bcm53xx would do that and still get a binary into the feed.
Generated by Claude Code
📦 Package Details
Maintainer: @kazutoiris
Description:
k3screenctrl is the package that drives the screen of Phicomm K3. If it is not installed, the screen will always stay on and display "Rebooting". Therefore, this package is essential for all Phicomm K3 users.
Over the past 10 years, this package was hosted in different maintainers' repositories, and all users have to search and compile it on their own.
🧪 Run Testing Details
✅ Formalities