Bug 665995 - Review Request: fmit - Free Music Instrument Tuner
Review Request: fmit - Free Music Instrument Tuner
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Volker Fröhlich
Fedora Extras Quality Assurance
:
: fmit 500277 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-28 05:06 EST by Damian Wrobel
Modified: 2011-03-05 17:58 EST (History)
8 users (show)

See Also:
Fixed In Version: fmit-0.99.2-1.fc13
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-03-02 21:51:18 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
volker27: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Damian Wrobel 2010-12-28 05:06:26 EST
Spec URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/fmit.spec
SRPM URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/fmit-0.98.1-1.fc13.src.rpm
Description: fmit is a graphical utility for tuning musical instruments, with
error and volume history and advanced features like waveform shape,
harmonics ratio (formants), and micro-tonal tuning.
Comment 1 Jason Tibbitts 2010-12-28 13:19:10 EST
*** Bug 500277 has been marked as a duplicate of this bug. ***
Comment 2 Jason Tibbitts 2010-12-28 13:19:27 EST
*** Bug 459455 has been marked as a duplicate of this bug. ***
Comment 3 Volker Fröhlich 2010-12-28 16:32:19 EST
I think, you should call make %{?_smp_mflags} in the build section.

It seems, the flag BUILD_SHARED_LIBS doesn't exist any more.

I suppose, you'll also have to use %find_lang: http://fedoraproject.org/wiki/Packaging/Guidelines#Why_do_we_need_to_use_.25find_lang.3F
Comment 4 Damian Wrobel 2010-12-28 18:00:08 EST
(In reply to comment #3)
> I think, you should call make %{?_smp_mflags} in the build section.
Corrected.

> 
> It seems, the flag BUILD_SHARED_LIBS doesn't exist any more.
It's seems to be necessary, otherwise the program expected internal libraries to be installed on the system and it simply didn't work.

> 
> I suppose, you'll also have to use %find_lang:
> http://fedoraproject.org/wiki/Packaging/Guidelines#Why_do_we_need_to_use_.25find_lang.3F
I've disabled installation of the .ts file until upstream will produce properly the .qm files.

Spec URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/fmit.spec
SRPM URL:
http://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/fmit-0.98.1-2.fc13.src.rpm
Comment 5 Volker Fröhlich 2010-12-28 18:30:37 EST
.qm files can be created from .ts with Qt's lrelease, in case that helps.

OK, BUILD_SHARED_LIBS is a Cmake flag, I only looked into the files. Setting it to "off" creates static libraries, which should be avoided: http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
Comment 6 Damian Wrobel 2010-12-29 03:20:37 EST
(In reply to comment #5)
> 
> OK, BUILD_SHARED_LIBS is a Cmake flag, I only looked into the files. Setting it
> to "off" creates static libraries, which should be avoided:
> http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries

The package doesn't ship any static libraries.
Comment 7 Volker Fröhlich 2010-12-29 14:46:10 EST
You are right, it therefore isn't necessary to have a shared lib.

To avoid -DBUILD_SHARED_LIBS=OFF, three CMakeLists.txt must be modified. They should declare the libraries static: Just add "STATIC" to ADD_LIBRARY as shown below:

ADD_LIBRARY(fmit_modules ${fmit_MODULE_SRCS_MOC} ${fmit_MODULE_SRCS})
ADD_LIBRARY(fmit_modules STATIC ${fmit_MODULE_SRCS_MOC} ${fmit_MODULE_SRCS})

I don't think it is necessary to finish the review though. Please inform upstream about it.
Comment 8 Damian Wrobel 2010-12-29 15:15:03 EST
(In reply to comment #7)

Volker, I have already a patch prepared for that[1] as I've even considered to inform the upstream but the problem is that I'm not sure whether the author should change anything. IMHO the makefiles are written properly and it looks that the cmake add_library[2] works in that way that someone can change the behaviour defining the BUILD_SHARED_LIBS variable (using default values everything works properly). The only doubt I have is why the /etc/rpm/macros.cmake overwrites it unconditionally? Thus to avoid patching something which is correct I prefer to just revert the BUILD_SHARED_LIBS to its default value.

[1]. http://dwrobel.fedorapeople.org/projects/patches/fmit/fmit-add-library-static.patch
[2]. http://www.cmake.org/cmake/help/cmake-2-8-docs.html#command:add_library
Comment 9 Kevin Kofler 2010-12-29 17:40:35 EST
If a library should always be built as static, which is the case for those convenience libraries, you MUST use the STATIC keyword in add_library. The default is determined by the distribution. We default to shared libraries because we generally want libraries to be shared when both options exist (which should really be the case for all libraries).


For those libraries which do make sense to be shared, they should be properly versioned with set_target_properties (the VERSION and SOVERSION properties must be set properly), and you should not specify STATIC or SHARED explicitly, this is supposed to be decided by whoever builds the software.

IMHO the fact that CMake defaults to static is broken: When both options exist, the default should be shared, at least on GNU/Linux. (In the absence of dependency resolution, static libraries can be useful. In its presence, not really.) But this is exactly why we set BUILD_SHARED_LIBS. But this is for true libraries, which are installed as such.


In this case, the libraries are only convenience libraries and not actually installed, so it doesn't make sense to build them shared. Therefore, the add_library call must use STATIC explicitly.
Comment 10 Kevin Kofler 2010-12-29 17:54:08 EST
In short:
* if you DON'T want to install your library for other programs to use:
  - use add_library(STATIC …) to make it always static
  - ask whether that stuff should be built as a library in the first place,
    it could be all put into the same executable target
* if you want to install your library for other programs to use:
  - use add_library without an explicit STATIC or SHARED qualifier
  - use set_target_properties to set a SOVERSION and a full VERSION;
    in particular, bump SOVERSION whenever you break binary compatibility
  - consider using:
    SET(BUILD_SHARED_LIBS ON CACHE BOOL
        "Build shared libraries (.dll/.so) instead of static ones (.lib/.a)")
    to change the default (but either way, this can be set by the user,
    "set" just fixes the default)
* DO NOT ASSUME that the default is STATIC (and in particular, also DO NOT
  force SHARED in add_library unless the library really CANNOT be static;
  use set(BUILD_SHARED_LIBS ON) instead if that's what you want; that way,
  it can be overridden by the user).
Comment 11 Damian Wrobel 2010-12-30 04:52:10 EST
(In reply to comment #9)
I generally share this idea, but maybe it's worth to spread aforementioned guidelines among cmake based projects just to minimize the amount of potential modifications necessary to incorporate particular project into a distribution.

In other words it looks that it's not so obvious how to write a fully portable c-makefiles based only on the cmake documentation.


Updated spec with the patch as per comment #8.
Spec URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/fmit.spec
SRPM URL:
http://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/fmit-0.98.1-3.fc13.src.rpm
Comment 12 Volker Fröhlich 2011-01-13 21:04:07 EST
The licenses for the two convenience libraries are LGPL 2.1+.
Comment 13 Damian Wrobel 2011-01-17 07:57:37 EST
(In reply to comment #12)
Do you see any other issues? I'd prefer to fix them at one time.
Comment 14 Kevin Kofler 2011-01-17 11:32:19 EST
In CaptureThreadImplALSA.cpp, please change the hardcoded:
	m_source = "hw:0";
to:
	m_source = "default";
so that software mixers such as PulseAudio or dsnoop (dmix capture) work.

It would also be helpful to allow this to be configured by the user, but "default" would at least be an acceptable default ("hw:0" is not).
Comment 15 Kevin Kofler 2011-01-17 11:36:36 EST
(Well, I'm not sure whether it actually is user-settable or not, but in any case the default should be "default", not "hw:0".)
Comment 16 Volker Fröhlich 2011-01-17 13:38:33 EST
[+] Good
[X] Needs work
[0] Does not apply

MUST:
=====

[+] rpmlint:

fmit.x86_64: W: spelling-error %description -l en_US formants -> formant, formats, form ants
fmit.x86_64: W: no-manual-page-for-binary fmit
2 packages and 1 specfiles checked; 0 errors, 2 warnings.

[+] Naming according to the Package Naming Guidelines
[+] Spec file matches base package name
[X] Packaging guidelines met

Preserve timestamps:

install -Dpm 0644 %{SOURCE1} %{buildroot}%{_datadir}/pixmaps/%{name}.png

[+] License approved for Fedora
[X] License field in spec matches

See comment 12

[+] License included, if source package includes it
[+] Spec in American English
[+] Spec is legible
[+] Sources match upstream md5sum: 707f062542cef1f3176d347c4f594b52
[+] Compiles and builds into binary RPMs on at least one primary architecture:
[0] ExcludeArch is specified and commented
[0] Locales are handled correctly
[+] All build dependencies listed
[0] Calls ldconfig for its shared libraries
[+] No bundled system libraries
[0] Stated as relocatable package
[+] Owns all its directories or requires package that does
[+] No file listing duplicates
[+] File permissions correct
[+] Consistent use of macros
[+] Code or permissable content
[0] Large documentation in -doc subpackage
[+] No runtime dependency of files listed as %doc
[0] Header files in -devel subpackage
[0] Static files in -static subpackage
[0] Library files without suffix in -devel subpackage
[0] Devel-package requires base package
[0] No .la libtool archives
[+] GUI application includes properly installed %{name}.desktop file
[+] No files or directories owned, that other packages own
[+] Filenames in packages are UTF-8

SHOULD:
=======

[0] Query upstream if no license text is included
[+] Package builds in mock
[X] Package works as described

See comment at the bottom!

[0] Scriptlets are sane, if used
[0] Subpackages other than -devel should require base package (versioned)
[0] pkgconfig files in -devel subpackage
[0] Dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself
[0] Contain man pages, where they make sense

COMMENT:
========

Patching to "default" seems mandatory, since I can reproduce a crash otherwise:

.) Start FMIT
.) Configure --> Change "hw:0" to "default"
.) Working fine
.) Open configuration dialog again
.) Crash

NO MUST:
=======

Remove "VERBOSE=1", since it is the default for %cmake.

Did you put "Music Instrument Tuner" instead of "Free Music Instrument Tuner" in the desktop file on purpose
Comment 17 Martin Gieseking 2011-01-17 14:06:49 EST
> [X] License field in spec matches
> 
> See comment 12

The license field is fine as it is. Since the libraries are statically linked to the executable, we get a binary licensed under GPLv2+ (GPLv2+ + LGPLv2+ = GPLv2+).
Comment 18 Damian Wrobel 2011-01-18 15:32:15 EST
(In reply to comment #16)
> Preserve timestamps:
> 
> install -Dpm 0644 %{SOURCE1} %{buildroot}%{_datadir}/pixmaps/%{name}.png
Fixed.

> 
> Patching to "default" seems mandatory, since I can reproduce a crash otherwise:
> 
> .) Start FMIT
> .) Configure --> Change "hw:0" to "default"
> .) Working fine
> .) Open configuration dialog again
> .) Crash
> 
The problem is that I've verified it on two different machines and the program
crashes for me using this scenario in both cases - without the patch and with the patch as well.

If I changed the hw:0 to default also in the ConfigForm.ui file the program even crashes every time during the startup.

On the other hand author can't reproduce the problem but has some suspicion what can cause it and promised to fix it shortly.

As the program works in most typical scenarios and even if the user would faced similar problem there is a simple workaround to recover from that situation by simply deleting the ~/.qt/fmit009700rc file I would prefer either to not apply that patch or to wait for a new version.

> NO MUST:
> =======
> 
> Remove "VERBOSE=1", since it is the default for %cmake.

Removed - assuming that the /etc/rpm/macros.cmake will keep it on - originally it was taken from the cmake packaging guideline (http://fedoraproject.org/wiki/Packaging/cmake).

> 
> Did you put "Music Instrument Tuner" instead of "Free Music Instrument Tuner"
> in the desktop file on purpose

Yes, I did.


Updated spec and SRPM:
Spec URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/fmit.spec
SRPM URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/fmit-0.98.1-4.fc13.src.rpm
Comment 19 Volker Fröhlich 2011-01-20 08:18:30 EST
OK, all issues besides the crash are addressed now.

If you want to, you can put FMIT into Rawhide. Or wait for Upstream to solve the problem -- as you like!

--------
APPROVED
--------
Comment 20 Damian Wrobel 2011-01-27 17:33:32 EST
Volker, 
Please find an updated version based on the qt4 as well as with the fix for the crash. If possible, please confirm whether it's working now on your environment.

Updated spec and SRPM:
Spec URL: http://dwrobel.fedorapeople.org/projects/rpmbuild/SPECS/fmit.spec
SRPM URL:
http://dwrobel.fedorapeople.org/projects/rpmbuild/SRPMS/fmit-0.99.2-1.fc13.src.rpm
Comment 21 Volker Fröhlich 2011-01-27 18:48:50 EST
Yes, works perfectly fine for me.

One thing I noticed though:

-Start FMIT and open the configuration window
-Close FMIT
-The configuration windows stays alive

Maybe you can tell upstream.
Comment 22 Damian Wrobel 2011-01-28 08:24:39 EST
(In reply to comment #21)
> Yes, works perfectly fine for me.
Thanks for review and testing, if there is no other issues I'll request for SCM.

> Maybe you can tell upstream.
Upstream has been informed.
Comment 23 Volker Fröhlich 2011-01-29 17:39:38 EST
Thank you, please proceed.
Comment 24 Damian Wrobel 2011-01-31 07:43:30 EST
New Package SCM Request
=======================
Package Name: fmit
Short Description: Free Music Instrument Tuner
Owners: dwrobel
Branches: f13 f14 f15
InitialCC:
Comment 25 Jason Tibbitts 2011-01-31 11:48:48 EST
There are two issues with your request:

1) It is too early to request f15 branches.
2) fmit already exists in the package database.

Are you requesting that the existing package be unretired?
Comment 26 Damian Wrobel 2011-01-31 12:34:29 EST
(In reply to comment #25)
> There are two issues with your request:
> 
> 1) It is too early to request f15 branches.
OK, please skip f15.

> 2) fmit already exists in the package database.
It's probably the result of the bug #459455, comment #19?

> Are you requesting that the existing package be unretired?
I'm not sure how to classify it, for me this package is neither retired nor orphaned (see bug #459455, comment #26, or your last comment in the bug #500277, comment #10). In other words I couldn't find it neither on orphaned[1] nor retired[2] list.

I would appreciate your help, how to proceed.


[1]. https://admin.fedoraproject.org/pkgdb/acls/orphans
[2]. http://fedoraproject.org/wiki/PackageMaintainers/RetiredPackages
Comment 27 Kevin Kofler 2011-01-31 12:52:20 EST
The previously reviewed package never got imported, and it got orphaned, then retired in pkgdb. So the package should be unretired, f13 and f14 branches created if they're not already there, and ownership on all active branches (devel, f14, f13) reassigned to dwrobel.
Comment 28 Jason Tibbitts 2011-01-31 13:46:46 EST
I've unretired the package and added the two new branches.  Please verify that everything is set up properly in pkgdb.
Comment 29 Damian Wrobel 2011-01-31 15:13:38 EST
The f13 and f14 branches seems to be missing:

git branch -r
  origin/HEAD -> origin/master
  origin/f10/master
  origin/f11/master
  origin/f9/master
  origin/master
Comment 30 Kevin Fenzi 2011-02-03 15:20:27 EST
Try now?
Comment 31 Damian Wrobel 2011-02-03 16:10:27 EST
Now it's ok, thanks.
Comment 32 Fedora Update System 2011-02-24 15:28:28 EST
fmit-0.99.2-1.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/fmit-0.99.2-1.fc13
Comment 33 Fedora Update System 2011-02-24 15:29:30 EST
fmit-0.99.2-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/fmit-0.99.2-1.fc14
Comment 34 Damian Wrobel 2011-02-24 15:49:55 EST
Package Change Request
======================
Package Name: fmit
New Branches: f15
Owners: dwrobel
Comment 35 Fedora Update System 2011-02-25 03:21:30 EST
fmit-0.99.2-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 fmit'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/fmit-0.99.2-1.fc13
Comment 36 Jason Tibbitts 2011-02-25 12:05:35 EST
Git done (by process-git-requests).
Comment 37 Fedora Update System 2011-02-25 16:01:40 EST
Package fmit-0.99.2-1.fc15:
* should fix your issue,
* was pushed to the Fedora 15 updates-testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing fmit-0.99.2-1.fc15'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/fmit-0.99.2-1.fc15
then log in and leave karma (feedback).
Comment 38 Fedora Update System 2011-03-02 21:50:59 EST
fmit-0.99.2-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 39 Fedora Update System 2011-03-05 17:56:29 EST
fmit-0.99.2-1.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 40 Fedora Update System 2011-03-05 17:58:29 EST
fmit-0.99.2-1.fc13 has been pushed to the Fedora 13 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.