Bug 665995

*** Bug 500277 has been marked as a duplicate of this bug. ***

*** Bug 459455 has been marked as a duplicate of this bug. ***

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

(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

.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

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

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.

(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

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.

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

(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

The licenses for the two convenience libraries are LGPL 2.1+.

(In reply to comment #12)
Do you see any other issues? I'd prefer to fix them at one time.

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

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

[+] 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

> [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+).

(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

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

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

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.

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

Thank you, please proceed.

New Package SCM Request
=======================
Package Name: fmit
Short Description: Free Music Instrument Tuner
Owners: dwrobel
Branches: f13 f14 f15
InitialCC:

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?

(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

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.

I've unretired the package and added the two new branches.  Please verify that everything is set up properly in pkgdb.

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

Try now?

Now it's ok, thanks.

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

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

Package Change Request
======================
Package Name: fmit
New Branches: f15
Owners: dwrobel

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

Git done (by process-git-requests).

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

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.

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.

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.