https://bugzilla.redhat.com/show_bug.cgi?id=2216600 Ben Beasley <code@xxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |code@xxxxxxxxxxxxxxxxxx --- Comment #14 from Ben Beasley <code@xxxxxxxxxxxxxxxxxx> --- Steve Cossette asked me to double-check the review. ---- The suggestions offered in the review process looked reasonable. ---- Regarding remaining rpmlint messages: > kaidan.x86_64: W: no-manual-page-for-binary kaidan Man pages are desired in general (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages) but this is a SHOULD rather than a MUST. It’s worth pointing this out in a review, but it shouldn’t be considered a blocker. It’s often difficult to convince upstreams of the value of maintaining man pages. I find that most upstreams won’t consider it unless the man pages can be automatically generated from some existing source. > kaidan.x86_64: W: files-duplicate /usr/share/kaidan/images/kaidan.svg /usr/share/icons/hicolor/scalable/apps/kaidan.svg This is fine. These files could probably safely be hardlinked without the risk of cross-filesystem hardlinks, since both are under /usr/share/ (but rpmlint would then complain about cross-directory hardlinks possibly being on different filesystems), or they could be explicitly symlinked, but duplicating a single 4 kiB file is just not a big deal. I would probably not recommend any change to the packaging. ---- The fedora-review tool suggests that large data in /usr/share should live in a noarch subpackage if package is arched. This allows the noarch subpackage to be shared across architectures, and reduces duplicated data on the mirrors. Whether ~1MiB is large enough to worry about is a matter of opinion. It would be reasonable to consider moving the data into a noarch -data subpackage, but I don’t think the data files are large enough to say this *needs* to happen. ---- I think the License should include an “AND LGPL-2.0-or-later” term for src/qml/elements/IconTopButton.qml. ---- The files src/hsluv-c/hsluv.h and src/hsluv-c/hsluv.c are a bundled copy of http://github.com/hsluv/hsluv-c. This is designed as a ”copylib” and doesn’t have an upstream build system for a separate library, so it’s unlikely it can be separately packaged for Fedora. However, it still needs to be handled in accordance with https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. At minimum, the appropriate virtual Provides should be added. ---- The sole source (Source0:) doesn’t need to be numbered; you can just write ”Source:”. Numbering it doesn’t hurt anything, though. ---- As a reviewer, it’s a good practice to post the filled-in review checklist template from the fedora-review output. This shows you’ve considered all of the things that it can’t check automatically, and is generally useful when people need to go back and check old reviews. ---- Overall, this looks like a clean package and a high-quality review. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2216600 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202216600%23c14 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
