Bug 1894605 - Review Request: realtime-tests - Suite of realtime tests including cyclictest
Summary: Review Request: realtime-tests - Suite of realtime tests including cyclictest
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jiri Kastner
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-11-04 15:49 UTC by John Kacur
Modified: 2020-12-16 06:46 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-12-16 06:46:36 UTC
Type: ---
Embargoed:
cz172638: fedora-review+


Attachments (Terms of Use)

Description John Kacur 2020-11-04 15:49:42 UTC
Spec URL: https://jkacur.fedorapeople.org/realtime-tests.spec
SRPM URL: https://jkacur.fedorapeople.org/realtime-tests-1.9-1.el8.src.rpm
Description: Programs that test various rt-features
Fedora Account System Username: jkacur

Comment 2 John Kacur 2020-11-04 16:01:47 UTC
This is my first fedora package and I need a sponsor

Comment 3 Robert-André Mauchin 🐧 2020-11-07 08:40:47 UTC
 - Not needed:

Group: Development/Tools

BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root

rm -rf $RPM_BUILD_ROOT
mkdir -p $RPM_BUILD_ROOT
mkdir -p $RPM_BUILD_ROOT/%{python3_sitelib}

%clean
rm -rf $RPM_BUILD_ROOT

%defattr(-,root,root,-)

 - Split your BR one per line:

BuildRequires: gcc
BuildRequires: numactl-devel
BuildRequires: python3-devel

 - 
make DESTDIR=$RPM_BUILD_ROOT prefix=/usr install → %make_install prefix=%{_prefix}

 - Use macros:

/usr/bin/ → %{_bindir}

/usr/share/man/ → %{_mandir}

 - Glob the manpages extension as the compression may change in the future + simplify the list

/usr/share/man/man8/*.8.*

 - I'm not specialist of these programs but could you justify this:

ExclusiveArch: x86_64

Comment 4 John Kacur 2020-11-11 17:04:07 UTC
I followed all of the review suggestions
- removed unnecessary parts of the spec file
- one BuildRequires per line
- modified make line to use _prefix macro
- used the _bindir and _mandir prefixes through out.

The updates are available here
https://jkacur.fedorapeople.org/realtime-tests-1.9-2.el8.src.rpm
https://jkacur.fedorapeople.org/realtime-tests.spec

The ExclusiveArch: x86_64 is because that is where the realtime kernel is fully supported.
Some of the programs in the suite could be made to work on other architectures that don't fully support the realtime kernel, but would just demonstrate poor results (poor latency)
but other programs in the suite would simply not work because of required missing functionality such as high resolution timers.

I would be willing to work to at least get future versions of all the programs compiled on all architectures and at least exiting with meaningful error messages,
but this would require some upstream work, so it would be nice to get an exception for this in the meantime.

Comment 6 Robert-André Mauchin 🐧 2020-11-12 15:27:03 UTC
 - This is 404: Source0: https://www.kernel.org/pub/linux/utils/rt-tests/%{name}-%{version}.tar.xz

   Correct URL would be:

Source0: https://www.kernel.org/pub/linux/utils/rt-tests/rt-tests-%{version}.tar.xz

   Which causes the following error:

+ cd realtime-tests-1.9
/var/tmp/rpm-tmp.bwMsBx: line 38: cd: realtime-tests-1.9: No such file or directory

  Use:
 
%setup -q -n rt-tests-%{version}


  Where did you get your archive from? It must match the SPEC.


 - make DESTDIR=$RPM_BUILD_ROOT prefix=%{_prefix} install → %make_install prefix=%{_prefix}

 - The syntax af the latest changelog entries is not correct: it's version-release not version.release

realtime-tests.x86_64: W: incoherent-version-in-changelog 1.9.2 ['1.9-2.fc34', '1.9-2']

 - Please pass Fedora defaults build flags.  Use:

%install
%set_build_flags


  - Build process should not gzip the manpages but let rpm handle it



Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[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]: 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "GNU General Public License, Version 2", "Unknown or
     generated", "GNU General Public License v2.0 or later [obsolete FSF
     postal address (Temple Place)]", "*No copyright* FSF All Permissive
     License". 66 files have unknown license. Detailed output of
     licensecheck in /home/bob/packaging/review/realtime-tests/review-
     realtime-tests/licensecheck.txt
[x]: %build honors applicable compiler flags or justifies otherwise.
[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.
[-]: Package contains systemd file(s) if in need.
[-]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[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 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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: 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).
[?]: 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.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[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]: Package should compile and build into binary rpms on all supported
     architectures.
[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).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Cannot parse rpmlint output:


Rpmlint (installed packages)
----------------------------
realtime-tests.x86_64: W: spelling-error %description -l en_US mutexes -> mutes, mutates, executes
realtime-tests.x86_64: W: incoherent-version-in-changelog 1.9.2 ['1.9-2.fc34', '1.9-2']
realtime-tests.x86_64: W: invalid-url URL git://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/cyclicdeadline
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/cyclictest
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/deadline_test
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/hackbench
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/oslat
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/pi_stress
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/pip_stress
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/pmqtest
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/ptsematest
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/queuelat
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/rt-migrate-test
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/signaltest
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/sigwaittest
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/ssdd
realtime-tests.x86_64: W: unstripped-binary-or-object /usr/bin/svsematest
realtime-tests.x86_64: W: no-manual-page-for-binary determine_maximum_mpps.sh
realtime-tests.x86_64: W: no-manual-page-for-binary get_cpuinfo_mhz.sh
1 packages and 0 specfiles checked; 0 errors, 20 warnings.

Comment 7 John Kacur 2020-11-19 06:37:58 UTC
Fedora has a package that is already named rt-tests but that rt stands for request-tracker instead of realtime
In order to avoid the name clash, we want to rename rt-tests to realtime-tests. It remains to be seen whether I can get the upstream community to go-along with this.
As the upstream maintainer I did push both rt-tests-1.9.tar.xz and realtime-tests-1.9.tar.xz to the the URL in the specfile.

This does allow spectool to correctly download the source from the specfile, even though 
fedpkg --release  f33 lint
Failed to get repository name from Git url or pushurl
still happens

I followed your suggests regarding
%setup
and fixed the version-release problems. (not sure how that happened)
and finally, I did this

%build
%set_build_flags
%make_build

for fedora flags

This time I built the source using fedpkg
fedpkg --release f33 local

and put source here
https://jkacur.fedorapeople.org/
and a koji build here
https://koji.fedoraproject.org/koji/taskinfo?taskID=55852014

Not sure what to do about the upstream Makefile gziping the manpages.
It seems to me that the upstream Makefile should be as distribution agnostic as possible
and that it is normal for it to zip the manpages

Thanks for your reviews, open to any suggestions you may have.

Comment 8 John Kacur 2020-11-21 06:29:35 UTC
I created a patch which I posted upstream that don't compress the man pages by default.
I added this patch to the spec file to create a new version of realtime-tests

You can view it here, with the spec file 

https://jkacur.fedorapeople.org/realtime-tests-1.9-4.fc33.src.rpm
https://jkacur.fedorapeople.org/realtime-tests.spec

Here is a link to the koji build

https://koji.fedoraproject.org/koji/taskinfo?taskID=55992610

Thanks!

Comment 9 John Kacur 2020-11-26 04:53:27 UTC
Ah, now see what you meant by 

   Correct URL would be:

Source0: https://www.kernel.org/pub/linux/utils/rt-tests/rt-tests-%{version}.tar.xz

   Which causes the following error:

+ cd realtime-tests-1.9
/var/tmp/rpm-tmp.bwMsBx: line 38: cd: realtime-tests-1.9: No such file or directory

  Use:
 
%setup -q -n rt-tests-%{version}


I have now made that change.

srpm and specfile here
https://jkacur.fedorapeople.org/

koji build here
https://koji.fedoraproject.org/koji/taskinfo?taskID=56265658

Comment 10 John Kacur 2020-11-26 05:23:00 UTC
I also did the latest build in COPR
https://copr.fedorainfracloud.org/coprs/jkacur/realtime-tests/

Comment 11 Jiri Kastner 2020-11-27 15:04:38 UTC
missing make in dependencies
escape '%description' on line 713 (change to %%description)

##

[root@8f0928087582 ~]# rpmlint rpmbuild/SPECS/realtime-tests.spec rpmbuild/RPMS/x86_64/realtime-tests-* realtime-tests-debugsource realtime-tests-debuginfo realtime-tests
realtime-tests.x86_64: W: spelling-error %description -l en_US mutexes -> mutes, mutates, executes
realtime-tests.x86_64: W: no-manual-page-for-binary determine_maximum_mpps.sh
realtime-tests.x86_64: W: no-manual-page-for-binary get_cpuinfo_mhz.sh
realtime-tests.x86_64: W: spelling-error %description -l en_US mutexes -> mutes, mutates, executes
realtime-tests.x86_64: W: no-manual-page-for-binary determine_maximum_mpps.sh
realtime-tests.x86_64: W: no-manual-page-for-binary get_cpuinfo_mhz.sh
6 packages and 1 specfiles checked; 0 errors, 6 warnings.

###

i tried those scripts and they do not offer help.
any plans on having some explanation of usage?

Comment 12 John Kacur 2020-11-27 16:13:48 UTC
(In reply to Jiri Kastner from comment #11)
> missing make in dependencies
> escape '%description' on line 713 (change to %%description)
> 
> ##
> 
> [root@8f0928087582 ~]# rpmlint rpmbuild/SPECS/realtime-tests.spec
> rpmbuild/RPMS/x86_64/realtime-tests-* realtime-tests-debugsource
> realtime-tests-debuginfo realtime-tests
> realtime-tests.x86_64: W: spelling-error %description -l en_US mutexes ->
> mutes, mutates, executes
> realtime-tests.x86_64: W: no-manual-page-for-binary determine_maximum_mpps.sh
> realtime-tests.x86_64: W: no-manual-page-for-binary get_cpuinfo_mhz.sh
> realtime-tests.x86_64: W: spelling-error %description -l en_US mutexes ->
> mutes, mutates, executes
> realtime-tests.x86_64: W: no-manual-page-for-binary determine_maximum_mpps.sh
> realtime-tests.x86_64: W: no-manual-page-for-binary get_cpuinfo_mhz.sh
> 6 packages and 1 specfiles checked; 0 errors, 6 warnings.
> 
> ###
> 
> i tried those scripts and they do not offer help.
> any plans on having some explanation of usage?

determine_maximum_mpps.sh is mentioned in the README in the src/queuelat dir.
I am planning on writing a short manpage and pushing it upstream soon.

get_cpuinfo_mhz.sh is a brief helper script for determine_maximum_mpps.sh
although you can run it standalone. I could also write a short manpage for it.

Comment 13 John Kacur 2020-12-04 20:38:43 UTC
I removed ExclusiveArch: x86_64
and replaced it with an ExcludeArch line that has architectures where the builds fail.
Currently builds fail where numactl is not available. It might be possible to work around this in the future.

The new src.rpm and spec file are here
https://jkacur.fedorapeople.org/realtime-tests-1.9-6.fc33.src.rpm
https://jkacur.fedorapeople.org/realtime-tests.spec

I also did a koji build
https://koji.fedoraproject.org/koji/taskinfo?taskID=56790256

Finally I did a copr build
https://copr.fedorainfracloud.org/coprs/jkacur/realtime-tests/

Let me know if there is anything else you need.

Comment 14 Jiri Kastner 2020-12-04 20:57:58 UTC
looks good, setting review to +

Comment 15 Jaroslav Škarvada 2020-12-09 19:44:36 UTC
John I will sponsor you. Few things I noted, please fix them:

- The following lines are not needed, the implicit deps generator is quite good:
%{?__python3:Requires: %{__python3}}
...
Requires: bash

- The following line is not needed, please drop it:
rm -rf %{buildroot}

- Please escape macros in the changelog, i.e.
in the changelog change %description to %%description, and also probably change %s to %%s and %ld to %%ld

Another thing I am just curious about:
- Isn't better (and safer) to just state that only root could run it instead of giving it caps?
%caps(cap_sys_rawio+ep) /usr/bin/cyclictest

Comment 16 Jaroslav Škarvada 2020-12-09 19:47:23 UTC
John you are sponsored now.

Comment 17 Gwyn Ciesla 2020-12-11 17:16:13 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/realtime-tests

Comment 18 John Kacur 2020-12-16 05:52:40 UTC
(In reply to Jaroslav Škarvada from comment #15)
> John I will sponsor you. Few things I noted, please fix them:
> 
> - The following lines are not needed, the implicit deps generator is quite
> good:
> %{?__python3:Requires: %{__python3}}
> ...
> Requires: bash
> 
> - The following line is not needed, please drop it:
> rm -rf %{buildroot}
> 
> - Please escape macros in the changelog, i.e.
> in the changelog change %description to %%description, and also probably
> change %s to %%s and %ld to %%ld
> 
> Another thing I am just curious about:
> - Isn't better (and safer) to just state that only root could run it instead
> of giving it caps?
> %caps(cap_sys_rawio+ep) /usr/bin/cyclictest

I removed 
%{?__python3:Requires: %{__python3}}
rm -rf %{buildroot}
and I escaped all the macros as you requested.

cyclictest uses caps to grant the user who joins the realtime group
access to smi registers in the analysis of realtime performance.


Note You need to log in before you can comment on or make changes to this bug.