https://bugzilla.redhat.com/show_bug.cgi?id=2216600
Ben Beasley <c...@musicinmybrain.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |c...@musicinmybrain.net
--- Comment #14 from Ben Beasley <c...@musicinmybrain.net> ---
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
Do not reply to spam, report it:
https://pagure.io/fedora-infrastructure/new_issue
