Repository navigation
docs(players): ship descriptors from /usr/share, not /etc - #662
Conversation
The guide still told plugin authors to install the WebUI descriptor and icon into /etc/hifiberry/players.d, the conffile pattern that hifiberry-librespot 0.8.0.10 existed to abolish: dpkg treats a pre-existing file there as a local edit and preserves it for ever, so a device that once had one placed by hand never receives another packaged update to it. Point the file table and the librespot worked example at /usr/share/hifiberry/players.d, which configurator has read since 2.17.6, and say plainly that /etc remains an administrator's override rather than a place for a package to write. Also correct the librespot example itself, which claimed the postinst creates the descriptor and icon and then listed four files under the heading "three". Only the configserver drop-in is written by a maintainer script; the rest are ordinary packaged files. This guide is what third-party player authors follow, so the stale advice propagated the problem rather than merely recording it.
|
Automated review of |
![hifiberry-codereview[bot]](https://avatars.githubusercontent.com/u/8243622?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review
This documentation-only update corrects docs/add-your-own-player.md to instruct plugin authors to install descriptors in /usr/share/hifiberry/players.d instead of /etc, preventing dpkg from treating packaged files as local edits and blocking future updates. The change also fixes factual errors in the librespot example regarding maintainer script behavior and file counts.
1 finding(s) (1 low) · reviewed the full diff at 94b1667 · 1 file(s)
Applied the review guidance in .codereview.toml.
Generated by codereview using thufir · 16,678/500,000 tokens over 3 call(s). Findings are machine-generated; verify before acting.
|
|
||
| No code changes required — everything is pure drop-in. | ||
|
|
||
| **Ship the descriptor and icon under `/usr/share`, not `/etc`.** A descriptor |
There was a problem hiding this comment.
🔵 LOW: Section 5 still directs plugin authors to /etc, contradicting the new /usr/share guidance
The PR's stated purpose is to stop the guide from telling plugin authors to install the descriptor and icon into /etc/hifiberry/players.d. The new guidance at line 249 explicitly says "Ship the descriptor and icon under /usr/share, not /etc", and the table in section 6 now lists /usr/share paths. However, section 5 ("Register in the Web UI"), which is the primary step-by-step instructional section that authors follow, still reads: "Create /etc/hifiberry/players.d/.json" and "Place an SVG icon at /etc/hifiberry/players.d/icons/.svg". These lines were not changed in this diff.
How it fails: A third-party plugin author follows the guide top-to-bottom. At section 5 they create the descriptor at /etc/hifiberry/players.d/my-player.json and the icon at /etc/hifiberry/players.d/icons/my-player.svg, exactly as instructed. They then reach section 6 and see the table listing /usr/share paths plus the new paragraph saying "Ship the descriptor and icon under /usr/share, not /etc". The guide now contradicts itself: the step-by-step instructions say /etc, the summary and guidance say /usr/share. The author either ships to /etc (reproducing the exact conffile-staleness problem the PR is fixing) or is confused about which path is correct. The PR's stated goal of eliminating the stale /etc advice is not achieved for the section that actually instructs the author where to create the files.
Suggested fix: Update section 5's sub-sections (a) and (b) to use /usr/share/hifiberry/players.d/.json and /usr/share/hifiberry/players.d/icons/.svg, matching the table in section 6 and the new guidance paragraph. For example: "Create /usr/share/hifiberry/players.d/.json:" and "Place an SVG icon at /usr/share/hifiberry/players.d/icons/.svg."
stated-intent · confidence 75%
The previous commit moved the summary table and the librespot example to /usr/share/hifiberry/players.d but left section 5 -- the step-by-step instructions an author actually follows -- still saying to create the descriptor and icon under /etc. The guide contradicted itself: an author working top to bottom would create the files at /etc exactly as instructed, then reach a table and a paragraph saying not to. That reproduces the conffile staleness the change exists to prevent, so the stale advice survived in the one section where it does the damage.
|
Automated review of |
There was a problem hiding this comment.
Automated review
This documentation update corrects docs/add-your-own-player.md to instruct plugin authors to install descriptors in /usr/share/hifiberry/players.d instead of /etc, preventing dpkg from treating packaged files as local edits and blocking future updates. The change also fixes inaccuracies in the librespot example regarding maintainer script responsibilities and file counts.
No blocking issues found. · reviewed changes since the last review at 7511b7e · 1 file(s)
Applied the review guidance in .codereview.toml.
Generated by codereview using thufir · 7,395/500,000 tokens over 2 call(s). Findings are machine-generated; verify before acting.
docs/add-your-own-player.mdstill told plugin authors to install the WebUI descriptor and icon into/etc/hifiberry/players.d— the conffile pattern hifiberry-librespot 0.8.0.10 existed to abolish. dpkg treats a pre-existing file there as a local edit and preserves it for ever, so a device that once had one placed by hand never receives another packaged update to it.This guide is what third-party player authors follow, so the stale advice propagated the problem rather than merely recording it.
Changes
/usr/share/hifiberry/players.d, which configurator has read since 2.17.6./etcremains an administrator's override, and a package should not write the descriptor from itspostinst— a file rewritten on every configure shadows the shipped one, destroys any override, and drifts from the packaged copy without anything reporting it.postinstcreates the descriptor and icon, and listed four files under the heading "three". Only the configserver drop-in is written by a maintainer script.Found while fixing exactly that drift in the package: hifiberry/hifiberry-librespot#2, where the
/etccopy had lost theconflicts_with: ["soloist"]declaration the shipped descriptor carries.Docs only — no code or packaging changes.