Bug 506232

Here's my review for this:

* Buildroot doesn't obey the guidelines:
   http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* The source files claim GPLv2+ but the COPYING file says GPLv3. This software cannot be GPLv3 because it links to kdebase-workspace which is GPLv2. GPLv2 and GPLv3 don't go very well together. See:
   http://fedoraproject.org/wiki/Licensing#GPL_Compatibility_Matrix
The license tag should be GPLv2+ but please ask upstream and verify this. Also, please don't package the false COPYING file.

* Source0 must be given in full URL. See:
   https://fedoraproject.org/wiki/Packaging/SourceURL#Referencing_Source

* build.log says
   NOTE: msgfmt not found. Translations will *not* be installed
You need to add gettext to BR.

! Please make the description span 80 columns (as much as possible).

* Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. Follow:
   http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
Also, rpmlint says:
   rekonq.x86_64: E: invalid-desktopfile /usr/share/applications/kde4/rekonq.desktop

* ScriptletSnippets aren't used properly. Follow:
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

! Please use the %{name} macro extensively for consistency

* A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
Currently, package does not own %{_kde4_appsdir}/rekonq/ . You can replace
   %{_kde4_appsdir}/rekonq/rekonqui.rc
   %{_kde4_appsdir}/rekonq/pics/loading.gif
   %{_kde4_appsdir}/rekonq/defaultbookmarks.xbel
   %{_kde4_appsdir}/rekonq/htmls/notfound.html
   %{_kde4_appsdir}/rekonq/pics/webkit-icon.png
with just
   %{_kde4_appsdir}/%{name}/
to deal with this issue.

? You are using versioned BR. Why are you not using the corresponding versioned R? 

! Also the versions you use for BRs do not match what CMakeLists.txt file says.

Orcan,

Thank you very much for your detailed and helpful package review ( you found a bit more issues than I was hoping for :) ). I think I've covered most of the issues you found by now. I'm now waiting for upstream to comment on the license part.

I'll post the updated files as soon as I've fully (I hope) sorted things out (can't access my Fedora account from here either). 

- Eelko

Ping. Status update on this?

Sorry for the wait, guys. I've been on vacation. 

As far as I have figured out for now ( I've got tons of e-mail to go through :/ ), this is still an upstream issue and under discussion for the next release.

Here's an upstream reference: http://mail.kde.org/pipermail/rekonq/2009-July/000230.html

Still ongoing as far as I know.

Shipping the GPLv3 with a software which is GPLv2+ is legal. You can specify any minimum version, it doesn't have to be the one you ship. You're only required to ship a copy of any (one) version of the GPL which is valid for your software, not all of them. So, I don't see any licensing issue there, it's safe to mark this as GPLv2+ if the license header says so.

Plus, are you sure the parts of kdebase-workspace which are being used are not actually LGPL or GPLv2+? GPL v2 only is deprecated in KDE, most stuff is not actually v2 only. But this isn't really relevant as the code is GPLv2+ anyway.

Finally an update;

Spec URL: http://kaboon.fedorapeople.org/rekonq/0.2.0/rekonq.spec

SRPM URL:
http://kaboon.fedorapeople.org/rekonq/0.2.0/F11/rekonq-0.2.0-1.fc11.src.rpm

I fixed the spec file and updated to the latest upstream version. I think I
fixed most, if not all, issues reported earlier.

Hi Eelko, thanks for the update.

I had a look at the new package:

* The BR's are messed up. You want to BR qt-devel and kde-workspace-devel. Not just qt and kde-workspace! Otherwise it won't build on koji.

I suggest you running mock or the scratch build feature of koji before posting updates.

Other than this, the following issues are still there:

(from comment #1)

> * ScriptletSnippets aren't used properly. Follow:
>    http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
> 

> * Currently, package does not own %{_kde4_appsdir}/rekonq/ .

Alright, thanks again Orcan.

I intentionally mixed/messed up the BR's. Somehow I thought it the -devel packes  weren't needed to build it as it build perfectly fine without them on my system (afaik). I'll change that back.

While I'm at it, I'll also try your other suggestions and fixes and will get back on it as soon as possible.

Yet again, thanks so far!

- Eelko

Alright, here it goes again. The scratch build was succesfull and I think I have resolved the remaining issues also. I sorted out the BR's and the ScriptletSnippets as far as I can tell. I'm not sure if I dealt correctly the ownership issue.

Spec URL: http://kaboon.fedorapeople.org/rekonq/0.2.0/rekonq.spec

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1642555

Hi Eelko,
The directory is still unowned. I gave the solution to this problem above in comment #1. Let me rephrase:

   %{_kde4_appsdir}/%{name}/* owns everything inside the directory but does not own the directory itself.

   %dir %{_kde4_appsdir}/%{name} owns just the directory but does not own the contents of the directory.

   %{_kde4_appsdir}/%{name}/ owns both the directory and its contents. This is what you want.

Alternatively you can do
   %dir %{_kde4_appsdir}/%{name}
   %{_kde4_appsdir}/%{name}/*
which is longer but has the same effect.

Hi Orcan,

Thank you very much for that last comment with example! I now fully understand the issue which is being referred to.

I will get that fixed this evening.

'lo Orcan,

I chose the last way to add the ownership. I think it's easier to read and better to understand (for me at least - so I will not forget it next time when I submit a package for review). :)

Hopefully the last and final update;

Spec URL: http://kaboon.fedorapeople.org/rekonq/0.2.0/rekonq.spec

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1646581

- Eelko

Hi Eelko,
Sorry, I've been busy with life. I will finish the review later this week.

Everything seems good now. Sorry for the delay!

-----------------------------------------
This package (rekonq) is APPROVED by oget
-----------------------------------------

Thank you very much Orcan. Getting this package reviewed has been quite a good learning process for me. I don't think I'll ever miss out on those pesky little details again. :)

New Package CVS Request
=======================
Package Name: rekonq
Short Description: KDE browser based on QtWebkit
Owners: kaboon
Branches: F-11 F-12
InitialCC: kaboon

cvs done.

rekonq-0.2.0-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rekonq-0.2.0-3.fc11

I am sorry and I've should have done this straight away;

Package Change Request
======================
Package Name: rekonq
New Branches: F-10
Owners: kaboon

CVS done.

rekonq-0.2.0-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rekonq-0.2.0-3.fc10

rekonq-0.2.0-3.fc10 has been pushed to the Fedora 10 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 rekonq'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-9826

rekonq-0.2.0-3.fc11 has been pushed to the Fedora 11 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 rekonq'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-9865

Should this be closed now that it is in the repos?

Bodhi is going to close it once the updates go stable. (By the way, they should be queued for stable now, one week of testing is enough!) But review requests can in principle be closed as soon as the package is imported.

Now marked as stable. I'll leave it up to Bodhi to close this one for me and next time I'll do it myself. :)

Thanks

rekonq-0.2.0-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

rekonq-0.2.0-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.