Bug 494695 - (qutIM) Review Request: qutim - Multiplatform Instant Messenger on Qt4
Review Request: qutim - Multiplatform Instant Messenger on Qt4
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On: 494693
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-07 15:40 EDT by Pavel Alexeev
Modified: 2010-02-21 18:52 EST (History)
10 users (show)

See Also:
Fixed In Version: 0.2.0-4.fc12
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-07 16:51:07 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lemenkov: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pavel Alexeev 2009-04-07 15:40:38 EDT
Spec URL: http://hubbitus.net.ru/rpm/Fedora9/qutIM/qutIM.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora9/qutIM/qutIM-0.2-0.11.alphaSVN20080404.fc9.src.rpm
Description: qutIM - free open-source multiprotocol ( ICQ, Jabber/GTalk/Ya.Online/LiveJournal.com, Mail.Ru, IRC ) instant messenger for 
Windows and Linux systems
Comment 1 Peter Lemenkov 2009-05-18 23:33:08 EDT
I'll review it.
Comment 2 Peter Lemenkov 2009-07-31 14:53:22 EDT
Ver. 0.2_beta2 is out. Please, consider updating your srpm, and I'll continue.

http://www.qutim.org/uploads/src/qutim-0.2_beta2.tar.gz
Comment 3 Pavel Alexeev 2009-08-01 01:16:11 EDT
Off course! Many time ago I was try build it, but it is impossible due bug: http://trac.qutim.org/task/258

I directly speak with maintainer, and problem occurred in directly patching bundled copy of gloox library. This way is unacceptable for Fedora. So, waiting until bug resolved.
Comment 4 Peter Lemenkov 2009-08-01 02:39:28 EDT
Ok, understood. Until then, I'm adding NotReady keyword to the whiteboard.
Comment 5 Alexey Torkhov 2009-09-17 02:54:58 EDT
There is build for 0.2 beta svn snapshot in this repo: ftp://volkoff.ru/repo/fedora11/
Here is discussion: http://linuxforum.ru/index.php?showtopic=99429
But perhaps they build it with bundled gloox. Is that bug still unfixed?
Comment 6 Pavel Alexeev 2009-09-18 07:21:16 EDT
As you can see, bug is reopened and still unfixed. If you want speedup - comment these bug in its bugtracker to fix error.
Previous maintainer of Qutim-Jabber plugin is unaccessible for the long time for me. I think it is main reason...

Also, as you known Fedora do not permit bundled libraries and built with it is prohibited.
Comment 8 Peter Lemenkov 2009-10-29 11:56:57 EDT
Koji scratchbuild for F-11 failed due to easy-to-fix issue in the %files section:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1776709
Comment 10 Peter Lemenkov 2009-11-01 10:03:19 EST
BTW, qutim 0.2 is out! 

http://www.qutim.org/forum/viewtopic.php?f=40&t=1335

Could you, please, update your package to the latest version?
Comment 11 Pavel Alexeev 2009-11-01 10:13:25 EST
Off course! Gloox release 1.0 also was released yesterday. We speak about it with Ruslan and I prepare both updates (I think do that in 1-3 days). But it is not contain major changes except bugfixes and api stability, can we finally start review?? Update process in the future should be persistent continuous process...
Comment 12 Peter Lemenkov 2009-11-06 08:16:45 EST
Builds fine now.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1791960

REVIEW:

+ rpmlint is not silent:

[petro@Workplace ~]$ rpmlint Desktop/qutim-*
qutim-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qutim/plugins/icq/clientidentify.h
qutim-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.cpp
qutim-debuginfo.i586: E: wrong-script-end-of-line-encoding /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.cpp
qutim-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qutim/plugins/icq/buddycaps.h
qutim-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.h
qutim-debuginfo.i586: E: wrong-script-end-of-line-encoding /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.h
qutim-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/qutim/plugins/icq/clientidentify.cpp
qutim-devel.i586: W: no-documentation
qutim-icq.i586: W: no-documentation
qutim-irc.i586: W: no-documentation
qutim-jabber.i586: W: no-documentation
qutim-mrim.i586: W: no-documentation
7 packages and 0 specfiles checked; 2 errors, 10 warnings.
[petro@Workplace ~]$ 

These issues should be fixed at %prep stage.

+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.

- The package does not fully  meet the Packaging Guidelines. There are some unlisted Requires and some unneeded calls for ldconfig. Also, and most important, I can;t find a package, who provides /usr/share/icons/mini.

So, you should add the following explicit requires:

hicolor-icon-theme (for /usr/share/icons/hicolor/64x64/apps )
cmake (for /usr/share/cmake/Modules )

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license.
+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.

+/- The sources used to build the package must match the upstream source, as provided in the spec URL. Since it's a pre-release, and new version was recently released, I'll wait until you update srpm to ver. 0.2.

+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji log above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files (not just symlinks) in any of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable.

- The package must own all directories that it creates. Please, add %{_libdir}/%{name} to the main package's %files section as %dir. Also add %{_includedir}/%{name} as %dir in devel sub-package.

- A Fedora package must not list a file more than once in the spec file's %files listings. Unfortunately, you're listing %{_bindir}/%{name} twice in main package's %files section.

- Permissions on files must be set properly, but there are some rpmlint complaints regarding wrong permissions. See rpmlint log posted above.

+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
0 Anything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are placed in a -devel package.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package does not contain library files with a suffix.
+ The devel package requires the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ The packages does NOT contain any .la libtool archives.
+ The package includes a %{name}.desktop file, and that file is properly installed with desktop-file-install in the %install section.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in the package are valid UTF-8.

So, please, fix issues noted above, and I'll continue with review.
Comment 13 Pavel Alexeev 2009-11-10 04:01:51 EST
Peter, you was right - in release many changes...

(In reply to comment #12)
> Builds fine now.
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1791960
> 
> REVIEW:
> 
> + rpmlint is not silent:
> 
> [petro@Workplace ~]$ rpmlint Desktop/qutim-*
> qutim-debuginfo.i586: W: spurious-executable-perm
> /usr/src/debug/qutim/plugins/icq/clientidentify.h
> qutim-debuginfo.i586: W: spurious-executable-perm
> /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.cpp
> qutim-debuginfo.i586: E: wrong-script-end-of-line-encoding
> /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.cpp
> qutim-debuginfo.i586: W: spurious-executable-perm
> /usr/src/debug/qutim/plugins/icq/buddycaps.h
> qutim-debuginfo.i586: W: spurious-executable-perm
> /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.h
> qutim-debuginfo.i586: E: wrong-script-end-of-line-encoding
> /usr/src/debug/qutim/src/3rdparty/qtwin/qtwin.h
> qutim-debuginfo.i586: W: spurious-executable-perm
> /usr/src/debug/qutim/plugins/icq/clientidentify.cpp
> qutim-devel.i586: W: no-documentation
> qutim-icq.i586: W: no-documentation
> qutim-irc.i586: W: no-documentation
> qutim-jabber.i586: W: no-documentation
> qutim-mrim.i586: W: no-documentation
> 7 packages and 0 specfiles checked; 2 errors, 10 warnings.
spurious-executable-perm fixed in %prep.

Add BR dos2unix and fix lineendings qtwin to avoid rpmlint complain wrong-script-end-of-line-encoding. But is it really needed for files inytended for Windows (just delete break build, but it nothing doing on Linux)?

> and some unneeded calls for ldconfig.
Why calls to ldconfig is unneeded?

http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries says:
Whenever possible (and feasible), Fedora Packages containing libraries should build them as shared libraries. In addition, every binary RPM package which contains shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. If the package has multiple subpackages with libraries, each subpackage should also have a %post/%postun section that calls /sbin/ldconfig.

> Also, and most
> important, I can;t find a package, who provides /usr/share/icons/mini.
Hm, yes, I too. May be I simple should delete this file?

> So, you should add the following explicit requires:
> 
> hicolor-icon-theme (for /usr/share/icons/hicolor/64x64/apps )
Added.
> cmake (for /usr/share/cmake/Modules )
Added to -devel subpackage.

> + The License field in the package spec file matches the actual license.
There I find some issues: Several files like (3rdparty/qtsolutions/*, plugins/plugman/libs/QZipReader) is 3dparty and have licensing similar to QT: Commercial/LGPL with exceptions in LGPL_EXCEPTION.txt/GPLv3.
This is free software, but it is not under GPLv2+ license as full package. And I in frustration - can we ship it? If you can't answer on this question, please set "legal" flag for Spot.

> +/- The sources used to build the package must match the upstream source, as
> provided in the spec URL. Since it's a pre-release, and new version was
> recently released, I'll wait until you update srpm to ver. 0.2.
I update it. Also several new plugins appeared in release.

> - A Fedora package must not list a file more than once in the spec file's
> %files listings. Unfortunately, you're listing %{_bindir}/%{name} twice in main
> package's %files section.
Yes, it with comment from old times, when symlink was also been installed.
Fixed.

http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-1.17.fc11.src.rpm
Comment 14 Pavel Alexeev 2009-11-11 06:46:12 EST
Add epoch in gloox dependency:
http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-1.18.fc11.src.rpm
Comment 15 Pavel Alexeev 2009-11-12 20:13:58 EST
Delete invoke ldconfig:
http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-1.19.fc11.src.rpm
Comment 16 Peter Lemenkov 2009-11-23 05:49:57 EST
Still not addressed:

* %global _miconsdir      %{_datadir}/icons/mini
There is no such package in Fedora repositories, which owns this directory. Also your package doesn't own it. This should be fixed. I believe, that this icon should be placed in some other directory than %{_datadir}/icons/mini .

* The macro %{_iconsdir} is superfluous.

%global _iconsdir               %{_datadir}/icons
%global _hiconsdir      %{_datadir}/icons/hicolor

Take a look at both places, where you're using %{_iconsdir} - actually, you should use %{_hiconsdir} macro there. I advice you to shorten the number of macro being used.

* Directory ownership issues:

%{_libdir}/%{name} should be owned by the main package.
%{_includedir}/%{name} should be owned by *-devel sub-package

* Also consider removing entirely %doc sections from plugin sub-packages.
Comment 17 Pavel Alexeev 2009-11-26 16:17:44 EST
Thank you. All mentioned issue fixed, except one:
> * Also consider removing entirely %doc sections from plugin sub-packages.  
paths fixed instead, and in plugins packages included its docs (only mrim do not have it because it is absent in upstream tarball)

http://koji.fedoraproject.org/koji/taskinfo?taskID=1832695

http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-1.20.fc11.src.rpm
Comment 18 Peter Lemenkov 2009-11-29 12:03:47 EST
(In reply to comment #17)
> Thank you. All mentioned issue fixed, except one:
> > * Also consider removing entirely %doc sections from plugin sub-packages.  
> paths fixed instead, and in plugins packages included its docs (only mrim do
> not have it because it is absent in upstream tarball)
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1832695
> 
> http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-1.20.fc11.src.rpm  

Some new issues :)

* In main package you forgot to mark owned directory %{_libdir}/%{name} as %dir
* I believe, that instead of installing small icon to %{buildroot}/%{_datadir}/icons/hicolor/16x16/ you intended to install to %{buildroot}/%{_datadir}/icons/hicolor/16x16/apps
Comment 19 Pavel Alexeev 2009-11-29 16:06:59 EST
(In reply to comment #18)
> * In main package you forgot to mark owned directory %{_libdir}/%{name} as %dir
Oh, yes, thank you.
> * I believe, that instead of installing small icon to
> %{buildroot}/%{_datadir}/icons/hicolor/16x16/ you intended to install to
> %{buildroot}/%{_datadir}/icons/hicolor/16x16/apps  
Fixed.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1836516
http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-1.21.fc11.src.rpm
Comment 20 Pavel Alexeev 2009-12-15 08:59:30 EST
ping?
Comment 21 Rex Dieter 2009-12-15 09:06:54 EST
a small comments regarding package description/summary, imho, it makes little sense to mention "multiplatform" or "on Qt4", (fedora) users won't care (or need to know) about what platforms are supported or the toolkit used.
Comment 22 Peter Lemenkov 2009-12-15 09:44:19 EST
Cleaning up NotReady, since it is almost ready.
Comment 23 Peter Lemenkov 2009-12-15 09:45:48 EST
(In reply to comment #21)
> a small comments regarding package description/summary, imho, it makes little
> sense to mention "multiplatform" or "on Qt4", (fedora) users won't care (or
> need to know) about what platforms are supported or the toolkit used.  

I would like to keep at least mention about toolkit, since some users prefer GTK apps over Qt or vice versa.
Comment 24 Rex Dieter 2009-12-15 10:42:01 EST
OK, but please leave it out of the package summary at least, this could be something mentionable in the package description.  that's a lesser of 2 evils.
Comment 25 Pavel Alexeev 2009-12-16 14:36:23 EST
Ok, I change "multiplatform" on "Multiprotocol" what is more important for IM. Mention of QT4 have worth on my mind too. Summary rewritten.

Release enumeration changed, some comments stripped.

http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim-0.2.0-2.fc11.src.rpm
http://hubbitus.net.ru/rpm/Fedora11/qutim/qutim.spec
Comment 26 Peter Lemenkov 2009-12-17 02:03:06 EST
Ok, I don't see any outstanding issues so far, so this package is

APPROVED.
Comment 27 Pavel Alexeev 2009-12-18 08:30:34 EST
Peter, thank you very much for the review.

New Package CVS Request
=======================
Package Name: qutim
Short Description: Multiprotocol (ICQ, Jabber, IRC etc) instant messenger with modern Qt4 interface
Owners: hubbitus
Branches: F-10 F-11 F-12
InitialCC:
Comment 28 Kevin Fenzi 2009-12-20 22:14:52 EST
We no longer do F-10 branches as it's now end of life. 

otherwise, cvs done.
Comment 29 Pavel Alexeev 2009-12-21 03:12:16 EST
Thank you, Kevin.
Comment 30 Fedora Update System 2009-12-21 06:15:42 EST
qutim-0.2.0-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/qutim-0.2.0-2.fc12
Comment 31 Fedora Update System 2009-12-21 23:42:15 EST
qutim-0.2.0-2.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 qutim'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2009-13435
Comment 32 Aleksey Davydov 2009-12-30 09:18:47 EST
Plugins don't work on x86-64. 
Please update plugin directory for x86-64 to "/usr/lib64/qutim/"
Comment 33 Peter Lemenkov 2009-12-30 11:13:52 EST
(In reply to comment #32)
> Plugins don't work on x86-64. 
> Please update plugin directory for x86-64 to "/usr/lib64/qutim/"  

Aleksey, I'm not sure that I can confirm your report - I just checked qutim-0.2.0-2.fc12.x86_64.rpm, qutim-icq-0.2.0-2.fc12.x86_64.rpm and qutim-jabber-0.2.0-2.fc12.x86_64.rpm - they all contain /usr/lib64/qutim directory (even with proper library in case of plugin sub-package).
Comment 34 Aleksey Davydov 2009-12-30 11:43:26 EST
Sorry, I wasn't clear enough.

I mean search path for plugins.

My installation gives me the following when run from command line:
Debug: ("/usr/bin/plugins", "/usr/lib/qutim", "/usr/lib/qutim/plugins", "/usr/PlugIns", "/home/nld/.config/qutim/plugins") 
Debug: ("", "../share/qutim", "/home/nld/.config/qutim") 
Debug: "English" "UnitedStates" 
Debug: ("en_US", "en_us", "en") 
Debug: "English" "UnitedStates" 
Debug: ("en_US", "en_us", "en") 
Debug: "en" 
Debug: "QTreeView {  }" 
Warning: QFSFileEngine::open: No file name specified

The first path should contain also "/usr/lib64/qutim/".

Thanks.
Comment 35 Pavel Alexeev 2010-01-07 05:48:45 EST
Aleksey thank you for the bugreport, but qutim already build and imported and it is not subject of Review Request. Please, for each problems in qutim fill separate bugs. Meantime I'll look on it. You say what plugins does not loaded in x86_64?
Comment 36 Fedora Update System 2010-01-07 16:51:01 EST
qutim-0.2.0-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 37 Fedora Update System 2010-01-07 16:54:20 EST
qutim-0.2.0-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 38 Fedora Update System 2010-01-07 20:12:58 EST
qutim-0.2.0-4.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/qutim-0.2.0-4.fc12
Comment 39 Fedora Update System 2010-01-08 14:59:39 EST
qutim-0.2.0-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 40 Fedora Update System 2010-01-08 15:00:51 EST
qutim-0.2.0-4.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 41 Christoph Wickert 2010-02-16 20:13:35 EST
The spec could be more readable if it was formatted properly. Please use line breaks at 80 charakters, especially in the descriptions. Otherwise they are hard to read on a terminal or in the PackageKit UI.

The name of the tag is "URL", not "Url".

The group of the -devel package should be Development/Libraries instead of Applications/Internet. For the other subpackages you can omit the group tag because it is the same than the one from the base package.

%descriptions should end with a dot. Exclamation marks are even worse than the missing dots.

There are a lot of linguistic and grammar errors in the English descriptions.

The icon cache scriptlets are outdated, please use the latest version from 
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

Omit --add-category="Network" from desktop-file-install because this is already in the file.

Please preserve timestamps during %install, 
install -p -m 644 icons/%{name}.png ...
...
make install DESTDIR="%{buildroot}" INSTALL="install -p"
see https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Please use %global instead of %define, see 
https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define
Just a hint, this is commented out currently.
Comment 42 Pavel Alexeev 2010-02-21 18:52:45 EST
(In reply to comment #41)
> The spec could be more readable if it was formatted properly. Please use line
> breaks at 80 charakters, especially in the descriptions. Otherwise they are
> hard to read on a terminal or in the PackageKit UI.
Yes, I was nit enouth carefully and in qutim-plugman description some lines exceedes 80 characters. Fixed. Thanks.
> 
> The name of the tag is "URL", not "Url".
Is it have any sence?

> The group of the -devel package should be Development/Libraries instead of
> Applications/Internet.
Thanks, fixed.

> For the other subpackages you can omit the group tag
> because it is the same than the one from the base package.
It was required before in each defined package. Some changed nowadays?

> %descriptions should end with a dot. Exclamation marks are even worse than the
> missing dots.
I'm do not see any worth in dot, especially in one line descriptions, but it is not bad idea in any case. Ok.

> There are a lot of linguistic and grammar errors in the English descriptions.
Ohh, yes, I known. I recheck it again. And if you can point to concrete mistakes I'll fix it with pleasure.

> The icon cache scriptlets are outdated, please use the latest version from 
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
Updated.

> Omit --add-category="Network" from desktop-file-install because this is already
> in the file.
Ok.

> 
> Please preserve timestamps during %install, 
> install -p -m 644 icons/%{name}.png ...
> ...
> make install DESTDIR="%{buildroot}" INSTALL="install -p"
> see https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
Yes, this is my mistake. Thanks. Fixed.

> Please use %global instead of %define, see 
> https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define
> Just a hint, this is commented out currently.    
Yes, it is not used now. BTW I replace it.


http://koji.fedoraproject.org/koji/taskinfo?taskID=2003770

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