[Bug 1529758] Review Request: paper-icon-theme - Modern freedesktop icon theme

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1529758



--- Comment #4 from Björn "besser82" Esser <besser82@xxxxxxxxxxxxxxxxx> ---
(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.

-- 
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
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux