I'll review it.

* 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.

(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.

(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
> > 
> > ==============

(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? ^^^

(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

(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!

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.

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

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! :)

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

(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.

Will do. Thanks for the review!

New Package SCM Request
=======================
Package Name: rtirq
Short Description: realtime IRQ threading
Owners: bsjones
Branches: f16 f17
InitialCC:

Git done (by process-git-requests).

rtirq-20120505-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rtirq-20120505-5.fc17

rtirq-20120505-5.fc17 has been pushed to the Fedora 17 testing repository.

Ping. Is it intentional what this package got stuck in updates-testing for more than a month?

The package is intended for F18 but some people wanted to try it for F17.