flashprog: add new package - #30228
Conversation
BKPepe
left a comment
There was a problem hiding this comment.
The split into four variants makes sense if it provides a meaningful reduction in package size and/or dependencies, but the PR currently doesn't show that. Could you provide the package/installed sizes of the four variants (ideally including their dependencies) and briefly explain the intended use case for each variant?
Also, since flashprog is already provided as the full variant and the other variants all have the same description, it would be useful to clarify when a user should choose flashprog-spi, flashprog-pci, or flashprog-external instead of the default full package.
openwrt-ai
left a comment
There was a problem hiding this comment.
Fresh review of the single commit c84a34c. The commit message matches what the diff does (four variants, the programmer split, and the deliberate per-variant binary names), and the packaging basics look right: PKG_MAINTAINER is set, the define indentation follows the metadata/recipe convention, BuildPackage is called with literal package names, and test.sh avoids grep -q.
Two inline comments, both about the variant layout rather than about correctness — neither blocks a merge as far as I can tell.
Generated by Claude Code
c84a34c to
d1c85cd
Compare
|
Hi Josef, thanks for taking a look. I was skeptical of needing variants
Dependencies counted: flashprog-pci libpci; flashprog-external libftdi1, Every variant carries flashprog-pci adds the internal and PCI device programmers, which reach a flashprog-external adds the programmers that attach over USB, a serial port flashprog carries all of them. I have given each variant its own description saying which programmers it p.s. sorry for not including initially. I was desperately trying not to -dc |
d1c85cd to
e145aea
Compare
flashprog carries the four-variant package now on openwrt/packages#30228, with the per-variant descriptions the review asked for; ci-flashprog carries the apk-only check that verifies each variant's programmer set.
e145aea to
2379c6b
Compare
|
You ought to stick that table into the commit message, so non-github users can see it, too. |
2379c6b to
f176d9f
Compare
|
thanks @efahl, done, I edited commit message for terseness too (and put closes tag in the right place!). |
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-review of the force-pushed f176d9f (replaces c84a34c). The branch was also rebased onto current master, so the only change to this PR's own files is in utils/flashprog/Makefile.
DEFAULT_VARIANT:=1 is gone, which resolves my earlier comment — with four distinct package names and no shared PROVIDES, it had no effect. The four per-variant descriptions are the standard $(call Package/<pkg>/Default/description) idiom used by ~100 other Makefiles in the feed, and the library lists match each variant's DEPENDS exactly. The commit message now carries the size table, its programmer counts are self-consistent (4 base + 17 pci + 16 external = 37 full) and match the numbers you posted in the thread, and the subject scope matches the files touched. All 14 checks are green on this head.
Two nits inline, neither blocking.
One earlier thread is still open and unanswered: the per-variant binary renaming on lines 124/129/134 means the command a user types depends on which variant they installed, so upstream docs and any script calling flashprog only work with the full variant. Your comment explains what each variant is for, which is useful, but doesn't say whether installing two of them side by side is an actual use case — that's the part that would justify renaming over CONFLICTS/ALTERNATIVES. Worth a line either way so the thread can be closed.
Generated by Claude Code
76dd009 to
0d71900
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-review of the force-pushed 0d71900 (replaces f176d9f). The only change is inside the four description blocks of utils/flashprog/Makefile — 6 insertions, 4 deletions, no other file touched. The commit message is byte-identical to the previous head and still matches what the diff does, so no commit checks this round.
Both nits from the last round are addressed:
libusb→libusb-1.0in theflashprogandflashprog-externaldescriptions (lines 71 and 79), so every library named across the four descriptions is now the exact feed package name.flashprog-pciandflashprog-spieach gained the missing "when to pick it" clause ("for a chip the kernel permits direct access to" / "for a chip it already exposes"), so all three narrow variants now read in parallel withflashprog-external.
The library lists still match each variant's DEPENDS exactly, and all four earlier threads are resolved, including the binary-renaming one. All 14 checks are green on this head.
Reviewed 1 new commit; no new issues found.
Generated by Claude Code
|
I'm sort of curious about your decision to name the delivered binaries all differently, rather than just Seems a lot like the |
|
hi @efahl i was matching flashrom 's beat here (albeit, using flashprog's real group names). happy to revise to your way, maybe flashrom itself would benefit from an update to this pattern too (isn't APK great!)? cheers |
|
I'm not sure we'd want to change Here's how it's done in https://github.com/openwrt/openwrt/blob/main/package/network/services/dnsmasq/Makefile#L51 You ultimately end up with something that looks like this on your device, where the $ apk query --format yaml --fields package,provides,provider-priority 'dnsmasq*'
# 3 items
- package: dnsmasq-2.93-r1
provider-priority: 100
provides: # 1 items
- dnsmasq-any
- package: dnsmasq-dhcpv6-2.93-r1
provides: # 2 items
- dnsmasq=2.93-r1
- dnsmasq-dhcpv6-any
- package: dnsmasq-full-2.93-r1
provides: # 2 items
- dnsmasq=2.93-r1
- dnsmasq-full-any |
0d71900 to
0c5278d
Compare
|
@efahl I went with the variant pattern, with one deviation from dnsmasq: all four variants provide So |
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-review of the force-pushed 0c5278d (replaces 0d71900). The change is the switch to the variant/PROVIDES pattern: PROVIDES:=flashprog-bin in Package/flashprog/Default, DEFAULT_VARIANT:=1 back on the full package, the three per-variant install blocks collapsed into Package/<variant>/install = $(Package/flashprog/install), and the matching case collapse in test.sh.
The commit message was updated in step with it — "each installs the same binary under its own name, so any combination can be installed together" became "each installs the same binary, and the variants are mutually exclusive" — so the message still matches the diff and the size table is unaffected by the rename. No commit checks this round.
I checked the mechanism against the build system rather than just against neighbours:
flashprog-bincarries no@prefix and the package has no ABI version, soAddProvideemits it asflashprog-bin=1.5, i.e. a versioned provide — "only one version of a provide can be installed at the same time". That is what actually makes the four variants exclusive under apk, and it is the same device thewget-ssl/wget-nosslexample in that file describes.- Putting the shared provide on all four (rather than dnsmasq's "narrow variants provide the base name") also lines up with
add_implicit_provides_conflicts: it needs one provider markedvariant_default, whichDEFAULT_VARIANT:=1now supplies, and it generates theConflictsentries and the menuconfig exclusions from there. Without the re-addedDEFAULT_VARIANTthat pass would bail out, so the two halves of this commit belong together. Package/<x>/install = $(Package/flashprog/install)is the established feed idiom (~30 Makefiles, e.g.libs/libulfius/Makefile,net/haproxy/Makefile), and the recursive assignment still satisfies theifdef Package/$(1)/installgate inpackage-pack.mk, so the three variants keep their install step.test.shstill avoidsgrep -q, andflashprog --versionprintsflashprog v1.5, which containsPKG_VERSION, so notest-version.shis needed.
All 14 checks are green on this head, and the earlier binary-naming thread is now settled by the change itself.
Reviewed 1 new commit; no new issues found.
Generated by Claude Code
|
Just one nit: It seems like listing the dependencies in the descriptions is redundant, as 1) you could get them with Otherwise, looks good to me! |
0c5278d to
82e211d
Compare
|
hi, fixed, thanks so much for all your time on this! |
82e211d to
67d370d
Compare
Detects, reads, writes, verifies and erases flash chips over SPI, LPC, FWH and parallel buses. Four variants split the programmers by the libraries they need; each installs the same binary, and the variants are mutually exclusive. Installed size with dependency closure, x86/64, in bytes (libc, libpthread and librt excluded): | package | programmers | installed +deps | |--------------------|-------------|-----------------| | flashprog-spi | 4 | 684,818 | | flashprog-pci | 21 | 935,964 | | flashprog-external | 20 | 1,027,401 | | flashprog | 37 | 1,274,442 | Signed-off-by: David Connolly <david@connol.ly>
67d370d to
34ed91a
Compare
📦 Package Details
Maintainer: @connollydavid
Description:
Adds flashprog 1.5 in four variants, splitting the programmers by the libraries
they need.
🧪 Run Testing Details
Closes #29591
✅ Formalities
If your PR contains a patch:
This PR contains no patches.