Description: Paper is a modern freedesktop icon theme whose design is based around the use of bold colours and simple geometric shapes to compose icons. Each icon has been meticulously designed for pixel-perfect viewing. While it does take some inspiration from the icons in Google's Material Design, some aspects have been adjusted to better suit a desktop environment. Issues: fedora-review shows no obvious issues. FAS-User: besser82 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.1.fc28.src.rpm Thanks for review in advance!
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.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.