Bug 517970 - Review Request: lingot-0.8.1 - musical instruments tuner
Summary: Review Request: lingot-0.8.1 - musical instruments tuner
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL: http://www.nongnu.org/lingot/
Whiteboard:
: 559668 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-08-18 08:43 UTC by Karel Volný
Modified: 2010-04-10 10:27 UTC (History)
5 users (show)

Fixed In Version: lingot-0.8.1-1.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-04-09 04:10:59 UTC
Type: ---
Embargoed:
martin.gieseking: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Karel Volný 2009-08-18 08:43:37 UTC
Spec URL: http://kvolny.fedorapeople.org/lingot.spec
SRPM URL: http://kvolny.fedorapeople.org/lingot-0.7.6-1.fc11.src.rpm
Description: LINGOT is a musical instrument tuner. It's accurate, easy to use, and highly configurable. Originally conceived to tune electric guitars, its configurability gives it a more general character.

it builds in koji - http://koji.fedoraproject.org/koji/taskinfo?taskID=1611459

rpmlint does not complain

two notes:

* I don't know of any better way how to run this than to use aoss - I've put it into the .desktop file, but running simply "lingot" from the commandline will fail ... I doubt this is worth adding README.Fedora; as it is graphical application, it is supposed to be run via the launcher (.desktop) file

* the package installs svg icon, but no cache updating is done - it seems that xdg-icon-resource works just with .png and .xpm while gtk-update-icon-cache is run on /user/share/icons and not on /usr/share/pixmaps

Comment 1 Jochen Schmitt 2009-08-18 15:28:28 UTC
Question:

Why do you not create a wrapper script for your application?

Comment 2 Karel Volný 2009-08-19 07:23:52 UTC
(In reply to comment #1)
> Question:
> 
> Why do you not create a wrapper script for your application?  

because I consider it overkill ... no problem starting the app from gui

but if you think it should be done, basically nothing prevents me from doing that

Comment 3 Martin Gieseking 2009-09-12 06:55:20 UTC
Here is my formal review of your package. I didn't find any serious things to be fixed. Just Requires: gtk2 should be removed as BuildRequires: gtk2-devel is present.
A wrapper script would be a nice addition but it's not a requirement to me. 


$ rpmlint /var/lib/mock/fedora-11-x86_64/result/lingot-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

---------------------------------
keys used in following checklist:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - GPLv2+ according to the source header comments

[+] MUST: The License field in spec file must match the actual license.
[+] MUST: File containing license text must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources must match the upstream source.
    md5 hash is e030c45af43e59521f6207ef5c9c5687

[+] MUST: The package MUST successfully compile and build into binary rpms
    - builds in koji
      https://koji.fedoraproject.org/koji/taskinfo?taskID=1672632

[.] MUST: If the package does not successfully compile,...

[X] MUST: All build dependencies must be listed in BuildRequires.
    - Requires: gtk2 is redundant   

[+] MUST: The spec file MUST handle locales properly. 
    - BuildRequires: gettext present
    - %find_lang present
    - %files -f %{name}.lang used
    
[.] MUST: Packages which store shared library files,...
    - no shared libs

[.] MUST: If the package is designed to be relocatable, ...
    - not relocatable

[+] MUST: A package must own all directories that it creates. 
[+] MUST: Files must not appear more than once in %files listings.
[+] MUST: Permissions on files must be set properly.
[+] MUST: %clean section must contain rm -rf %{buildroot}.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.

[.] MUST: Large documentation files must go in a -doc subpackage.
    - no large docs

[+] MUST: Files in %doc must not affect the runtime of the application.

[.] MUST: Header files must be in a -devel package.
    - no header files

[.] MUST: Static libraries must be in a -static package.
    - no static libs

[.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
    - no .pc files

[.] MUST: .so files without suffix must go in a -devel package.
    - no shared libs

[.] MUST: devel packages must require the base package.
    - no devel subpackage

[+] MUST: Packages must NOT contain any .la libtool archives.

[+] MUST: Packages containing GUI applications must include a %{name}.desktop file.
    - desktop file present
    - properly installed with desktop-file-validate
    - referenced svg file added to pixmaps folder


[+] MUST: Packages must not own files or directories already owned by other packages.

[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}.

[+] MUST: All filenames in rpm packages must be valid UTF-8.


[+] SHOULD: The reviewer should test that the package builds in mock.
    - builds in mock

[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
    - works as expected
    
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
    - no scriptlets necessary 

[.] SHOULD: subpackages should require the base package.
    - no subpackages

Comment 4 Karel Volný 2010-03-02 16:40:56 UTC
OOPS, I forgot about this one, shame on me!

thanks for the review

there is a new version 0.8.0 which does not require the wrapper, however, there is some problem in the audio input buffer initialisation that makes the core thread freeze

I've reported it on the devel mailinglist - stay tuned for the update (and kick me if there's no activity for a long time :-)

Comment 5 Martin Gieseking 2010-03-02 17:01:49 UTC
To me, it's not a problem since I was pretty busy in the last couple of month. :)
Just clear the Whitboard field above when the package is ready.

Comment 6 Karel Volný 2010-03-03 10:40:03 UTC
(In reply to comment #4)
> I've reported it on the devel mailinglist

and here's the link: http://lists.gnu.org/archive/html/lingot-devel/2010-03/msg00000.html

btw, I've found a duplicate request bug #559668

Comment 7 Thomas Spura 2010-03-03 11:09:39 UTC
*** Bug 559668 has been marked as a duplicate of this bug. ***

Comment 8 Karel Volný 2010-03-05 23:53:08 UTC
I have found that the bug occured in previous version too - http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=562425
(or at least it looks to me the same)

and lingot 0.8.0 works for me on another machine

so let's go on with this, knowing there is a bug to be fixed

I've updated the spec here:
http://kvolny.fedorapeople.org/lingot.spec
new srpm:
http://kvolny.fedorapeople.org/lingot-0.8.0-1.fc12.src.rpm
koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2033900

note that the new version no longer needs the aoss wrapper, so comments #1 and #2 can be forgotten

Comment 9 Martin Gieseking 2010-03-06 09:36:15 UTC
The package looks fine. However it fails to build for F-13 and rawhide because of the missing linker flag -lm:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2034423

You can solve this by patching the line
  AC_CHECK_LIB(m, sin) 
into configure.in. I think, this should also be reported upstream.

Comment 10 Karel Volný 2010-03-08 13:11:38 UTC
(In reply to comment #9)
> The package looks fine. However it fails to build for F-13 and rawhide because
> of the missing linker flag -lm:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=2034423
> 
> You can solve this by patching the line
>   AC_CHECK_LIB(m, sin) 
> into configure.in. I think, this should also be reported upstream.    

thanks, I've suggested the change upstream - I hope it can make it into 0.8.1 which is getting ready, so let's wait for it ... patching configure.in would require more changes

Comment 11 Karel Volný 2010-03-17 12:47:03 UTC
great news, seems that all the troubles are fixed in 0.8.1

preliminary builds:
dist-f12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2058208
dist-f13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2058226

- these packages are from mercurial sources, so there are some temporary changes in .spec, I'll do the proper version once the 0.8.1 update is released

Comment 12 Martin Gieseking 2010-03-17 13:01:00 UTC
That sounds promising. I'll review your updated package as soon as it's ready.

Comment 14 Martin Gieseking 2010-03-18 19:05:23 UTC
Here is the review of your latest package. This time, everything looks fine, so we can finish here.

$ rpmlint /var/lib/mock/fedora-12-i386/result/lingot-*.rpm
lingot.i686: W: spelling-error %description -l en_US configurability -> configuration, comparability, curability
lingot.src: W: spelling-error %description -l en_US configurability -> configuration, comparability, curability
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

The warnings of the spell checker can be ignored.

---------------------------------
keys used in following checklist:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: File containing the text of the license(s) must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL.
    $ md5sum lingot-0.8.1.tar.gz*
    ab46e142f47bfb8e8fd565517202e46a  lingot-0.8.1.tar.gz
    ab46e142f47bfb8e8fd565517202e46a  lingot-0.8.1.tar.gz.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.

[+] MUST: If the package does not successfully compile, ...
[+] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: The spec file MUST handle locales properly.
[.] MUST: Every binary RPM package (or subpackage) which stores shared library files ...
    - no shared libs

[.] MUST: If the package is designed to be relocatable, ...
    - not relocatable

[+] MUST: A package must own all directories that it creates.
[+] MUST: A Fedora package must not list a file more than once in the spec file's %files listings.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
    - no header files

[.] MUST: Static libraries must be in a -static package.
    - no static libs

[.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
    - no .pc files

[.] MUST: If a package contains library files with a suffix ...
[.] MUST: devel packages must require the base package 
[.] MUST: Packages must NOT contain any .la libtool archives. 
[+] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. 
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[+] SHOULD: The reviewer should test that the package builds in mock.
    - builds in mock

[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.

------------------------
The package is APPROVED.
------------------------

Comment 15 Karel Volný 2010-03-19 09:29:24 UTC
New Package CVS Request
=======================
Package Name: lingot
Short Description: A musical instruments tuner
Owners: kvolny ankursinha
Branches: F-12 F-13
InitialCC:

Comment 16 Kevin Fenzi 2010-03-19 19:38:13 UTC
CVS done (by process-cvs-requests.py).

Comment 17 Fedora Update System 2010-03-22 10:16:06 UTC
lingot-0.8.1-1.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/lingot-0.8.1-1.fc13

Comment 18 Fedora Update System 2010-03-22 10:24:56 UTC
lingot-0.8.1-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/lingot-0.8.1-1.fc12

Comment 19 Fedora Update System 2010-03-23 23:22:06 UTC
lingot-0.8.1-1.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update lingot'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/lingot-0.8.1-1.fc13

Comment 20 Fedora Update System 2010-03-23 23:38:41 UTC
lingot-0.8.1-1.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update lingot'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/lingot-0.8.1-1.fc12

Comment 21 Fedora Update System 2010-04-09 04:10:53 UTC
lingot-0.8.1-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 22 Fedora Update System 2010-04-10 10:27:09 UTC
lingot-0.8.1-1.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.


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