https://bugzilla.redhat.com/show_bug.cgi?id=2035944 --- Comment #5 from Maxwell G <gotmax@e.email> --- (In reply to Fabio Valentini from comment #4) > 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 I changed it, as there's not much of a point to use the forge macros in this scenario. I have not noticed any issues between rpmautospec and the forge macros. I have noticed that the way the forge macros handle git snapshots (%commit macro) is incompatible with the new versioning guidelines[0]; `%forgemeta` still adds the commit information to the `Release:` field. Perhaps the SourceURL[1] Packaging Guidelines should be amended to account for the problematic forge macros? [0]: https://pagure.io/packaging-committee/pull-request/908 [1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > 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. Done. > 3) Separate file / directory ownership for /usr/share/touchegg I know that this isn't necessary, but I wanted to be explicit as a personal reminder and to prevent issues down the line. Still, I ended up liking your layout better, so I adopted it. > 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 That's a good point. I fixed it and added an explanatory comment to the specfile. -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. 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
