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.)
* 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
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.
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. :)
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.
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?
I'd take it, but it could take a few days. If you don't mind that, I'm fine.
No problem. Take your time. Thanks.
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
Did you find the time to update the package yet?
Yes, but it is not ready yet. Sorry, I'm quite busy recently.
(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
ping
You're next! :)
> You're next! :) Did I miss something?
==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!
OK. Thank you very much.
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:
Git done (by process-git-requests).
Jan, seems like you didn't submit builds yet.
You are right, I didn't. Sorry for that, I'm quite busy recently. Will do it soon. :-)
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
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
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
Finished. Thanks to everyone involved.
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc18 has been pushed to the Fedora 18 stable repository.
fatrat-subtitlesearch-1.2.0-0.3.beta1.fc17 has been pushed to the Fedora 17 stable repository.
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.