https://bugzilla.redhat.com/show_bug.cgi?id=2007918 Jakub Kadlčík <jkadlcik@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jkadlcik@xxxxxxxxxx --- Comment #1 from Jakub Kadlčík <jkadlcik@xxxxxxxxxx> --- Hello Frank, I like your project and the package as well. > %if 0%{?python3_pkgversion} > Requires: python%{python3_pkgversion} > %else > Requires: python3 > %endif According to the documentation https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_dependencies A package should not specify its dependency on python3 package. I think you can drop those lines. > BuildRequires: sed python3-rpm-macros python-srpm-macros I cannot find the proper documentation right now but I believe you don't have to explicitly depend on `sed` as well because it is a part of the minimal buildroot. Also, I would recommend specifying one dependency per line. Like this: BuildRequires: sed BuildRequires: python3-rpm-macros BuildRequires: python-srpm-macros Later on, it will be much easier to read the diffs when some dependency gets removed/added. This is just a tip from personal experience, and it is totally up to your preference. The way you did it is also correct. > Tuptime track and report historical and statistical real time of the > system, keeping the uptime and downtime between shutdowns. The space at the beginning of the second line is unnecessary > %build > exit 0 You can also leave the %build phase empty > cp -R %{_topdir}/BUILD/%{name}-%{version}/src/tuptime %{buildroot}%{_bindir}/ > ... You don't have to specify the absolute path to the sources. You can simply use cp -R src/tuptime %{buildroot}%{_bindir}/ Also, using -R for files doesn't have any benefits and makes it a little bit harder to read. > %{_mandir}/man1/tuptime.1.gz I would recommend using the following wildcard instead %{_mandir}/man1/tuptime.1.* >From time to time, it happens that a compression format for something is changed, and this way you won't have to deal with that. Every package I know uses this :-) > %doc misc/scripts/ Those scripts don't look like they are used for documentation purposes but rather as something that is supposed to be executed between major upgrades. I think they should be stored somewhere else than the /usr/share/doc directory > %defattr(-,root,root) > %dir %attr(0755, _tuptime, _tuptime) %{_sharedstatedir}/tuptime/ Is there any motivation for setting these permissions? AFAIK they are the defaults because dropping these two lines don't seem to have any effect on the output. > su -s /bin/sh _tuptime -c "(umask 0022 && /usr/bin/tuptime -x)" I am not sure what this line does. Can you please help me understand? I made a couple of suggestions. They are just minor details that are easy to fix. Overall, I really like the package. Good job here, Frank. -- 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=2007918 _______________________________________________ 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
