Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1529758
Summary: | Review Request: paper-icon-theme - Modern freedesktop icon theme | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Björn 'besser82' Esser <besser82> |
Component: | Package Review | Assignee: | Carl George <carl> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | carl, package-review |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | carl:
fedora-review+
|
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2018-01-09 16:18:37 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Björn 'besser82' Esser
2017-12-29 19:58:19 UTC
Scratch build: Rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=23944517 EPEL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=23944531 === Updated package === Scratch build: Rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=23944844 EPEL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=23944846 Urls: Spec URL: https://pagure.io/besser82/package-review/raw/master/f/paper-icon-theme.spec SRPM URL: https://pagure.io/besser82/package-review/raw/master/f/paper-icon-theme-1.4.0-0.2.fc28.src.rpm This package looks good to me overall, but I have questions/comments about a few minor things. The package requires adwaita-icon-theme, gnome-icon-theme, and hicolor-icon-theme. Why is that? The giturl macro is only used once. That's a pet peeve of mine, so I personally would remove the macro and just insert the url into Source0. Just a suggestion, not required. That said, Source0 doesn't follow the recommend format for GitHub tag tarballs [1]. Please change it to: https://github.com/snwh/%{name}/archive/v%{version}/%{name}-%{version}.tar.gz I think only the LICENSE file should be marked %license [2]. AUTHORS should be marked with %doc. I don't think COPYING should be included, as it appears to just be a brief summary of the actual license. If you still want to include it, it should probably be marked as %doc as well. The scriptlet example in the icon cache guidelines [3] runs touch before gtk-update-icon-cache in %postun, but your %postun just runs gtk-update-icon-cache. After contemplating it, I think the example is incorrect because that directory won't exist at that point. I don't think any action is needed for this, I just wanted to check and see if you understood this the same way I do. [1]: https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Tags [2]: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text [3]: https://fedoraproject.org/wiki/Packaging:Scriptlets#Icon_Cache (In reply to Carl George from comment #3) > The package requires adwaita-icon-theme, gnome-icon-theme, and > hicolor-icon-theme. Why is that? This is because the icon theme inherits from them in the following order: Adwaita ---> Gnome ---> Hicolor See: https://github.com/snwh/paper-icon-theme/blob/master/Paper/index.theme `Inherits=Adwaita,gnome,hicolor` For this reason all three need to be present and thus I've added those explicit Requires for them. > The giturl macro is only used once. That's a pet peeve of mine, so I > personally would remove the macro and just insert the url into Source0. Mhh… Personal preference, I'd say… I like to keep lines short. > Just a suggestion, not required. That said, Source0 doesn't follow the > recommend format for GitHub tag tarballs [1]. Please change it to: > https://github.com/snwh/%{name}/archive/v%{version}/%{name}-%{version}.tar.gz Same here. The guidelines say 'For the source tarball, you *can* use the following syntax'. The syntax I'm using here works that way since 5 years and actually reflects the original link as provided on github. > I think only the LICENSE file should be marked %license [2]. AUTHORS should > be marked with %doc. I don't think COPYING should be included, as it > appears to just be a brief summary of the actual license. If you still want > to include it, it should probably be marked as %doc as well. I'll move COPYING to %%doc during import… About the AUTHORS file I really disagree, since we're dealing with a CC-BY-SA license here, which means, we need to attribute all authors. When the file is in %%doc, it is not guaranteed to be installed (and thus the authors are not properly attributed). One could omit installation of %%doc by adding `--nodocs` to dnf command or `--excludedocs` to rpm command. The AUTHORS file is generally a culprit depending on the underlying license of the sources; some licenses *require* it to be guaranteed to be present in the 'binary' package, e.g. BSD, CC-BY-SA, ISC, NCSA. > The scriptlet example in the icon cache guidelines [3] runs touch before > gtk-update-icon-cache in %postun, but your %postun just runs > gtk-update-icon-cache. After contemplating it, I think the example is > incorrect because that directory won't exist at that point. I don't think > any action is needed for this, I just wanted to check and see if you > understood this the same way I do. Yes… The example in the guidelines refers to hicolor icons. Since those are almost always present, it is required to modify the mtime of that directory to sth. more recent then the generated icon-cache when things change, so gtk-update-icon-cache will rebuild the cache with the changed (added / removed) icons in that dir. For this package things are different as you already pointed out, I agree. Thanks for those extra details. I'm happy with each of those responses, so package APPROVED! (fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/paper-icon-theme. You may commit to the branch "f27" in about 10 minutes. paper-icon-theme-1.4.0-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-8c19ed35f9 paper-icon-theme-1.4.0-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-530d05cfa0 paper-icon-theme-1.4.0-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2018-3cf5fe9165 paper-icon-theme-1.4.0-1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report. paper-icon-theme-1.4.0-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-8c19ed35f9 paper-icon-theme-1.4.0-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-3cf5fe9165 paper-icon-theme-1.4.0-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-3cf5fe9165 paper-icon-theme-1.4.0-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report. paper-icon-theme-1.4.0-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report. |