Bug 565376 - Review Request: qstardict - StarDict clone written in Qt4
Summary: Review Request: qstardict - StarDict clone written in Qt4
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-02-15 05:24 UTC by Robin Lee
Modified: 2010-05-25 21:05 UTC (History)
6 users (show)

Fixed In Version: qstardict-0.13.1-3.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-03-17 07:53:54 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Robin Lee 2010-02-15 05:24:41 UTC
Spec URL: http://dl.dropbox.com/u/612778/qstardict.spec
SRPM URL: http://dl.dropbox.com/u/612778/qstardict-0.13.1-1.fc12.src.rpm
Description: 

QStarDict is a StarDict clone written in Qt4. The user interface
is similar to StarDict.

Main features
* Full support of StarDict dictionaries
* Working in system tray
* Scanning mouse selection and showing popup window with translation of
selected word
* Translations reformatting
* Pronouncing translated word
* Plugins support
* KDE 4 plasmoid


rpmlint checks show clean.

Comment 1 Chen Lei 2010-02-16 13:00:44 UTC
Some notes here:
Missing %{?_kde4_macros_api:Requires: kde4-macros(api) = %{_kde4_macros_api} }
before the %description section

Source:     %{name}-%{version}.tar.bz2 ->
Source:     http://qstardict.ylsoftware.com/files/%{name}-%{version}.tar.bz2

Requires:   %name = %version ->
Requires:   %{name} = %{version}-%{release} 

qmake-qt4 PLUGINS_DIR=%_libdir/%name/plugins ENABLED_PLUGINS="stardict web" ->
%_qt4_qmake PLUGINS_DIR=%{_libdir}/%{name}/plugins ENABLED_PLUGINS="stardict web"

%__make -> make(no need to use this macro)

desktop-file-install --vendor="fedora" --delete-original    \
    --dir=%{buildroot}%{_datadir}/applications              \
     %{buildroot}%{_datadir}/applications/%{name}.desktop
->
desktop-file-install --vendor=""    \
    --dir=%{buildroot}%{_datadir}/applications              \
     %{buildroot}%{_datadir}/applications/%{name}.desktop
New packages should not use --vendor="fedora".

rm -fr %buildroot -> rm -fr %{buildroot}
rm -fr %buildroot%_datadir/doc -> rm -fr %{buildroot}%{_docdir}

Comment 2 Robin Lee 2010-02-16 15:10:13 UTC
Corrected all following your instructions.
Updated spec and srpm links.
Thank you!

(In reply to comment #1)
> Some notes here:
> Missing %{?_kde4_macros_api:Requires: kde4-macros(api) = %{_kde4_macros_api} }
> before the %description section
> 
> Source:     %{name}-%{version}.tar.bz2 ->
> Source:     http://qstardict.ylsoftware.com/files/%{name}-%{version}.tar.bz2
> 
> Requires:   %name = %version ->
> Requires:   %{name} = %{version}-%{release} 
> 
> qmake-qt4 PLUGINS_DIR=%_libdir/%name/plugins ENABLED_PLUGINS="stardict web" ->
> %_qt4_qmake PLUGINS_DIR=%{_libdir}/%{name}/plugins ENABLED_PLUGINS="stardict
> web"
> 
> %__make -> make(no need to use this macro)
> 
> desktop-file-install --vendor="fedora" --delete-original    \
>     --dir=%{buildroot}%{_datadir}/applications              \
>      %{buildroot}%{_datadir}/applications/%{name}.desktop
> ->
> desktop-file-install --vendor=""    \
>     --dir=%{buildroot}%{_datadir}/applications              \
>      %{buildroot}%{_datadir}/applications/%{name}.desktop
> New packages should not use --vendor="fedora".
> 
> rm -fr %buildroot -> rm -fr %{buildroot}
> rm -fr %buildroot%_datadir/doc -> rm -fr %{buildroot}%{_docdir}

Comment 3 Chen Lei 2010-02-17 02:52:46 UTC
update-desktop-database is needed only if your desktop file has MimeType, so you can remove it from spec.
%post
update-desktop-database &> /dev/null ||:

%postun
if [ $1 -eq 0 ] ; then
update-desktop-database &> /dev/null ||:
fi

Comment 4 Robin Lee 2010-02-17 03:30:02 UTC
Thank you!
URLs updated again.

(In reply to comment #3)
> update-desktop-database is needed only if your desktop file has MimeType, so
> you can remove it from spec.
> %post
> update-desktop-database &> /dev/null ||:
> 
> %postun
> if [ $1 -eq 0 ] ; then
> update-desktop-database &> /dev/null ||:
> fi

Comment 5 Naveen Kumar 2010-02-24 10:54:51 UTC
Here's an unofficial review, using the Tibbs checklist as reference from: http://fedoraproject.org/wiki/User:Tibbs/Review_Template


+ source files match upstream: 
sha256sum from both source and upstream were:
52cb7b699159a804158045d2ae79bf80ce28e2febc2bb3e808784242d829c9ee  qstardict-0.13.1.tar.bz2

+ package meets naming and versioning guidelines.
+ specfile is properly named, is cleanly written and uses macros consistently.
+ dist tag is present.
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible: license text included in package.
+ latest version is being packaged.
+ BuildRequires are proper.
+ compiler flags are appropriate.
+ %clean is present.
+ package builds in Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2011001
+ package installs properly.
* rpmlint is gives warning but no errors. (rpmlint version 0.94)

	rpmlint OUTPUT on qstardict-0.13.1-1.fc12.src.rpm & qstardict-0.13.1-1.fc12.x86_64.rpm:
qstardict.src: W: spelling-error %description -l en_US popup -> pop up, pop-up, popular
qstardict.src: W: invalid-url URL: http://qstardict.ylsoftware.com IncompleteRead(0 bytes read)
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


* final provides and requires are sane:
	rpm -qp --provides OUTPUT on qstardict-0.13.1-1.fc12.x86_64.rpm:
libstardict.so()(64bit)  
libweb.so()(64bit)  
qstardict = 0.13.1-1.fc12
qstardict(x86-64) = 0.13.1-1.fc12

	rpm -qp --requires OUTPUT on qstardict-0.13.1-1.fc12.x86_64.rpm:
libQtCore.so.4()(64bit)  
libQtDBus.so.4()(64bit)  
libQtGui.so.4()(64bit)  
libQtNetwork.so.4()(64bit)  
libQtXml.so.4()(64bit)  
libX11.so.6()(64bit)  
libc.so.6()(64bit)  
libc.so.6(GLIBC_2.2.5)(64bit)  
libc.so.6(GLIBC_2.3.4)(64bit)  
libc.so.6(GLIBC_2.4)(64bit)  
libgcc_s.so.1()(64bit)  
libgcc_s.so.1(GCC_3.0)(64bit)  
libglib-2.0.so.0()(64bit)  
libm.so.6()(64bit)  
libpthread.so.0()(64bit)  
libpthread.so.0(GLIBC_2.2.5)(64bit)  
libstdc++.so.6()(64bit)  
libstdc++.so.6(CXXABI_1.3)(64bit)  
libstdc++.so.6(GLIBCXX_3.4)(64bit)  
libz.so.1()(64bit)  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  
rpmlib(PayloadIsXz) <= 5.2-1


+ shared libraries are added: ldconfig not run since it acts as plugin and does not provide API to be used.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ scriptlets sane
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ desktop files valid and installed properly.

Comment 6 Mamoru TASAKA 2010-02-27 20:37:58 UTC
From next time please change the release number every time
you modify your spec file (unless version number changes)

Some notes:

* License
  - License tag should be "GPLv2+" (the developer can change the
    license freely even from GPLv3, if all the contributor for
    the source codes agree it)

* BuildRequires
  - qt-devel is pulled in by kdelibs-devel, so explicitly writing
    "BR: qt-devel" is not needed.

  - Also using vitrual dependency related BuildRequires is preferable
    (i.e. "BR: kdelibs4-devel" is preferable)

? Enabled plugin
  - Would you explain why you enable only "stardict web" plugins?

* Macros
  - Choose %{__make} or make, not use both.

* %defattr
  - Now we prefer to use %defattr(-,root,root,-)

* Build failure
  - Your srpm does not build on F-13:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=2018034
    This is because Fedora 13 changed the behavior of linker:
    http://fedoraproject.org/wiki/UnderstandingDSOLinkChange

    To check F-13 linker behavior on F-12/11, you can do this by
    'make %{?_smp_mflags} LINK="g++ -Wl,--no-add-needed"' for
    this package.

Comment 7 Robin Lee 2010-03-05 18:06:29 UTC
* Followed all your instructions.
* The default enabled plugins are just 'stardict' and 'web'. I removed explicit plugin claim in this revision.
* Build successfully now on F-13:
https://koji.fedoraproject.org/koji/taskinfo?taskID=2033580

Spec URL: http://dl.dropbox.com/u/612778/qstardict.spec
SRPM URL: http://dl.dropbox.com/u/612778/qstardict-0.13.1-2.fc12.src.rpm


(In reply to comment #6)
> From next time please change the release number every time
> you modify your spec file (unless version number changes)
> 
> Some notes:
> 
> * License
>   - License tag should be "GPLv2+" (the developer can change the
>     license freely even from GPLv3, if all the contributor for
>     the source codes agree it)
> 
> * BuildRequires
>   - qt-devel is pulled in by kdelibs-devel, so explicitly writing
>     "BR: qt-devel" is not needed.
> 
>   - Also using vitrual dependency related BuildRequires is preferable
>     (i.e. "BR: kdelibs4-devel" is preferable)
> 
> ? Enabled plugin
>   - Would you explain why you enable only "stardict web" plugins?
> 
> * Macros
>   - Choose %{__make} or make, not use both.
> 
> * %defattr
>   - Now we prefer to use %defattr(-,root,root,-)
> 
> * Build failure
>   - Your srpm does not build on F-13:
>     http://koji.fedoraproject.org/koji/taskinfo?taskID=2018034
>     This is because Fedora 13 changed the behavior of linker:
>     http://fedoraproject.org/wiki/UnderstandingDSOLinkChange
> 
>     To check F-13 linker behavior on F-12/11, you can do this by
>     'make %{?_smp_mflags} LINK="g++ -Wl,--no-add-needed"' for
>     this package.

Comment 8 Mamoru TASAKA 2010-03-09 08:27:40 UTC
Okay.

----------------------------------------------------
  This package (qstardict) is APPROVED by mtasaka
----------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 11/12/13, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

Comment 9 Robin Lee 2010-03-09 15:10:28 UTC
New Package CVS Request
=======================
Package Name: qstardict
Short Description: StarDict clone written in Qt4
Owners: cheeselee
Branches: F-11 F-12 F-13
InitialCC:

Comment 10 Jason Tibbitts 2010-03-10 22:24:17 UTC
CVS done (by process-cvs-requests.py).

Comment 11 Robin Lee 2010-03-12 06:02:41 UTC
Package Change Request
======================
Package Name: qstardict
Owners: cheeselee
Updated Description: StarDict clone written using Qt4

Comment 12 Fedora Update System 2010-03-12 07:34:25 UTC
qstardict-0.13.1-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/qstardict-0.13.1-3.fc13

Comment 13 Fedora Update System 2010-03-13 02:34:05 UTC
qstardict-0.13.1-3.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 qstardict'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/qstardict-0.13.1-3.fc13

Comment 14 Mamoru TASAKA 2010-03-17 07:53:54 UTC
Closing.

Comment 15 Kevin Fenzi 2010-03-17 18:17:36 UTC
No need to have cvs change description. It should update from the description in the package.

Comment 16 Fedora Update System 2010-03-23 02:02:27 UTC
qstardict-0.13.1-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Robin Lee 2010-05-24 05:24:40 UTC
Package Change Request
======================
Package Name: qstardict
New Branches: EL-6
Owners: cheeselee

Comment 18 Dennis Gilmore 2010-05-25 21:05:14 UTC
CVS Done


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