Bug 483451 - Review Request: k3guitune - Musical instrument tuner
Review Request: k3guitune - Musical instrument tuner
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-01 09:04 EST by David Timms
Modified: 2010-07-19 00:40 EDT (History)
3 users (show)

See Also:
Fixed In Version: 1.01-4.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-13 09:58:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description David Timms 2009-02-01 09:04:34 EST
Spec URL: http://members.iinet.net.au/~timmsy/k3guitune/k3guitune.spec
SRPM URL: http://members.iinet.net.au/~timmsy/k3guitune/k3guitune-1.01-1.fc10.src.rpm
Description: 
3Guitune is a guitar-and-other-instruments tuner. It takes a signal from the 
microphone, calculates its frequency, and displays it on a note scale 
graphic and an oscilloscope. It supports normal, Wien, and physical tuning.
===
rpmlint:
k3guitune.i386: W: dangling-symlink /usr/share/doc/HTML/de/k3guitune/common /usr/share/doc/HTML/de/common
k3guitune.i386: W: symlink-should-be-relative /usr/share/doc/HTML/de/k3guitune/common /usr/share/doc/HTML/de/common
k3guitune.i386: W: dangling-symlink /usr/share/doc/HTML/nl/k3guitune/common /usr/share/doc/HTML/nl/common
k3guitune.i386: W: symlink-should-be-relative /usr/share/doc/HTML/nl/k3guitune/common /usr/share/doc/HTML/nl/common
k3guitune.i386: W: dangling-symlink /usr/share/doc/HTML/en/k3guitune/common /usr/share/doc/HTML/en/common
k3guitune.i386: W: symlink-should-be-relative /usr/share/doc/HTML/en/k3guitune/common /usr/share/doc/HTML/en/common
3 packages and 0 specfiles checked; 0 errors, 6 warnings.
====
This package is not quite ready:
1. Need to work out what is going on with the locales stuff, and the rpmlint warnings above:
/usr/share/doc/HTML/de/k3guitune/common 
/usr/share/doc/HTML/de/common

/usr/share/doc/HTML/de/k3guitune/common 
/usr/share/doc/HTML/de/common
Any tips regarding locales in specs appreciated.

2. In the default audio driver=auto mode, no audio is detected changing this oss works, but also no audio detected. If you switch to alsa mode, the application segfaults:
http://fedora.pastebin.com/m4b5602f5

3. The patch for .desktop icon field doesn't match the name of the icon.
Comment 1 Orcan Ogetbil 2009-03-05 02:34:10 EST
1. First, replace those links with relative ones. This is all you need to do. To get a better understanding, look at the symlink
  /usr/share/doc/HTML/en/k3guitune/common --> ../common
Here, the symlink points to /usr/share/doc/HTML/en/common, which looks like dangling, but it isn't because /usr/share/doc/HTML/en/common belongs to the kdelibs-common which is in the dependency chain (it will be pulled up by kdelibs3). 

For the other language files, the symlinks will remain dangling until the user installs the relevant language package, for instance: kde-i18n-German
We don't require language packages explicitly, so we will have to ignore these rpmlints.

2- You need to add BR: alsa-lib-devel. Otherwise alsa support won't be compiled.
To fix the segfault use this patch:
   http://www.info-telecom.com/files/k3guitune-1.01-fftw.patch
which I found in 
   http://www.kde-apps.org/content/show.php/K3Guitune?content=15358

3- Can you fix the k3guitune-desktop-file.patch accordingly then?

Other than these, can you explain (as comments in the SPEC file) what the patches do and give links from the upstream tracking system?

Also, where does the xpm file come from? Any possible license issues? If not, this one also deserves some explanation in the SPEC file.
Comment 2 Orcan Ogetbil 2009-03-06 01:28:45 EST
I just packaged bio2jack. If you want to add jack support to k3guitune, check this out (bug# 488910)
Comment 3 David Timms 2009-03-07 20:18:11 EST
Oget, thanks for the initial review, updated spec/srpm for perusal at:
http://members.iinet.net.au/~timmsy/k3guitune/k3guitune.spec
http://members.iinet.net.au/~timmsy/k3guitune/k3guitune-1.01-2.fc10.src.rpm

(In reply to comment #1)
> 1. First, replace those links with relative ones. This is all you need to do.
The install part now:
- rm the 3x links
- creates new relative symbolic links

> 2- You need to add BR: alsa-lib-devel. Otherwise alsa support won't be
> compiled.
Done.

> To fix the segfault use this patch:
>    http://www.info-telecom.com/files/k3guitune-1.01-fftw.patch
> which I found in 
>    http://www.kde-apps.org/content/show.php/K3Guitune?content=15358
Patch included.

> 3- Can you fix the k3guitune-desktop-file.patch accordingly then?
OK, I've got that sorted now, the icon is displayed.

> Other than these, can you explain (as comments in the SPEC file) what the
> patches do and give links from the upstream tracking system?
Done.

> Also, where does the xpm file come from?
An earlier version of guitune, also GPL. Noted along side the patch definition.
=====
In running with debuginfo inside gdb:
$ gdb k3guitune
GNU gdb Fedora (6.8-29.fc10)
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i386-redhat-linux-gnu"...
(gdb) r
Starting program: /usr/bin/k3guitune 
shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
[Thread debugging using libthread_db enabled]
[New Thread 0xb7fd3700 (LWP 10675)]
Detaching after fork from child process 10682.
shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
kbuildsycoca running...
Couldn't initialize Arts: can't connect to aRts soundserver
[New Thread 0xb3d32b90 (LWP 10710)]
ALSA audio initialized succesfully
Audio driver initialized
[Thread 0xb3d32b90 (LWP 10710) exited]

Program exited normally.
(gdb) q
==
Is the shell-init - cannot access parent directories an issue ?

=====
In running from the command line:
$ k3guitune
Couldn't initialize Arts: can't connect to aRts soundserver
ALSA audio initialized succesfully
Audio driver initialized
==
It seems to take a few seconds to start up (for such a small program), even after it has just been running. Would it be worth trying to disable detection/connection to arts ?

=====
I noticed that configure has a look for bio2jack's presence. Would it be OK, to have a single version that is both alsa (which shows up, and record level control can be controlled in the pulseaudio volume control) and jack enabled, or would it be better to package it built for jack separately ?

I haven't looked in detail at what would be involved, it works for me as is, and would make it easier for Fedora music enthusiasts (who don't know/want to mess with the intricacies of jack) to use the tuner.
Comment 4 Orcan Ogetbil 2009-03-07 22:41:15 EST
Thanks for the update.

(In reply to comment #3)
> - rm the 3x links
> - creates new relative symbolic links
> 

You put one "../" too much. Also you may want update the script for symlinking to something like:
   # remove symlinks with absolute paths, and recreate with relative paths
   cd %{buildroot}/%{_docdir}/HTML/
   for lang in *; do
      ln -sf ../common $lang/%{name}/
   done
So that if upstream adds other languages you won't need to modify the script.

   

> =====
> In running with debuginfo inside gdb:
> $ gdb k3guitune
> GNU gdb Fedora (6.8-29.fc10)
> Copyright (C) 2008 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i386-redhat-linux-gnu"...
> (gdb) r
> Starting program: /usr/bin/k3guitune 
> shell-init: error retrieving current directory: getcwd: cannot access parent
> directories: No such file or directory
> [Thread debugging using libthread_db enabled]
> [New Thread 0xb7fd3700 (LWP 10675)]
> Detaching after fork from child process 10682.
> shell-init: error retrieving current directory: getcwd: cannot access parent
> directories: No such file or directory
> kbuildsycoca running...
> Couldn't initialize Arts: can't connect to aRts soundserver
> [New Thread 0xb3d32b90 (LWP 10710)]
> ALSA audio initialized succesfully
> Audio driver initialized
> [Thread 0xb3d32b90 (LWP 10710) exited]
> 
> Program exited normally.
> (gdb) q
> ==
> Is the shell-init - cannot access parent directories an issue ?
> 
> =====
> In running from the command line:
> $ k3guitune
> Couldn't initialize Arts: can't connect to aRts soundserver
> ALSA audio initialized succesfully
> Audio driver initialized
> ==
> It seems to take a few seconds to start up (for such a small program), even
> after it has just been running. Would it be worth trying to disable
> detection/connection to arts ?
> 

The program doesn't work for me with arts at all. No crashes though. It just behaves like it receives no sound. So I can't say anything for this. If you want to disable arts support, be my guest.

> =====
> I noticed that configure has a look for bio2jack's presence. Would it be OK, to
> have a single version that is both alsa (which shows up, and record level
> control can be controlled in the pulseaudio volume control) and jack enabled,
> or would it be better to package it built for jack separately ?
> 
> I haven't looked in detail at what would be involved, it works for me as is,
> and would make it easier for Fedora music enthusiasts (who don't know/want to
> mess with the intricacies of jack) to use the tuner.  

If you enable jack support, it won't mess up with the alsa support. You just add add BR: bio2jack-devel and another checkbox for jack fill appear on the audio settings dialog. It is not necessary to have jack support though. It would be just a bonus.



There are a few more things that I caught in the second pass.

* The desktop scriptlets need to be updated according to new guidelines
   https://fedoraproject.org/wiki/PackagingDrafts/Icon_Cache
   https://www.redhat.com/archives/fedora-devel-list/2009-February/msg01604.html
   https://www.redhat.com/archives/fedora-devel-list/2009-February/msg01273.html

* We need to preserve timestamps for the files: ChangeLog AUTHORS
You can use 
   touch -r $nonutffile $nonutffile.conv
for this purpose.

* We don't need to package the file INSTALL


I'll do the full review very soon.
Comment 5 Orcan Ogetbil 2009-03-07 23:58:14 EST
Actually, I spent some more time on this and finished the review. There are only a few more issues remaining:

* Please remove the binary .gmo files in %prep

* You don't need explicit Requires: hicolor-icon-theme. kdelibs3 will pick that up for you.

* %{_datadir}/doc/ must be replaced by %{_docdir}, especially in the %files section.

* The files
   acinclude.m4
   admin/*
   configure.in
are LGPL. But they don't get into the final binary RPM. But some source files under k3guitune directory are GPLv2+ and some are GPLv2 (no +). So the license tag should be "GPLv2 and GPLv2+".

! You can remove the commented lines that you won't need. e.g.
   #%{_datadir}/doc/HTML/*en/common

! Each package must consistently use macros. 
   -The SPEC file contains mixed instances of both %{name} and k3guitune. 
    The former is preferred. 
   
   -Also in this notation, you should use %{__install} instead of install.
Comment 6 David Timms 2009-03-10 08:22:27 EDT
(In reply to comment #5)
> Actually, I spent some more time on this and finished the review. There are
> only a few more issues remaining:
OK, great thanks for that, I think I got all changes, although I prefer that the URL and source paths use the actual name rather than the macro, to simplify being able to check that the paths really exist. If the name was changed, it would be unlikely to be on the same server etc, so I don't think that using the macro is appropriate.

Since the jack wrapper/lib is not yet in fedora, I haven't included that, but will look again once it does get in.

Update spec,srpm:
http://members.iinet.net.au/~timmsy/k3guitune/k3guitune.spec
http://members.iinet.net.au/~timmsy/k3guitune/k3guitune-1.01-3.fc10.src.rpm

Changes:
- mod icon cache update to use newly ratified scriptlets
- enable jack audio support via BR: bio2jack
- mod symlink fix to be more flexible in the future
- remove install of INSTALL
- mod to use more macro style invocations, except url and source
- del Requires: hicolor-icon-them since it wil be detected via kdelibs3
- clarify License tag since some source files are licensed GPLv2 only.
- fix previous changelog date
Comment 7 Orcan Ogetbil 2009-03-10 15:41:55 EDT
Thanks David. There is a little problem. We want to "touch" the original timestamp to the corrected file before overwriting the original. So these two lines:
   %{__mv} -f $nonutffile.conv $nonutffile
   touch -r $nonutffile $nonutffile.conv
must be in reverse order.

Otherwise k3guitune is good to go. Please do that change before you commit. I'm approving this now.

--------------------------------------------
This package (k3guitune) is APPROVED by oget
--------------------------------------------
Comment 8 David Timms 2009-03-10 17:43:24 EDT
(In reply to comment #7)
>    %{__mv} -f $nonutffile.conv $nonutffile
>    touch -r $nonutffile $nonutffile.conv
> must be in reverse order.
Good pickup. I missed following the ordering, and didn't check the end result.

New Package CVS Request
=======================
Package Name: k3guitune
Short Description: Musical instrument tuner
Owners: dtimms
Branches: F-9 F-10
InitialCC:
Comment 9 Kevin Fenzi 2009-03-12 22:37:32 EDT
cvs done.
Comment 10 Fedora Update System 2009-03-13 10:07:58 EDT
k3guitune-1.01-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/k3guitune-1.01-4.fc10
Comment 11 Fedora Update System 2009-03-13 10:08:04 EDT
k3guitune-1.01-4.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/k3guitune-1.01-4.fc9
Comment 12 Fedora Update System 2009-04-02 13:11:42 EDT
k3guitune-1.01-4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 13 Fedora Update System 2009-04-02 13:15:17 EDT
k3guitune-1.01-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 14 David Timms 2010-06-30 18:42:44 EDT
Package Change Request
======================
Package Name: k3guitune
New Branches: EL-4 EL-5
Owners: dtimms

Want to support epel, thanks.
Comment 15 Jason Tibbitts 2010-07-01 00:49:10 EDT
CVS done (by process-cvs-requests.py).
Comment 16 David Timms 2010-07-17 04:19:11 EDT
Package Change Request
======================
Package Name: k3guitune
New Branches: EL-6
Owners: dtimms
Comment 17 Kevin Fenzi 2010-07-19 00:40:41 EDT
CVS done (by process-cvs-requests.py).

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