Bug 839527 - Review Request: rtirq - realtime IRQ threading
Review Request: rtirq - realtime IRQ threading
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraAudio
  Show dependency treegraph
 
Reported: 2012-07-12 04:36 EDT by Brendan Jones
Modified: 2012-10-10 01:02 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-10-10 01:02:04 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Brendan Jones 2012-07-12 04:36:49 EDT
rtirq - realtime IRQ threading. rtirq is currentlp packaged in Planet CCRMA and is used quite widely for production use for preemptive/realtime kernels.

Threadirqs parameter now makes it possible to use this script with stock kernels.

This is a requirement for the Fedora Audio Spin.

SRPM: http://bsjones.fedorapeople.org/rtirq-20120505-2.fc17.src.rpm
SPEC: http://bsjones.fedorapeople.org/rtirq.spec

Rpmlint output:

rtirq.src: W: spelling-error Summary(en_US) Realtime -> Mealtime, Real time, Real-time
rtirq.src: W: spelling-error %description -l en_US realtime -> mealtime, real time, real-time
rtirq.noarch: W: spelling-error Summary(en_US) Realtime -> Mealtime, Real time, Real-time
rtirq.noarch: W: spelling-error %description -l en_US realtime -> mealtime, real time, real-time
rtirq.noarch: W: no-documentation
rtirq.noarch: W: non-conffile-in-etc /etc/udev/rules.d/95-rtirq.rules
rtirq.noarch: W: no-manual-page-for-binary rtirq-udev
rtirq.noarch: W: service-default-enabled /etc/rc.d/init.d/rtirq
rtirq.noarch: W: service-default-enabled /etc/rc.d/init.d/rtirq
rtirq.noarch: E: subsys-not-used /etc/rc.d/init.d/rtirq
2 packages and 0 specfiles checked; 1 errors, 9 warnings.

I will suggest upstream to release as systemd enabled.
This is not a daemon so subsys lock is not required.
service-default-enabled is intended
Comment 1 Peter Lemenkov 2012-07-19 08:47:25 EDT
I'll review it.
Comment 2 Peter Lemenkov 2012-07-19 15:30:18 EDT
* I really don't like the idea to introduce another SysVinit-script. It won't be easy to switch to systemd later. You may use the following approach which is used in libvirt-clients

==============

[Unit]
Description=Realtime IRQ thread tunning
After=sound.target alsa-restore.service

[Service]
EnvironmentFile=-/etc/sysconfig/rtirq
ExecStart=/usr/bin/rtirq.sh start
ExecStop=/usr/bin/rtirq.sh stop

[Install]
WantedBy=multi-user.target

==============


* Installation path for udev rules is wrong. In fact you should install them into /usr/lib/udev/rules.d unless you're installing them on EL5. Since EL6 and Fedora 12 (afaik) /etc/udev/rules.d is intended for udev-rules override by the sysadmin.
Comment 3 Brendan Jones 2012-07-19 17:41:29 EDT
(In reply to comment #2)
> * I really don't like the idea to introduce another SysVinit-script. It
> won't be easy to switch to systemd later. You may use the following approach
> which is used in libvirt-clients

I thought long and hard about this whilst I was preparing this for review but am hesitant to do this - I'm sure Rui upstream will get there eventually and I'd prefer to leave it up to him (he's started packaging for Suse so I'm sure he'll get there).  As this is moving from CCRMA I'm also very cognizant of shifting the ground under the feet of current users.

Copied Fernando for comment
> 
> ==============
> 
> [Unit]
> Description=Realtime IRQ thread tunning
> After=sound.target alsa-restore.service
> 
> [Service]
> EnvironmentFile=-/etc/sysconfig/rtirq
> ExecStart=/usr/bin/rtirq.sh start
> ExecStop=/usr/bin/rtirq.sh stop
> 
> [Install]
> WantedBy=multi-user.target
> 
> ==============
> 
> 
> * Installation path for udev rules is wrong. In fact you should install them
> into /usr/lib/udev/rules.d unless you're installing them on EL5. Since EL6
> and Fedora 12 (afaik) /etc/udev/rules.d is intended for udev-rules override
> by the sysadmin.
Comment 4 Fernando Lopez-Lezcano 2012-07-19 23:45:58 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > * I really don't like the idea to introduce another SysVinit-script. It
> > won't be easy to switch to systemd later. 

Why is that?

> You may use the following approach
> > which is used in libvirt-clients
> 
> I thought long and hard about this whilst I was preparing this for review
> but am hesitant to do this - I'm sure Rui upstream will get there eventually
> and I'd prefer to leave it up to him (he's started packaging for Suse so I'm
> sure he'll get there).  As this is moving from CCRMA I'm also very cognizant
> of shifting the ground under the feet of current users.
> 
> Copied Fernando for comment

I would not mind moving to systemd but Rui should be in the loop, I think. I considered it when rebuilding for fc17 but I had no time at all to learn systemd. 

> > ==============
> > 
> > [Unit]
> > Description=Realtime IRQ thread tunning
> > After=sound.target alsa-restore.service

Why would alsa-restore.service be a dependency? (if that is the term). Rtirq should happen after all kernel sound drivers are loaded and that's all it needs, I think. 

> > [Service]
> > EnvironmentFile=-/etc/sysconfig/rtirq
> > ExecStart=/usr/bin/rtirq.sh start
> > ExecStop=/usr/bin/rtirq.sh stop

There is also a status target that lists the current irq ordering. 

> > [Install]
> > WantedBy=multi-user.target
> > 
> > ==============
Comment 5 Peter Lemenkov 2012-07-20 06:28:56 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > * I really don't like the idea to introduce another SysVinit-script. It
> > won't be easy to switch to systemd later. You may use the following approach
> > which is used in libvirt-clients
> 
> I thought long and hard about this whilst I was preparing this for review
> but am hesitant to do this - I'm sure Rui upstream will get there eventually
> and I'd prefer to leave it up to him (he's started packaging for Suse so I'm
> sure he'll get there).  As this is moving from CCRMA I'm also very cognizant
> of shifting the ground under the feet of current users.

Ok, understood. I won't insist here.

> > * Installation path for udev rules is wrong. In fact you should install them
> > into /usr/lib/udev/rules.d unless you're installing them on EL5. Since EL6
> > and Fedora 12 (afaik) /etc/udev/rules.d is intended for udev-rules override
> > by the sysadmin.

How about this one? ^^^
Comment 6 Peter Lemenkov 2012-07-20 06:36:15 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > * I really don't like the idea to introduce another SysVinit-script. It
> > > won't be easy to switch to systemd later. 
> 
> Why is that?

It will require some %trigger magic in the spec-file. Something like this:

============
%if 0%{?fedora} > 16
%triggerun -- %{name} < 1.7.2-1
# Save the current service runlevel info
# User must manually run systemd-sysv-convert --apply opensips
# to migrate them to systemd targets
/usr/bin/systemd-sysv-convert --save %{name} >/dev/null 2>&1 ||:

# Run these because the SysV package being removed won't do them
/sbin/chkconfig --del %{name} >/dev/null 2>&1 || :
/bin/systemctl try-restart %{name}.service >/dev/null 2>&1 || :
%endif
============

It's not a blocker anyway - I just gave an advice to Brendan to avoid this right from the start.

> I would not mind moving to systemd but Rui should be in the loop, I think. I
> considered it when rebuilding for fc17 but I had no time at all to learn
> systemd. 

Ok, let's wait for Rui then. It's not a blocker issue.

> > > ==============
> > > 
> > > [Unit]
> > > Description=Realtime IRQ thread tunning
> > > After=sound.target alsa-restore.service
> 
> Why would alsa-restore.service be a dependency? (if that is the term). Rtirq
> should happen after all kernel sound drivers are loaded and that's all it
> needs, I think. 

I'm not familiar with the application - I just thought it's a good idea to start it right after the init of ALSA. I'm sure there is a lot of room for improvements so consider my systemd script only as a starting point.

> > > [Service]
> > > EnvironmentFile=-/etc/sysconfig/rtirq
> > > ExecStart=/usr/bin/rtirq.sh start
> > > ExecStop=/usr/bin/rtirq.sh stop
> 
> There is also a status target that lists the current irq ordering. 

This should be handled differently. Here is a possible solution for custom SysVinit commands:

* http://thread.gmane.org/gmane.linux.redhat.fedora.devel/166326
Comment 7 Peter Lemenkov 2012-07-23 15:05:39 EDT
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > * I really don't like the idea to introduce another SysVinit-script. It
> > > won't be easy to switch to systemd later. You may use the following approach
> > > which is used in libvirt-clients
> > 
> > I thought long and hard about this whilst I was preparing this for review
> > but am hesitant to do this - I'm sure Rui upstream will get there eventually
> > and I'd prefer to leave it up to him (he's started packaging for Suse so I'm
> > sure he'll get there).  As this is moving from CCRMA I'm also very cognizant
> > of shifting the ground under the feet of current users.
> 
> Ok, understood. I won't insist here.
> 
> > > * Installation path for udev rules is wrong. In fact you should install them
> > > into /usr/lib/udev/rules.d unless you're installing them on EL5. Since EL6
> > > and Fedora 12 (afaik) /etc/udev/rules.d is intended for udev-rules override
> > > by the sysadmin.
> 
> How about this one? ^^^

Brendan, ping!
Comment 8 Brendan Jones 2012-07-23 15:20:13 EDT
Thanks Peter,

I will provide an update in the next day or so.

At this stage I think I'll update the udev rules as you suggested and work with Rui upstream to move to systemd once the package is accepted.
Comment 9 Brendan Jones 2012-07-24 07:09:00 EDT
I have updated the package to relocate the udev rules.


SRPM: http://bsjones.fedorapeople.org/rtirq-20120505-3.fc17.src.rpm
SPEC: http://bsjones.fedorapeople.org/rtirq.spec
Comment 10 Peter Lemenkov 2012-07-24 08:02:19 EDT
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint is NOT silent

sulaco ~/rpmbuild/SPECS: rpmlint /home/petro/rpmbuild/SRPMS/rtirq-20120505-3.fc18.src.rpm /home/petro/rpmbuild/RPMS/noarch/rtirq-20120505-3.fc18.noarch.rpm
rtirq.src: W: spelling-error Summary(en_US) Realtime -> Mealtime, Real time, Real-time
rtirq.src: W: spelling-error %description -l en_US realtime -> mealtime, real time, real-time

^^^ False positives.

rtirq.src:86: W: libdir-macro-in-noarch-package (main package) %{_libdir}/udev/rules.d/95-rtirq.rules

^^^ This one is serious. Please use /usr/lib/ instead of %{_libdir} which value depends on a host architecture.

rtirq.noarch: W: spelling-error Summary(en_US) Realtime -> Mealtime, Real time, Real-time
rtirq.noarch: W: spelling-error %description -l en_US realtime -> mealtime, real time, real-time

^^^ False positives.

rtirq.noarch: W: only-non-binary-in-usr-lib

^^^ This should be ignored. It was triggered by the udev-rule (I personally believe that they must be relocated into /usr/share but we can't do much here).

rtirq.noarch: W: no-documentation
rtirq.noarch: W: no-manual-page-for-binary rtirq-udev

^^^ May be ignored.

rtirq.noarch: W: service-default-enabled /etc/rc.d/init.d/rtirq
rtirq.noarch: W: service-default-enabled /etc/rc.d/init.d/rtirq

^^^ Please explain these ones. Is it intentional that this script is on by default? If yes then it's not a blocker. If no - this must be fixed.
 
rtirq.noarch: E: subsys-not-used /etc/rc.d/init.d/rtirq

^^^ This message advises you to use /var/lock/subsys and it's a good idea in general. But considering that we will eventually switch to systemd instead I wouldn't invest my time in fixing this. So this may be omitted as well.

2 packages and 0 specfiles checked; 1 errors, 10 warnings.
sulaco ~/rpmbuild/SPECS: 

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (GPLv2 or later)

- The file, containing the text of the license(s) for the package, MUST be included in %doc. Please do.

+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum rtirq-20120505.tar.gz*
254371e5bf812fd66668eb06417b48f0739aba36c62abe319a51c24ccbc22cb9  rtirq-20120505.tar.gz
254371e5bf812fd66668eb06417b48f0739aba36c62abe319a51c24ccbc22cb9  rtirq-20120505.tar.gz.1
sulaco ~/rpmbuild/SOURCES: 

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.
+ All non-default build dependencies are listed in BuildRequires. None actually.
0 No need to handle locales.
0 No shared library files.
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). This is not required for Fedora anymore - only for EL5 (and maybe for EL6).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files with a suffix (e.g. libfoo.so.1.1).
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT). This is not required for Fedora anymore - only for EL5 (and maybe for EL6).
+ All filenames in rpm packages are valid UTF-8.

Ok, so - please address issue with %{_libdir} macro (it's not allowed to use it in noarch-packages), mark LICENSE as %doc, explain/fix all other issues mentioned by me, and I'll finish it.

And start switching to systemd asap - it's great! :)
Comment 11 Brendan Jones 2012-07-24 09:03:12 EDT
Thanks Peter.

OK - changed to %_prefix/lib . 

Default on is intended,

The service is not run as a daemon so no need for the subsys lock.

New SPEC and SRPM:


SRPM: http://bsjones.fedorapeople.org/rtirq-20120505-4.fc17.src.rpm
SPEC: http://bsjones.fedorapeople.org/rtirq.spec
Comment 12 Peter Lemenkov 2012-07-24 09:23:30 EDT
(In reply to comment #11)
> Thanks Peter.
> 
> OK - changed to %_prefix/lib . 
> 
> Default on is intended,
> 
> The service is not run as a daemon so no need for the subsys lock.
> 
> New SPEC and SRPM:
> 
> 
> SRPM: http://bsjones.fedorapeople.org/rtirq-20120505-4.fc17.src.rpm
> SPEC: http://bsjones.fedorapeople.org/rtirq.spec

Still missing LICENSE file in %doc - please add before importing. Otherwise looks good and I don't see any  other issues. This package is


APPROVED.


ps it would be great if you review this one in return - https://bugzilla.redhat.com/652629 . Sorry it's rather big but I really desperately need this to be reviewed.
Comment 13 Brendan Jones 2012-07-24 09:32:51 EDT
Will do. Thanks for the review!

New Package SCM Request
=======================
Package Name: rtirq
Short Description: realtime IRQ threading
Owners: bsjones
Branches: f16 f17
InitialCC:
Comment 14 Jon Ciesla 2012-07-24 09:56:42 EDT
Git done (by process-git-requests).
Comment 15 Fedora Update System 2012-07-28 02:52:35 EDT
rtirq-20120505-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rtirq-20120505-5.fc17
Comment 16 Fedora Update System 2012-07-28 20:52:12 EDT
rtirq-20120505-5.fc17 has been pushed to the Fedora 17 testing repository.
Comment 17 Peter Lemenkov 2012-09-12 06:04:21 EDT
Ping. Is it intentional what this package got stuck in updates-testing for more than a month?
Comment 18 Brendan Jones 2012-09-12 07:31:26 EDT
The package is intended for F18 but some people wanted to try it for F17.

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