Bug 595638 - Review Request: qwit - Qt4 cross-platform client for Twitter
Review Request: qwit - Qt4 cross-platform client for Twitter
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
13
All Linux
low Severity medium
: ---
: ---
Assigned To: Kalev Lember
Fedora Extras Quality Assurance
:
Depends On: 595637
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-25 05:09 EDT by Chen Lei
Modified: 2010-07-02 02:58 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-02 02:58:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kalevlember: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Chen Lei 2010-05-25 05:09:33 EDT
Description of problem:
Qwit is a simple Qt4-based client for Twitter.

SPEC:http://dl.dropbox.com/u/1338197/1/qwit.spec
SRPM:http://dl.dropbox.com/u/1338197/1/qwit-1.1-0.1.beta.fc13.src.rpm
Comment 1 Kalev Lember 2010-06-26 15:32:00 EDT
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.
Comment 2 Kalev Lember 2010-06-26 15:40:10 EDT
The Debian package installs a man page. Please include it in this package and submit it for upstream inclusion too.
Comment 3 Chen Lei 2010-06-28 02:59:04 EDT
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
Comment 4 Kalev Lember 2010-06-28 03:27:11 EDT
%{_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
Comment 5 Kalev Lember 2010-06-28 04:23:46 EDT
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 )
Comment 6 Chen Lei 2010-06-28 04:31:20 EDT
(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.
Comment 7 Chen Lei 2010-06-28 04:35:46 EDT
(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.
Comment 8 Kalev Lember 2010-06-30 07:09:36 EDT
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
Comment 9 Chen Lei 2010-07-01 23:18:26 EDT
New Package CVS Request
=======================
Package Name: qwit
Short Description: Lightweight desktop client for Twitter microblogging service
Owners: supercyper
Branches: F-12 F-13
InitialCC:
Comment 10 Chen Lei 2010-07-01 23:19:27 EDT
(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!
Comment 11 Jason Tibbitts 2010-07-02 00:50:05 EDT
CVS done (by process-cvs-requests.py).
Comment 12 Chen Lei 2010-07-02 02:58:22 EDT
Moved icons to /usr/share/icons/hicolor/*/*/ as comment#5, close this report now.

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