I'm not taking it for formal review for now, just got a few comments here.

> License:        GPLv3 or LGPLv2 with exceptions

The files in this tarball appear to have the same license text as Qt has. Fedora Qt package's license tag reads "LGPLv2 with exceptions or GPLv3 with exceptions". I'm not sure where exactly the GPLv3 exception is, though. rdieter is already in CC, maybe he can comment about that. In any case, if the license is the same, we should use the same license tag in both qt and in this package.

> $ rpm -qlp qtsingleapplication-devel-2.6-1.fc14.i686.rpm
> /usr/include/QtSolutions
> /usr/include/QtSolutions/QtSingleApplication
> /usr/include/QtSolutions/QtSingleCoreApplication
> /usr/include/QtSolutions/qtsingleapplication.h
> /usr/include/QtSolutions/qtsinglecoreapplication.h
> /usr/lib/libQtSolutions_SingleApplication-2.6.so

You have QtSingleCoreApplication header, but is the actual library missing?

qtsingleapplication-build.diff file contains some build fixes. Removing examples from build isn't upstreamable, but the rest might be. Have you already submitted the fixes back to upstream?

> %description        devel

> This package contains libraries and header files for developing applications
> that use QtSingleCoreApplication.

I think it shouldn't mention only "QtSingleCoreApplication" here (as opposed to "QtSingleApplication").

This package appears to bundle qtlockedfile library which is also distributed separately: http://qt.nokia.com/products/appdev/add-on-products/catalog/4/Utilities/qtlockedfile

openSUSE's package also contains a qtsingleapplication.prf file with the following contents:
INCLUDEPATH *= $$QMAKE_INCDIR_QT/QtSolutions
DEPENDPATH  *= $$QMAKE_INCDIR_QT/QtSolutions
LIBS *= -lQtSolutions_SingleApplication-2.6
QT *= network

Is it supposed to ease with linking against the "libQtSolutions_SingleApplication-2.6.so" file? If so, it might make sense to include it in our package too. The .so file name makes my eyes hurt ...

Re: licensing
Unless there's reason to the contrary, I agree that the License tag here should match what's in qt.

(In reply to comment #1)

Thanks for having a look!

> I'm not taking it for formal review for now, just got a few comments here.
> 
> > License:        GPLv3 or LGPLv2 with exceptions
> 
> The files in this tarball appear to have the same license text as Qt has.
> Fedora Qt package's license tag reads "LGPLv2 with exceptions or GPLv3 with
> exceptions". I'm not sure where exactly the GPLv3 exception is, though. rdieter
> is already in CC, maybe he can comment about that. In any case, if the license
> is the same, we should use the same license tag in both qt and in this package.
> 

I didn't make an investigation on this yet. I will verify this and proceed accordingly.

> > $ rpm -qlp qtsingleapplication-devel-2.6-1.fc14.i686.rpm
> > /usr/include/QtSolutions
> > /usr/include/QtSolutions/QtSingleApplication
> > /usr/include/QtSolutions/QtSingleCoreApplication
> > /usr/include/QtSolutions/qtsingleapplication.h
> > /usr/include/QtSolutions/qtsinglecoreapplication.h
> > /usr/lib/libQtSolutions_SingleApplication-2.6.so
> 
> You have QtSingleCoreApplication header, but is the actual library missing?
> 

Both libraries are is inside the /usr/lib/libQtSolutions_SingleApplication-2.6.so. As far as I remember I took the build.diff patch from SusE. But I'll have another look.

> qtsingleapplication-build.diff file contains some build fixes. Removing
> examples from build isn't upstreamable, but the rest might be. Have you already
> submitted the fixes back to upstream?
> 

Nope, not yet. I am gonna do this when we are sure we make things the right way.

> > %description        devel
> 
> > This package contains libraries and header files for developing applications
> > that use QtSingleCoreApplication.
> 
> I think it shouldn't mention only "QtSingleCoreApplication" here (as opposed to
> "QtSingleApplication").
> 

That's a typo. Will fix.

> This package appears to bundle qtlockedfile library which is also distributed
> separately:
> http://qt.nokia.com/products/appdev/add-on-products/catalog/4/Utilities/qtlockedfile
> 

Gee. That means I will package that one separately too. Thanks for letting me know.

> openSUSE's package also contains a qtsingleapplication.prf file with the
> following contents:
> INCLUDEPATH *= $$QMAKE_INCDIR_QT/QtSolutions
> DEPENDPATH  *= $$QMAKE_INCDIR_QT/QtSolutions
> LIBS *= -lQtSolutions_SingleApplication-2.6
> QT *= network
> 
> Is it supposed to ease with linking against the
> "libQtSolutions_SingleApplication-2.6.so" file? If so, it might make sense to
> include it in our package too. The .so file name makes my eyes hurt ...    

I am not familiar with .prf files. Suse ships this through an external source. I didn't include it. Is this like a pkg-config mechanism? Rex?

I made some research about the license. Qt has a GPLv3 exception when its distributed together with OpenSSL. See the bottom of the
   /usr/share/doc/qt-4.6.2/LICENSE.GPL3
On the other hand, qtsingleapplication does not specify such an exception at the end of its LICENSE.GPL3 file. If you take a diff of the two files you see that the GPLv3 exception clause is the only difference. Since also qtsingleapplication does not link directly to OpenSSL, I think the license tag is correct as "GPLv3 or LGPLv2 with exceptions".

fedorapeople.org was down, I uploaded these somewhere else:
SPEC: http://6mata.com:8014/review/qtsingleapplication.spec
SRPM: http://6mata.com:8014/review/qtsingleapplication-2.6-2.fc12.src.rpm

Changelog: 2.6-2
- Include .prf file
- Don't bundle external qtlockedfile library
- Fix typo in the description

I found that the .prf file is for qmake and it works like pkg-config. I also included a .prf file for the qtlockedfile library and made use of that file for building this package.

Looks good, comments follow:


naming: good, though I'm curious why upstream tacks on _1 in the tarball name. ?

sources: ok
md5sum *.gz
902795eb13ecedbdc112f00d7ec22949  qtsingleapplication-2.6_1-opensource.tar.gz

patches: please document patches (short .spec comment will suffice) , and preferably consult upstream about them.

$ rpmlint *.rpm x86_64/*.rpm
qtsingleapplication.src:52: W: configure-without-libdir-spec
qtsingleapplication.x86_64: E: non-standard-executable-perm /usr/lib64/libQtSolutions_SingleApplication-2.6.so.1.0.0 0775L
4 packages and 0 specfiles checked; 1 errors, 1 warnings.

I suppose using 'install -p -m755' instead of 'cp -a' can workaround this one, but you can choose whatever solution works best for you.

license: ok

scriptlets: ok

macros: ok

dependencies: ok

%files: ok


please address these comments, and I'll evaluate for final approval.

(In reply to comment #6)
> Looks good, comments follow:
> 

Thanks Rex

> patches: please document patches (short .spec comment will suffice) , and
> preferably consult upstream about them.

I submitted the patches and the .prf files of both this and qtlockedfile to the developers.
   http://bugreports.qt.nokia.com/browse/QTSOLBUG-119

> 
> $ rpmlint *.rpm x86_64/*.rpm
> qtsingleapplication.src:52: W: configure-without-libdir-spec
> qtsingleapplication.x86_64: E: non-standard-executable-perm
> /usr/lib64/libQtSolutions_SingleApplication-2.6.so.1.0.0 0775L
> 4 packages and 0 specfiles checked; 1 errors, 1 warnings.
> 
> I suppose using 'install -p -m755' instead of 'cp -a' can workaround this one,
> but you can choose whatever solution works best for you.
> 

I assume that you made a local mock build. Is that the case? If so, mock screws up with umask when building certain packages, e.g. those built with scons. Executables end up with 775. However when the same package is not built in koji, the problem does not occur.

"install -pm 755 *" will dereference the symlinks and I would have 4 copies of the same file. I can do an "ln -s" afterwards but that doesn't seem to be an elegant solution.

Instead I added a "chmod 755" to make sure the libarry gets the right permission. Here are the latest files:

SPEC: http://6mata.com:8014/review/qtsingleapplication.spec
SRPM: http://6mata.com:8014/review/qtsingleapplication-2.6-3.fc12.src.rpm

Rex asked me to wrap up the review for him.

(In reply to comment #6)
> naming: good, though I'm curious why upstream tacks on _1 in the tarball name.
> ?

I think I have an answer to that question. I pulled up qtsingleapplication's changelog from https://build.opensuse.org/package/show?package=qtsingleapplication&project=home%3Akoprok and there was this changeset:
 
 Name: qtsingleapplication
-Version: 2.6
+Version: 2.6_1
 Release: 1
 Url: http://qt.nokia.com/products/appdev/add-on-products/catalog/4/Utilities/qtsingleapplication/
 Group: Development/Libraries/C and C++
@@ -97,5 +97,7 @@
 %{_datadir}/qt4/mkspecs/features/%{name}.prf
 
 %changelog
+* Wed Apr 14 2010 Todor Prokopov <koprok>
+- Update to 2.6_1.
 * Thu Dec  3 2009 Todor Prokopov <koprok>
 - Initial package.


So it appears that 2.6_1 tarball is newer than 2.6.

I wonder if upstream is going to be consistent with using _ in versions; we should be careful to avoid introducing Epoch in case they want to release, say, 2.6.1 next.
rpmdev-vercmp says that 2.6_1 is newer than 2.6.1.

/usr/include/QtSolutions should be owned by qtsingleapplication-devel, otherwise removing the package will leave dangling empty directory behind.

There are separate qtsingleapplication.h and qtsinglecoreapplication.h headers in the include dir and in the build dir there are separate qtsingleapplication.o and qtsinglecoreapplication.o object files; however there is only qtsingleapplication DSO installed and the Core variant appears to be missing.

Looks like upstream has done the split to avoid pulling in QtGui libraries with the Core version. In QtLockedFile you carefully avoided (needlessly) linking with QtGui; but now the Core version which is supposed to not need QtGui isn't included in the final rpm.

rpmlint output:
qtsingleapplication.src:57: W: configure-without-libdir-spec
qtsingleapplication.i686: W: unused-direct-shlib-dependency /usr/lib/libQtSolutions_SingleApplication-2.6.so.1.0.0 /lib/libm.so.6
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

Both warnings can be ignored.

It might actually make sense to just remove Core headers too. We can always fix up Core library build later when some programs that actually need this show up in Fedora. Right now I can't think of any package which can take advantage of QtSingleCoreApplication.

However if you choose to fix Core build, I think it should go in a separate subpackage:
qtsingleapplication -> depends on QtQui
qtsinglecoreapplication -> depends only on QtCore

Thanks for the comments.

(In reply to comment #8)
> I wonder if upstream is going to be consistent with using _ in versions; we
> should be careful to avoid introducing Epoch in case they want to release, say,
> 2.6.1 next.
> rpmdev-vercmp says that 2.6_1 is newer than 2.6.1.    

Currently I have set the version to 2.6. So we are safe for the time being,  there is no problem with updating without introducing epoch. Are we allowed to have _ in the version tag? Guidelines tell us to move the nonnumeric characters to the release tag.

http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Version

(In reply to comment #9)
> /usr/include/QtSolutions should be owned by qtsingleapplication-devel,
> otherwise removing the package will leave dangling empty directory behind.
> 

Nope. There is no need. qtlockedfile owns that directory.

> There are separate qtsingleapplication.h and qtsinglecoreapplication.h headers
> in the include dir and in the build dir there are separate
> qtsingleapplication.o and qtsinglecoreapplication.o object files; however there
> is only qtsingleapplication DSO installed and the Core variant appears to be
> missing.
> 

They are not missing. The symbols of both are built into the same DSO.

(In reply to comment #10)
> It might actually make sense to just remove Core headers too. We can always fix
> up Core library build later when some programs that actually need this show up
> in Fedora. Right now I can't think of any package which can take advantage of
> QtSingleCoreApplication.
> 

clementine includes a modified version of QtSingleCoreApplication in addition to QtSingleApplication, but I don't think they are using it. I can remove this library...

> However if you choose to fix Core build, I think it should go in a separate
> subpackage:
> qtsingleapplication -> depends on QtQui
> qtsinglecoreapplication -> depends only on QtCore    

... or move it into another subpackage. But is there really a need? It occupies so little space, and as you said, there is nothing else that requires it.

(In reply to comment #11)
> clementine includes a modified version of QtSingleCoreApplication in addition
> to QtSingleApplication, but I don't think they are using it. I can remove this
> library...
> 

By the way, the only reason I included this library is because opensuse is including it, as you can see in the above link you gave. I took the patch from there. I kept QtSingleCoreApplication in the same DSO to keep things consistent with opensuse.

> Currently I have set the version to 2.6. So we are safe for the time being, 
> there is no problem with updating without introducing epoch. Are we allowed to
> have _ in the version tag? Guidelines tell us to move the nonnumeric characters
> to the release tag.

I'm sure we are technically allowed to have _ in the version tag, but I think we don't want to do that. Keeping the version at 2.6 is fine. Another option would be to set version to 2.6.1 to take into account the _1, but it doesn't matter much either way.


> They are not missing. The symbols of both are built into the same DSO.

Aha, I guess that works for now.


> > /usr/include/QtSolutions should be owned by qtsingleapplication-devel,
> > otherwise removing the package will leave dangling empty directory behind.
> 
> Nope. There is no need. qtlockedfile owns that directory.

Yes, qtlockedfile-devel owns the directory, but that doesn't help us because qtsingleapplication-devel doesn't depend on qtlockedfile-devel. The directory needs to be owned by something in the dependency chain; if some other unrelated leaf package somewhere in the Fedora package collection owns the directory, it doesn't help us solve the problem here.

There are a few options how to solve this:
a) have both packages own the directory;
b) have qtsingleapplication-devel depend on qtlockedfile-devel;
c) let qt-devel own the directory.

Option b is not a good one because it pulls in an unrelated package which we wouldn't normally need.

Option a is what I think is the most correct approach here: https://fedoraproject.org/wiki/Packaging/Guidelines#Multiple_packages_own_files_in_a_common_directory_but_none_of_them_needs_to_require_the_others.

wrt versioning, translating 2.6_1 into 2.6.1 is definitely the way to go here.  (and lobbying upstream to something saner in the meantime)

Oh, good catch Kalev. I missed the devel part of the dependencies. I will add the directory ownership.

So, we are not playing safe and setting version to 2.6.1? I am fine with that. Keep your fingers crossed.

Meanwhile, I experienced with clementine for wrapping the qtapplication with a new class and using that one instead, but these guys modified some private members. I have to rewrite at least 80% of the code (which isn't that long, ~60-70 lines) to wrap the class. This doesn't make sense. How about I add new members with overloading or renaming so that the original API stays intact, and we have an additional API that can be used by clementine?

We can then send this patch upstream, although I doubt that they will respond. They didn't respond to my previous queries.

(In reply to comment #15)
> So, we are not playing safe and setting version to 2.6.1? I am fine with that.
> Keep your fingers crossed.

In my eyes either 2.6 or 2.6.1 is fine; but I'm against using 2.6_1 in version tag.

SPEC: http://oget.fedorapeople.org/review/qtsingleapplication.spec
SRPM: http://oget.fedorapeople.org/review/qtsingleapplication-2.6.1-1.fc13.src.rpm

Changelog: 2.6.1-1
- Change version to 2.6.1. Upstream uses weird version convention 2.6_1
- Own the directory %%{_qt4_headerdir}/QtSolutions/

Fedora review qtsingleapplication-2.6.1-1.fc13.src.rpm 2010-07-05

+ OK
! needs attention

rpmlint output:
qtsingleapplication.src:60: W: configure-without-libdir-spec
qtsingleapplication.i686: W: unused-direct-shlib-dependency /usr/lib/libQtSolutions_SingleApplication-2.6.so.1.0.0 /lib/libm.so.6
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

+ The package is named according to the Package Naming Guidelines.
+ Spec file name matches the base package name
+ The package follows the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains license files (LGPL_EXCEPTION.txt, LICENSE.GPL3, and LICENSE.LGPL)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  902795eb13ecedbdc112f00d7ec22949  qtsingleapplication-2.6_1-opensource.tar.gz
  902795eb13ecedbdc112f00d7ec22949  Download/qtsingleapplication-2.6_1-opensource.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file MUST handle locales properly
+ ldconfig is properly called in %post and %postun
+ Package does not bundle copies of system libraries
n/a Does not use Prefix: /usr
+ Package owns all directories it creates
+ No duplicate files in %files
+ Permissions are properly set and %files has %defattr
+ Consistent use of macros
+ The package must contain code, or permissable content.
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc don't affect the package
+ Header files are in -devel
n/a Static libraries should be in -static
+ Library files that end in .so are in -devel package
+ -devel requires the fully versioned base
+ Package doesn't contain any libtool .la files
n/a Packages containing GUI apps must include %{name}.desktop file
+ Directory ownership sane
+ Filenames are valid UTF-8


APPROVED

Thanks a bunch Kalev!

New Package CVS Request
=======================
Package Name: qtsingleapplication
Short Description: Qt library to start applications only once per user
Owners: oget
Branches: F-12 F-13
InitialCC:

CVS done (by process-cvs-requests.py).

I think there may be an issue for programs using QtCore but not QtGui.

The qtsingleapplication solution provides both QtCore and QtGui classes, for example, QtSingleApplication and QtSingleCoreApplication.

According to the examples provided by Nokia for qtsingleapplication, only the programs using the QtSingleApplication class need to link against the so file, the programs using only QtSingleCoreApplication don't.

The issue is that your qtsingleapplication-devel package only provide qtsingleapplication.prf, no qtsinglecoreapplication.prf file.

qtsinglecoreapplication.prf file would contain only:
INCLUDEPATH *= $$QMAKE_INCDIR_QT/QtSolutions
DEPENDPATH  *= $$QMAKE_INCDIR_QT/QtSolutions
QT *= network

instead of:
INCLUDEPATH *= $$QMAKE_INCDIR_QT/QtSolutions
DEPENDPATH  *= $$QMAKE_INCDIR_QT/QtSolutions
LIBS *= -lQtSolutions_SingleApplication-2.6
QT *= network

I believe Programs that don't use QtGui should not depend on the qtsingleapplication package (which provides the so file) because it requires QtGui.

What do you think about this?

Hi Christophe,
Yes you are right. Kalev pointed out this issue in comment #9. Currently beth qtsingleapplication and qtsinglecoreapplication symbols are in the file
   libQtSolutions_SingleApplication-2.6.so.1.0.0
which links to QtGui, which means anything that uses qtsinglecoreapplication will drag in QtGui library and all its dependencies. We didn't split qtsinglecoreapplication, because no other application was using it.

But if there is need, sure, we will split them into different .so files.

However, I believe the qtsinglecoreapplication.prf file should contain
INCLUDEPATH *= $$QMAKE_INCDIR_QT/QtSolutions
DEPENDPATH  *= $$QMAKE_INCDIR_QT/QtSolutions
LIBS *= -lQtSolutions_SingleCoreApplication-2.6
QT *= network

so that you can link to the correct .so file. If you don't link to anything, you will get missing symbols, as in here: http://fpaste.org/AbgK/

Is it okay for you if we do it this way? Kalev, what is your take on this?

I agree with Orcan: qtsinglecoreapplication.h file only contains function prototypes and to use these the program certainly needs to be linked with the actual DSO providing the symbols.

(In reply to comment #22)
> LIBS *= -lQtSolutions_SingleCoreApplication-2.6

I must say I don't like the symlink name libQtSolutions_SingleCoreApplication-2.6.so, but at least it's consistent with the non-Core library naming and we should follow upstream naming even if we think it's bad. Upstream should really get their act together and name it to something like:
libQtSingleCoreApplication.so (symlink)
libQtSingleCoreApplication.so.2 (soname)
libQtSingleCoreApplication.so.2.6.1 (DSO)
instead. Right now the .prf files hide these names from apps which use qmake, but apps which use other build systems need to spell out the whole name (which includes version!) to link with the lib.

The way Orcan is proposing the split in comment #22 is in my opinion the best we can do here.

Yes, what Orcan is proposing seems good to me too.
Just don't forget to include the qtsinglecoreapplication.prf file.

Any proposals for the subpackage name?
  qtsingleapplication-core
  qtsinglecoreapplication
... ?

How about the devel package? Do we need to split that too?

I would go with 
  qtsingleapplication
  qtsingleapplication-devel
  qtsinglecoreapplication
  qtsinglecoreapplication-devel

I agree with:
  qtsingleapplication
  qtsingleapplication-devel
  qtsinglecoreapplication
  qtsinglecoreapplication-devel

It seems logical.

Hey folks: I made the new builds with the libraries split:

F-12: https://admin.fedoraproject.org/updates/qtsingleapplication-2.6.1-3.fc12
F-13: https://admin.fedoraproject.org/updates/qtsingleapplication-2.6.1-3.fc13
devel: http://koji.fedoraproject.org/koji/buildinfo?buildID=185326

Please test and let me know how things go.

(In reply to comment #27)
> Hey folks: I made the new builds with the libraries split:
> 
> F-12: https://admin.fedoraproject.org/updates/qtsingleapplication-2.6.1-3.fc12
> F-13: https://admin.fedoraproject.org/updates/qtsingleapplication-2.6.1-3.fc13
> devel: http://koji.fedoraproject.org/koji/buildinfo?buildID=185326
> 
> Please test and let me know how things go.    


I have tested qbittorrent and the new packages function as expected.

http://koji.fedoraproject.org/koji/buildinfo?buildID=185966

Can we get buildroot overrides for F13 & F12?

Closing the ticket. qtsingleapplication-2.6.1-3 is in F-14 + rawhide and in stable updates for F-12 and F-13.

Weird that I forgot to link the bodhi update request to this review request.

Thanks for keeping track.

Package Change Request
======================
Package Name: qtsingleapplication
New Branches: el6 epel7
Owners: fale

epel7 is already done. sorry for the wrong copy&paste

Git done (by process-git-requests).