https://bugzilla.redhat.com/show_bug.cgi?id=1409884 --- Comment #4 from Brian Exelbierd <bex@xxxxxxxxx> --- Hi Igor, Thank you for the feedback. It is extremely valuable. > 1. I would recommend to change name of package to something in lower-case (https://fedoraproject.org/wiki/Packaging:Naming#General_Naming) Ahh. I read "should" as meaning that projects that have formally upcased names can have upcased packages. This change will definitely make other lines easier. > 2. Missing BuildRequires: gcc (all BuildRequires must be specified) Ok. I has thought that allowing it to be brought in implicitly because vala needs it was enough. I'll fix this. > 3. Since upstream uses pkg-config to find dependencies (PKG_CHECK_MODULES(DAYJOURNAL, gee-0.8 > [gtk+-3.0 libnotify appindicator3-0.1 gio-2.0 gdk-3.0 gtk+-3.0 glib-2.0])), it's worth to change style of BuildRequires: > pkgconfig(gtk+-3.0) > pkgconfig(libnotify) > ... > and remaining ones This also makes sense. If anyone comes after me and reads this, I think this requirement is documented here: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_based_on_pkg-config > 4. if you will rename package to dayjornal, then you can drop "-n ..." part in %autosetup -n dayjournal-%{version} > 5. cp %{SOURCE1} . doesn't preserve timestamps (you can add `-a` to cp invocation) > 6. make %{?_smp_mflags} -> %make_build (this is preferred because of some switches in parallel building inside RPM) done. I was able to eliminate the copies, see %install > 7. install -m 644 -D dayjournal.desktop %{buildroot}/usr/share/applications/dayjournal.desktop > 7.1. Add `-p` to preserve timestamps > 7.2. Use %{name} instead of dayjornal if you will rename it > 7.3. replace /usr/share with %{_datadir} I missed %{_datadir} in my reading. I should have searched more. > 8. %make_install install-exec, is it really needed to specify install-exec? In this case, I think so. The install will, by default, install bunch of zero-byte "documentation" files. The patch I wrote eliminates those from the build at the install-exec level. > 9. /usr/share/icons/hicolor/48x48/apps/dayjournal-icon.png > 9.1. same to replace /usr/share with %{_datadir} > 9.2. I think having "-icon" is pointless I updated to use the macro, however the icon was not getting picked up unless named -icon. I am not sure why. > 10. You should run desktop-file-validate against desktop file in %check section Based on my read of https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files I am wondering if using desktop-file-install in %install isn't better. Let me know. > 11. License is GPLv3+, not GPLv3 fixed. The spec file and SRPM have been uploaded to the same urls. The COPR has been updated as well. -- 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