https://bugzilla.redhat.com/show_bug.cgi?id=839527
Bug ID: 839527 QA Contact: extras-qa@fedoraproject.org Severity: unspecified Version: rawhide Priority: unspecified CC: notting@redhat.com, package-review@lists.fedoraproject.org Assignee: nobody@fedoraproject.org Summary: Review Request: rtirq - realtime IRQ threading Regression: --- Story Points: --- Classification: Fedora OS: Unspecified Reporter: brendan.jones.it@gmail.com Type: Bug Documentation: --- Hardware: Unspecified Mount Type: --- Status: NEW Component: Package Review Product: Fedora
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
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Brendan Jones brendan.jones.it@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |805236 (FedoraAudio)
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Peter Lemenkov lemenkov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |lemenkov@gmail.com Assignee|nobody@fedoraproject.org |lemenkov@gmail.com Flags| |fedora-review?
--- Comment #1 from Peter Lemenkov lemenkov@gmail.com --- I'll review it.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #2 from Peter Lemenkov lemenkov@gmail.com --- * 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.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #3 from Brendan Jones brendan.jones.it@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #4 from Fernando Lopez-Lezcano nando@ccrma.stanford.edu --- (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
==============
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #5 from Peter Lemenkov lemenkov@gmail.com --- (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? ^^^
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #6 from Peter Lemenkov lemenkov@gmail.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Peter Lemenkov lemenkov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(brendan.jones.it@ | |gmail.com)
--- Comment #7 from Peter Lemenkov lemenkov@gmail.com --- (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!
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Brendan Jones brendan.jones.it@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(brendan.jones.it@ | |gmail.com) |
--- Comment #8 from Brendan Jones brendan.jones.it@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #9 from Brendan Jones brendan.jones.it@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #10 from Peter Lemenkov lemenkov@gmail.com --- 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! :)
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #11 from Brendan Jones brendan.jones.it@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Peter Lemenkov lemenkov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #12 from Peter Lemenkov lemenkov@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Brendan Jones brendan.jones.it@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #13 from Brendan Jones brendan.jones.it@gmail.com --- Will do. Thanks for the review!
New Package SCM Request ======================= Package Name: rtirq Short Description: realtime IRQ threading Owners: bsjones Branches: f16 f17 InitialCC:
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #14 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #15 from Fedora Update System updates@fedoraproject.org --- rtirq-20120505-5.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/rtirq-20120505-5.fc17
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #16 from Fedora Update System updates@fedoraproject.org --- rtirq-20120505-5.fc17 has been pushed to the Fedora 17 testing repository.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #17 from Peter Lemenkov lemenkov@gmail.com --- Ping. Is it intentional what this package got stuck in updates-testing for more than a month?
https://bugzilla.redhat.com/show_bug.cgi?id=839527
--- Comment #18 from Brendan Jones brendan.jones.it@gmail.com --- The package is intended for F18 but some people wanted to try it for F17.
https://bugzilla.redhat.com/show_bug.cgi?id=839527
Brendan Jones brendan.jones.it@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |NEXTRELEASE Last Closed| |2012-10-10 01:02:04
package-review@lists.fedoraproject.org