https://bugzilla.redhat.com/show_bug.cgi?id=2035944 --- Comment #4 from Fabio Valentini <decathorpe@xxxxxxxxx> --- Package looks good, some small comments before I finish up the review: 1) Consider not using the Forge macros I would advise against using the forge macros, except if you're doing Go or Font packages (where you have no choice other than to use them). The "%forge*" macros are unmaintained for a while, and their original author is no longer contributing to Fedora. They are also not entirely compatible with rpmautospec, as far as I can tell. Instead, just use the standard SourceURL guidelines, for something like: URL: https://github.com/JoseExposito/touchegg Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz 2) BuildRequires: %{_bindir}/desktop-file-validate This is not good. %{_bindir} should not be used in BuildRequires, as it is influenced by build-specific settings like %{_prefix}, which might evaluate to /app instead of /usr, for example. Just use "BuildRequires: desktop-file-utils" instead. 3) Separate file / directory ownership for /usr/share/touchegg %dir %{_datadir}/%{name} %{_datadir}/%{name}/%{name}.conf This looks weird to me. Why not just have the package own "%{_datadir}/%{name}/" directly? That already includes the directory and all files under it, without having to list everything explicitly. 4) The order of files in %files is very creative While this is entirely within the realm of "personal choice", I recommend to follow what other packages do in this regard, i.e. list files that use the special %license and %doc macros first (they execute code, and not only list files), then %config files, followed by an alphabetical list of files, for example, like this: """ %files %license COPYING COPYRIGHT %doc README.md CHANGELOG.md %config(noreplace) %{_sysconfdir}/xdg/autostart/%{name}.desktop %{_bindir}/%{name} %{_datadir}/%{name}/ %{_unitdir}/%{name}.service """ This makes it easy to consistently add new files to the list and keep it easily readable. Feel free to ignore this point, it is just my advice based on personal experience. 5) The systemd service is not restartable Restarting the systemd service on package update apparently breaks clients: https://github.com/JoseExposito/touchegg/issues/453 In this case, the systemd scriptlet guidelines state that you should use %systemd_postun (without _with_restart): https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets -- 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=2035944 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
