Bug 595638

Taking for review.

> Summary: Qt4 cross-platform client for Twitter

I think it's better to avoid mentioning the toolkit (Qt4) in summary, but rather concentrate on what the client does. It's really not interesting for non-technical users. Also, I wouldn't say "cross-platform" either, as it basically just says that we can run that program. I'd suggest to use the summary from Debian package [1] instead:
"Lightweight desktop client for Twitter microblogging service"

[1] http://packages.debian.org/sid/qwit


Why aren't you using the cmake-based build system instead of qmake? The CMakeLists.txt file looks pretty sophisticated and among other things installs translation files automatically, which are currently missing from the rpm.


Instead of installing the icon directly to %{_datadir}/icons/*.png, I think it'd be better to install it to %{_datadir}/icons/hicolor/*/apps/*.png as the latter supports icon themes.

The Debian package installs a man page. Please include it in this package and submit it for upstream inclusion too.

I dropped Qt4 in summary and fixed the icon path.

Some issue I didn't fix yet:

1.CMakeLists.txt in qwit is buggy currently, e.g. wrong include path for qca2, wrong install path for qwit binary.
2.The man page in Debian package is useless, qwit can't accept command line argument.



 
SPEC:http://dl.dropbox.com/u/1338197/1/qwit.spec
SRPM:http://dl.dropbox.com/u/1338197/1/qwit-1.1-0.2.beta.fc13.src.rpm

%{_datadir}/pixmaps/ is generally regarded as a legacy location for icons. You can use it if you want, but it's probably better to install them to hicolor icon theme instead.

What about translations which the cmake build system installs but qmake seems to skip?
translations/qwit_zh_CN.qm
translations/qwit_pl_PL.qm
translations/qwit_tr_TR.qm
translations/qwit_kk_KZ.qm
translations/qwit_pt_BR.qm
translations/qwit_en_US.qm
translations/qwit_it_IT.qm
translations/qwit_de_DE.qm
translations/qwit_es_ES.qm
translations/qwit_fi_FI.qm
translations/qwit_ru_RU.qm

If you are interested, we could try to fix the CMakeLists.txt together. But it only makes sense if you are willing to push those changes upstream; if not, it's probably easier to keep using qmake.


I took a quick look at the CMakeLists.txt file and it looks like we need to do the following changes:
-       /usr/include/qca2/QtCrypto
+       /usr/include/QtCrypto

However, changing include directory like that is just a quick hack to get it building and it's not a proper fix to send upstream. Proper fix would be to create a directory cmake_modules/ in qwit source tree and copy svn://anonsvn.kde.org/home/kde/trunk/KDE/kdelibs/cmake/modules/FindQCA2.cmake over there. Then we can do something like this:
# Custom cmake modules
set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules")
find_package(QCA2 REQUIRED)

When this is done, we can use ${QCA2_INCLUDE_DIR} instead of /usr/include/qca2/QtCrypto in the include_directories() call:

-       /usr/include/qca2/QtCrypto
+       ${QCA2_INCLUDE_DIR}


The following two changes should be easily upstreamable:

-install( TARGETS qwit DESTINATION ${QT_BINARY_DIR} )
+install( TARGETS qwit DESTINATION bin )

-install( FILES images/qwit.png DESTINATION share/icons )
+install( FILES images/qwit.png DESTINATION share/icons/hicolor/32x32/apps )

(In reply to comment #4)
> %{_datadir}/pixmaps/ is generally regarded as a legacy location for icons. You
> can use it if you want, but it's probably better to install them to hicolor
> icon theme instead.
> 
> What about translations which the cmake build system installs but qmake seems
> to skip?
> translations/qwit_zh_CN.qm
> translations/qwit_pl_PL.qm
> translations/qwit_tr_TR.qm
> translations/qwit_kk_KZ.qm
> translations/qwit_pt_BR.qm
> translations/qwit_en_US.qm
> translations/qwit_it_IT.qm
> translations/qwit_de_DE.qm
> translations/qwit_es_ES.qm
> translations/qwit_fi_FI.qm
> translations/qwit_ru_RU.qm    

Those translations are embedded in the qwit binary.

(In reply to comment #5)
> If you are interested, we could try to fix the CMakeLists.txt together. But it
> only makes sense if you are willing to push those changes upstream; if not,
> it's probably easier to keep using qmake.
> 
> 
> I took a quick look at the CMakeLists.txt file and it looks like we need to do
> the following changes:
> -       /usr/include/qca2/QtCrypto
> +       /usr/include/QtCrypto
> 
> However, changing include directory like that is just a quick hack to get it
> building and it's not a proper fix to send upstream. Proper fix would be to
> create a directory cmake_modules/ in qwit source tree and copy
> svn://anonsvn.kde.org/home/kde/trunk/KDE/kdelibs/cmake/modules/FindQCA2.cmake
> over there. Then we can do something like this:
> # Custom cmake modules
> set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules")
> find_package(QCA2 REQUIRED)
> 
> When this is done, we can use ${QCA2_INCLUDE_DIR} instead of
> /usr/include/qca2/QtCrypto in the include_directories() call:
> 
> -       /usr/include/qca2/QtCrypto
> +       ${QCA2_INCLUDE_DIR}
> 
> 
> The following two changes should be easily upstreamable:
> 
> -install( TARGETS qwit DESTINATION ${QT_BINARY_DIR} )
> +install( TARGETS qwit DESTINATION bin )
> 
> -install( FILES images/qwit.png DESTINATION share/icons )
> +install( FILES images/qwit.png DESTINATION share/icons/hicolor/32x32/apps )  

Yesterday, I fixed those issues locally and write a new spec, but since qwit is a quite small package, so I finally decided to continue use qmake. Also, the default building way for qwit is qmake, See INSTALL in the tarball.

Fedora review qwit-1.1-0.2.beta.fc13.src.rpm 2010-06-30

+ OK
! needs attention

rpmlint output:
qwit.src: W: spelling-error Summary(en_US) microblogging -> micro blogging, micro-blogging, microbiologist
qwit.src: W: spelling-error %description -l en_US http -> HTTP
qwit.src: W: spelling-error %description -l en_US userpics -> user pics, user-pics, username
qwit.src: W: spelling-error %description -l en_US clickable -> click able, click-able, checkable
qwit.src: W: spelling-error %description -l en_US timelines -> timeline, timeliness, time lines
qwit.src: W: spelling-error %description -l en_US retweets -> re tweets, re-tweets, tweets
qwit.src: W: spelling-error %description -l en_US customizable -> customization, customize, customable
qwit.src: W: spelling-error %description -l en_US twitpic -> twit pic, twit-pic, twitchy
qwit.src: W: spelling-error %description -l en_US url -> URL, curl, purl
qwit.src: W: spelling-error %description -l en_US shorteners -> shortener, shortener s, shortening
qwit.src: W: spelling-error %description -l en_US identi -> identic, identify, identity
qwit.src: W: spelling-error %description -l en_US aitu -> ait, ait u, Aitken
qwit.src: W: spelling-error %description -l en_US kz -> k, z, kHz
qwit.src: W: no-cleaning-of-buildroot %install
qwit.src: W: no-buildroot-tag
qwit.src: W: invalid-url Source0: http://qwit.googlecode.com/files/qwit-1.1-beta-src.tar.bz2 HTTP Error 404: Not Found
qwit.i686: W: spelling-error Summary(en_US) microblogging -> micro blogging, micro-blogging, microbiologist
qwit.i686: W: spelling-error %description -l en_US http -> HTTP
qwit.i686: W: spelling-error %description -l en_US userpics -> user pics, user-pics, username
qwit.i686: W: spelling-error %description -l en_US clickable -> click able, click-able, checkable
qwit.i686: W: spelling-error %description -l en_US timelines -> timeline, timeliness, time lines
qwit.i686: W: spelling-error %description -l en_US retweets -> re tweets, re-tweets, tweets
qwit.i686: W: spelling-error %description -l en_US customizable -> customization, customize, customable
qwit.i686: W: spelling-error %description -l en_US twitpic -> twit pic, twit-pic, twitchy
qwit.i686: W: spelling-error %description -l en_US url -> URL, curl, purl
qwit.i686: W: spelling-error %description -l en_US shorteners -> shortener, shortener s, shortening
qwit.i686: W: spelling-error %description -l en_US identi -> identic, identify, identity
qwit.i686: W: spelling-error %description -l en_US aitu -> ait, ait u, Aitken
qwit.i686: W: spelling-error %description -l en_US kz -> k, z, kHz
qwit.i686: W: no-manual-page-for-binary qwit
3 packages and 0 specfiles checked; 0 errors, 30 warnings.

+ rpmlint warnings are harmless and can be ignored
+ 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 the license file (COPYING)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  b5d2985152413eaef6913a6e3fab8bc7  qwit-1.1-beta-src.tar.bz2
  b5d2985152413eaef6913a6e3fab8bc7  Download/qwit-1.1-beta-src.tar.bz2
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a The spec file MUST handle locales properly
n/a 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
+ Package contains code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
n/a Header files must be in a -devel package.
n/a Static libraries must be in a -static package
n/a Library files that end in .so must go in a -devel package
n/a -devel must require the fully versioned base
+ Packages should not contain libtool .la files
+ Packages containing GUI apps must include %{name}.desktop file
+ Packages must not own files or directories owned by other packages
+ Filenames must be valid UTF-8


APPROVED

New Package CVS Request
=======================
Package Name: qwit
Short Description: Lightweight desktop client for Twitter microblogging service
Owners: supercyper
Branches: F-12 F-13
InitialCC:

(In reply to comment #8)
> Fedora review qwit-1.1-0.2.beta.fc13.src.rpm 2010-06-30
> 
> 
> APPROVED    

Thanks for the review, Kalev!

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

Moved icons to /usr/share/icons/hicolor/*/*/ as comment#5, close this report now.