Bug 758734 - Review Request: fatrat-subtitlesearch - FatRat plugin enabling OpenSubtitles.org and Sublight.si integration
Summary: Review Request: fatrat-subtitlesearch - FatRat plugin enabling OpenSubtitles....
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Volker Fröhlich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-30 15:47 UTC by Jan Vcelak
Modified: 2013-03-04 01:29 UTC (History)
4 users (show)

Fixed In Version: fatrat-subtitlesearch-1.2.0-0.3.beta1.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-11-26 10:20:57 UTC
Type: ---
volker27: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jan Vcelak 2011-11-30 15:47:32 UTC
Spec URL:
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/1.2.0_beta1-1/fatrat-subtitlesearch.spec

SRPM URL:
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/fatrat-subtitlesearch-1.2.0_beta1-1.fc17.src.rpm

Description:
FatRat is download manager written in C++ and build on top of the Qt4 library.
It is rich in features and is continuously extended.

This package contains plugin for integration with OpenSubtitles.org and
Sublight.si (easy subtitle searching).


$ rpmlint fatrat-subtitlesearch.spec fatrat-subtitlesearch-1.2.0_beta1-1.fc17.src.rpm fatrat-subtitlesearch-1.2.0_beta1-1.fc17.x86_64.rpm

fatrat-subtitlesearch.src: W: spelling-error Summary(en_US) si -> chi, sigh, see
fatrat-subtitlesearch.src: W: spelling-error %description -l en_US si -> chi, sigh, see
fatrat-subtitlesearch.x86_64: W: spelling-error Summary(en_US) si -> chi, sigh, see
fatrat-subtitlesearch.x86_64: W: spelling-error %description -l en_US si -> chi, sigh, see
2 packages and 1 specfiles checked; 0 errors, 4 warnings

(New fatrat from F17 is required for this package to be built.)

Comment 1 Volker Fröhlich 2011-12-09 02:01:27 UTC
* Package doesn't build on F16

* "Beta" must be in release, not version: http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

* Drop the version requirement on Qt, we don't have versions older than 4.5

* The files indicate GPLv2, not v2+

* You can use the name macro in Source0 and in the files section

* Please add a comment on what your patch does and why you're deleting _docdir

Comment 2 Jan Vcelak 2011-12-12 13:23:26 UTC
Spec URL (updated):
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/1.2.0-0.1.beta1/fatrat-subtitlesearch.spec

SRPM URL (updated):
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/fatrat-subtitlesearch-1.2.0-0.1.beta1.fc17.src.rpm

> * Package doesn't build on F16

The build depends on the new version of fatrat, which is not in stable updates yet. Therefore you need to install fatrat-devel manually into your build environment. I have crated a build override in Koji, so it builds in Koji:

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

> * "Beta" must be in release, not version:
> http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

Thank you for catching this. I didn't know that. It is fixed now.

> * Drop the version requirement on Qt, we don't have versions older than 4.5

Done.

> * The files indicate GPLv2, not v2+

You are right. Fixed.

> * You can use the name macro in Source0 and in the files section

Well, I left this untouched because I think this is more readable.

> * Please add a comment on what your patch does and why you're deleting _docdir

I always put comments in the patch headers. So it is here.

%{_docdir} is deleted because upstream installs it into a wrong subdirectory. All documentation is installed later in %files section into correct location.

Comment 3 Volker Fröhlich 2011-12-12 19:13:47 UTC
Concerning the name macro: I recently discovered spectool. Spectool substitutes the macros and prints the result. The guidelines don't force you to use it, so it's up to you. Though, in your case, the name is pretty long and I can't see a potential disadvantage.

Is there something wrong with the locales? I see they're not installed.

OK, I'm fine with the comment in the diff. It's a "SHOULD": http://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

I meant to add the %docdir comment in the spec file, not here. :)

Comment 4 Jan Vcelak 2012-02-08 14:29:34 UTC
Sorry for the delay.

Spec URL (updated):
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/1.2.0-0.2.beta1/fatrat-subtitlesearch.spec

SRPM URL (updated):
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/fatrat-subtitlesearch-1.2.0-0.2.beta1.fc17.src.rpm

> Concerning the name macro: I recently discovered spectool. Spectool substitutes
> the macros and prints the result. The guidelines don't force you to use it, so
> it's up to you. Though, in your case, the name is pretty long and I can't see a
> potential disadvantage.

OK, I've added the macro.

> Is there something wrong with the locales? I see they're not installed.

Hmm, missing cmake flag. Fixed now.

> I meant to add the %docdir comment in the spec file, not here. :)

It's there now.

Comment 5 Jan Vcelak 2012-03-09 12:20:47 UTC
Volker, are you going to finish this review? Or should I ask someone? Do you have some packages on review too, do you want me to review some?

Comment 6 Volker Fröhlich 2012-03-13 07:38:27 UTC
I'd take it, but it could take a few days. If you don't mind that, I'm fine.

Comment 7 Jan Vcelak 2012-03-13 09:40:23 UTC
No problem. Take your time. Thanks.

Comment 8 Volker Fröhlich 2012-04-10 17:14:22 UTC
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
     Note: The package did not built BR could therefore not be checked or the
     package failed to build because of missing BR
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[!]: MUST Changelog in prescribed format.

     I guess you added the epoch when you changed the release style to pre-release. Since no actual package was published yet, we can and should step back from that and just drop the epoch.

[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[-]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[ ]: MUST Package does not generates any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[-]: MUST Package must own all directories that it creates.
[!]: MUST Package does not own files or directories owned by other packages.

     This package owns %{_datadir}/fatrat/lang, owned by fatrat

[ ]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.
[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/makerpm/758734/fatrat-subtitlesearch-1.2.0_beta1.tar.gz :
  MD5SUM this package     : ad8a24060826bcde1d85581e6844ea29
  MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

[!]: MUST Spec file is legible and written in American English.

Please make it: "FatRat is a download manager" and "built"

[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[!]: SHOULD Package functions as described.

After starting fatrat-1.2.0-0.3.beta1.fc16.x86_64 , I see ...
"2012-04-10 18:57:17 - Loaded a plugin: libfatrat-subtitlesearch.so
2012-04-10 18:57:17 - WARNING: the plugin is incompatible: libfatrat-subtitlesearch.so"
... on the log tab.

[x]: SHOULD Package does not include license text files separate from
     upstream.
[!]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.

     Please inform the developer, that we want to be able to use a system-wide qtsoap. They could either un-bundle it or change their build system to allow for it without patching.

[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Patch100: fatrat-subtitlesearch-qtsoap.patch (fatrat-
     subtitlesearch-qtsoap.patch)
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[-]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

[!]: MUST The locales aren't handled properly yet:
Please see http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

Comment 9 Volker Fröhlich 2012-05-11 20:00:53 UTC
Did you find the time to update the package yet?

Comment 10 Jan Vcelak 2012-05-12 19:01:27 UTC
Yes, but it is not ready yet. Sorry, I'm quite busy recently.

Comment 11 Jan Vcelak 2012-05-22 09:13:24 UTC
(In reply to comment #8)
> [!]: MUST Changelog in prescribed format.
> 
>      I guess you added the epoch when you changed the release style to
> pre-release. Since no actual package was published yet, we can and should
> step back from that and just drop the epoch.

Fixed.

> [!]: MUST Package does not own files or directories owned by other packages.
> 
>      This package owns %{_datadir}/fatrat/lang, owned by fatrat

Fixed.

> [!]: MUST Sources used to build the package match the upstream source, as
>      provided in the spec URL.
> /home/makerpm/758734/fatrat-subtitlesearch-1.2.0_beta1.tar.gz :
>   MD5SUM this package     : ad8a24060826bcde1d85581e6844ea29
>   MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

This is a false positive.

$ spectool -S fatrat-subtitlesearch.spec 
Source0: http://www.dolezel.info/download/data/fatrat-subtitlesearch/fatrat-subtitlesearch-1.2.0_beta1.tar.gz
$ curl -s http://www.dolezel.info/download/data/fatrat-subtitlesearch/fatrat-subtitlesearch-1.2.0_beta1.tar.gz | md5sum -
ad8a24060826bcde1d85581e6844ea29  -

Something went wrong with your download probably. The file must have been emtpy.

$ echo -n | md5sum
d41d8cd98f00b204e9800998ecf8427e  -

> [!]: MUST Spec file is legible and written in American English.
> 
> Please make it: "FatRat is a download manager" and "built"

Fixed.

> [!]: SHOULD Package functions as described.
> 
> After starting fatrat-1.2.0-0.3.beta1.fc16.x86_64 , I see ...
> "2012-04-10 18:57:17 - Loaded a plugin: libfatrat-subtitlesearch.so
> 2012-04-10 18:57:17 - WARNING: the plugin is incompatible:
> libfatrat-subtitlesearch.so"
> ... on the log tab.

I'm getting just a warning:

dlopen "/usr/lib64/fatrat/plugins/libfatrat-subtitlesearch.so" 
WARNING! Version conflict. "libfatrat-subtitlesearch.so" is 1.1.3 but FatRat is 1.2.0_beta1

Anyway the plugin works fine, the version exported by the library is wrong. I do not think this is critical for this review. I will report this problem to the maintainer.

> [!]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
>      justified.
> 
>      Please inform the developer, that we want to be able to use a
> system-wide qtsoap. They could either un-bundle it or change their build
> system to allow for it without patching.

I'm always writing comments directly to patch headers. And I know the developer in person, we already talked about it. It will be hopefully resolved upstream in future.

> [!]: SHOULD SourceX / PatchY prefixed with %{name}.
>      Note: Patch100: fatrat-subtitlesearch-qtsoap.patch (fatrat-
>      subtitlesearch-qtsoap.patch)

Fixed.

> [!]: MUST The locales aren't handled properly yet:
> Please see
> http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files

Fixed.


Spec URL (updated):
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/1.2.0-0.3.beta1/fatrat-subtitlesearch.spec

SRPM URL (updated):
http://jvcelak.fedorapeople.org/review/fatrat-subtitlesearch/fatrat-subtitlesearch-1.2.0-0.3.beta1.fc17.src.rpm

Comment 12 Jan Vcelak 2012-08-06 16:23:10 UTC
ping

Comment 13 Volker Fröhlich 2012-08-09 15:00:21 UTC
You're next! :)

Comment 14 Jan Vcelak 2012-08-10 11:52:47 UTC
> You're next! :)

Did I miss something?

Comment 15 Volker Fröhlich 2012-08-10 15:38:24 UTC
==APPROVED==

Sorry for the long delay!

Spelling mistakes are gone
Directory ownership is fine now
Locales are fine
Name macro on the patch is fine
Source checksum was a false positive, as you said

I trust in you, concerning the functionality of the package.

The patch is commented, but there is no indication in the spec file or the patch, telling you informed upstream. According to http://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment this is a SHOULD and thus does not block the review.

About the epoch:

The guidelines do not forbid epochs, even when unnecessary: http://fedoraproject.org/wiki/Packaging:Guidelines#Use_of_Epochs

Nevertheless, this draft document makes it clear, they are not desireable, if not necessary:

http://fedoraproject.org/wiki/PackagingDrafts/Epoch#Use_of_Epochs

I think it should be fine to drop the epoch, just say "1" instead of the macro and summarize or manipulate the changelog. If the epoch of Fatrat would change, you'd have to change it manually for this package anyway.

Of course feel free to ask for other opinions!

Comment 16 Jan Vcelak 2012-08-10 17:58:55 UTC
OK. Thank you very much.

Comment 17 Jan Vcelak 2012-08-10 18:00:06 UTC
New Package SCM Request
=======================
Package Name: fatrat-subtitlesearch
Short Description: FatRat plugin enabling OpenSubtitles.org and Sublight.si integration
Owners: jvcelak
Branches: f16 f17 f18
InitialCC:

Comment 18 Gwyn Ciesla 2012-08-10 18:01:49 UTC
Git done (by process-git-requests).

Comment 19 Volker Fröhlich 2012-11-25 19:43:07 UTC
Jan, seems like you didn't submit builds yet.

Comment 20 Jan Vcelak 2012-11-26 09:30:20 UTC
You are right, I didn't. Sorry for that, I'm quite busy recently. Will do it soon. :-)

Comment 21 Fedora Update System 2012-11-26 10:13:29 UTC
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/fatrat-subtitlesearch-1.2.0-0.3.beta1.fc18

Comment 22 Fedora Update System 2012-11-26 10:14:06 UTC
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/fatrat-subtitlesearch-1.2.0-0.3.beta1.fc17

Comment 23 Fedora Update System 2012-11-26 10:19:05 UTC
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/fatrat-subtitlesearch-1.2.0-0.3.beta1.fc16

Comment 24 Jan Vcelak 2012-11-26 10:20:57 UTC
Finished. Thanks to everyone involved.

Comment 25 Fedora Update System 2012-12-01 09:52:49 UTC
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc18 has been pushed to the Fedora 18 stable repository.

Comment 26 Fedora Update System 2012-12-15 17:52:09 UTC
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc17 has been pushed to the Fedora 17 stable repository.

Comment 27 Fedora Update System 2012-12-15 17:59:26 UTC
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.


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