Spec URL: https://download.copr.fedorainfracloud.org/results/frankcrawford/tuptime/fedora-rawhide-x86_64/02842104-tuptime/tuptime.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/frankcrawford/tuptime/fedora-rawhide-x86_64/02842104-tuptime/tuptime-5.0.2-2.fc36.src.rpm Description: Tuptime track and report historical and statistical real time of the system, keeping the uptime and downtime between shutdowns. Fedora Account System Username: frankcrawford
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.
Jakub, Thanks for looking at this, and taking it on. I'll go through your comments sometime in the next few days and either adopt them or discuss them with you further. At first glance, they look to be sensible. As for the line > su -s /bin/sh _tuptime -c "(umask 0022 && /usr/bin/tuptime -x)" that essentially creates and initialises the tuptime DB with its first entry. Doing it here ensures that the permissions, etc, are controlled, rather than being in a more random state.
Have updated the spec file, pretty much as you noted, although with a few other changes, when I considered what was happening. Spec URL: https://download.copr.fedorainfracloud.org/results/frankcrawford/tuptime/fedora-rawhide-x86_64/03043781-tuptime/tuptime.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/frankcrawford/tuptime/fedora-rawhide-x86_64/03043781-tuptime/tuptime-5.0.2-3.fc36.src.rpm Note, I think that the line: > %dir %attr(0755, _tuptime, _tuptime) %{_sharedstatedir}/tuptime/ is required, because it is setting the permissions for the state directory in /var/lib. This is not really created during the build, with the right permissions.
@jkadlcik any update on this review?
Thank you for pinging me, I forgot. > # Check for EPEL Python (python34, python36) > %if 0%{?python3_pkgversion} > BuildRequires: python%{python3_pkgversion}-devel > %else > BuildRequires: python3-devel I've never seen this before. Is there any motivation for it? IMHO simply BuildRequires: python3-devel is good enough. > that essentially creates and initialises the tuptime DB with its > first entry. Doing it here ensures that the permissions, etc, are > controlled, rather than being in a more random state. Can you please put that as a comment in the spec file? I think it will be useful for whoever is going to read that spec file in the future. > %systemd_user_preun tuptime.service I would expect %systemd_preun since there is %systemd_post in the %post section.
Also, the fedora-review tool is reporting this issue Issues: ======= - systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in tuptime See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Scriptlets/#_scriptlets To be honest, I am not sure, what the problem is but we should figure it out.
New versions available here: Spec URL: https://download.copr.fedorainfracloud.org/results/frankcrawford/tuptime/fedora-rawhide-x86_64/03123758-tuptime/tuptime.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/frankcrawford/tuptime/fedora-rawhide-x86_64/03123758-tuptime/tuptime-5.0.2-4.fc36.src.rpm (In reply to Jakub Kadlčík from comment #5) > Thank you for pinging me, I forgot. > > > # Check for EPEL Python (python34, python36) > > %if 0%{?python3_pkgversion} > > BuildRequires: python%{python3_pkgversion}-devel > > %else > > BuildRequires: python3-devel > > I've never seen this before. Is there any motivation for it? IMHO simply > > BuildRequires: python3-devel > > is good enough. This is mostly for EPEL7, although technically could be for other systems, if they decided to handle multiple Python3 releases. The biggest thing is that it fixes on a particular version of Python3, in case they upgrade to a later one at some point. See https://fedoraproject.org/wiki/User:Bkabrda/EPEL7_Python3 > > that essentially creates and initialises the tuptime DB with its > > first entry. Doing it here ensures that the permissions, etc, are > > controlled, rather than being in a more random state. > > Can you please put that as a comment in the spec file? I think it will > be useful for whoever is going to read that spec file in the future. Added. > > %systemd_user_preun tuptime.service > > I would expect %systemd_preun since there is %systemd_post in the > %post section. I never noticed it was user and not system before. That also corrects the other issue, which I believe was the same complaint, just poorly worded. The actual test in Fedora-Review code is that the scriptlet exists in all three states (%post, $preun and %postun).
> This is mostly for EPEL7, although technically could be for other > systems, if they decided to handle multiple Python3 releases. Thank you, TIL ... :-) > That also corrects the other issue, which I believe was the same > complaint, just poorly worded. I also thought so, but I still see the error. However, the spec looks good to me and I saw the %systemd_* macros used in the documentation the same way you did. > [!]: %check is present and all tests pass. It looks like the project doesn't have any tests. Ideally, please add the %check section in the future, if someone implements them. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in tuptime See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Scriptlets/#_scriptlets ===== MUST items ===== Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU General Public License, Version 2", "GNU General Public License v2.0 or later", "*No copyright* GNU General Public License v2.0 or later". 47 files have unknown license. Detailed output of licensecheck in /var/lib/copr- rpmbuild/results/tuptime/licensecheck.txt [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 51200 bytes in 4 files. [x]: Package complies to the Packaging Guidelines [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [?]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [?]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [?]: Package should compile and build into binary rpms on all supported architectures. [!]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). Rpmlint ------- Checking: tuptime-5.0.2-4.fc36.noarch.rpm tuptime-5.0.2-4.fc36.src.rpm tuptime.noarch: W: spelling-error %description -l en_US uptime -> up time, up-time, suppertime tuptime.noarch: W: non-standard-uid /var/lib/tuptime _tuptime tuptime.noarch: W: non-standard-gid /var/lib/tuptime _tuptime tuptime.src: W: spelling-error %description -l en_US uptime -> up time, up-time, suppertime 2 packages and 0 specfiles checked; 0 errors, 4 warnings. Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/rfrail3/tuptime/archive/5.0.2.tar.gz : CHECKSUM(SHA256) this package : b7b5a47fd0935b03d2768beaf4542c9512b389c844c0a78795bcba38978099f2 CHECKSUM(SHA256) upstream package : b7b5a47fd0935b03d2768beaf4542c9512b389c844c0a78795bcba38978099f2 Requires -------- tuptime (rpmlib, GLIBC filtered): /bin/sh /usr/bin/bash /usr/bin/python3 shadow-utils systemd Provides -------- tuptime: tuptime Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review --no-colors --prebuilt --rpm-spec --name tuptime --mock-config /var/lib/copr-rpmbuild/results/configs/child.cfg Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api Disabled plugins: Perl, R, Java, Ocaml, fonts, Python, C/C++, Haskell, SugarActivity, PHP Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
@jkadlcik Thanks for that. I'll ask upstream about adding a %check and how we can test it. Strangely if I run fedora-review locally (on a F35 system) for the systemd issue, I no longer have an issue reported and get an extra check-box: ... [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: systemd_post is invoked in %post, systemd_preun in %preun, and systemd_postun in %postun for Systemd service files. Note: Systemd service file(s) in tuptime [x]: File names are valid UTF-8. ... That checkbox never appeared when I was using F34, despite fedora-review reporting as the same version.
> Strangely if I run fedora-review locally (on a F35 system) for the > systemd issue, I no longer have an issue reported and get an extra > check-box: True, I run fedora-review tool in Copr, and in F34 chroot it didn't generate any issue but in F35 and Fedora Rawhide chroots it did. I am not sure what caused it, I will try to investigate.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/tuptime
FEDORA-2022-48f9775054 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-48f9775054
FEDORA-2022-c5787c15b2 has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2022-c5787c15b2
FEDORA-EPEL-2022-87f5383f07 has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-87f5383f07
FEDORA-EPEL-2022-81f3551856 has been submitted as an update to Fedora EPEL 8. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-81f3551856
FEDORA-EPEL-2022-660ec9100e has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-660ec9100e
FEDORA-EPEL-2022-87f5383f07 has been pushed to the Fedora EPEL 9 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-87f5383f07 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2022-81f3551856 has been pushed to the Fedora EPEL 8 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-81f3551856 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2022-660ec9100e has been pushed to the Fedora EPEL 7 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-660ec9100e See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-c5787c15b2 has been pushed to the Fedora 34 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-c5787c15b2 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-c5787c15b2 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-48f9775054 has been pushed to the Fedora 35 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-48f9775054 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-48f9775054 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2022-87f5383f07 has been pushed to the Fedora EPEL 9 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-48f9775054 has been pushed to the Fedora 35 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-EPEL-2022-81f3551856 has been pushed to the Fedora EPEL 8 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-c5787c15b2 has been pushed to the Fedora 34 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-EPEL-2022-660ec9100e has been pushed to the Fedora EPEL 7 stable repository. If problem still persists, please make note of it in this bug report.