Bug 570017

Summary: Review Request: k4dirstat - KDE4 version of kdirstat
Product: [Fedora] Fedora Reporter: Laurent Rineau <laurent.rineau__fedora>
Component: Package ReviewAssignee: Kalev Lember <kalevlember>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, kalevlember, kryzhev, notting, rdieter
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-10-10 04:43:28 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Laurent Rineau 2010-03-02 18:05:08 EST
Spec URL: http://www.rineau.fr/k4dirstat.spec
SRPM URL: http://www.rineau.fr/k4dirstat-0-0.2.20100223gitd3b530af3.fc12.src.rpm
Description: 
KDirStat (KDE Directory Statistics) is a utility program that sums up
disk usage for directory trees - very much like the Unix 'du' command.
It can also help you clean up used space.

K4DirStat is the port to KDE4.

---------------
For the moment k4dirstat is quite new. But it is usable.
See http://grumpypenguin.org/index.php?/archives/3-KDirStat-coming-to-KDE4.html

rpmlint says:
/home/lrineau/RPM/SPECS/k4dirstat.spec: W: invalid-url Source0: k4dirstat-20100223gitd3b530af3.tar.gz
k4dirstat.src: W: spelling-error %description -l en_US du -> dew, doe, Du
k4dirstat.src: W: invalid-url Source0: k4dirstat-20100223gitd3b530af3.tar.gz
k4dirstat.i686: W: spelling-error %description -l en_US du -> dew, doe, Du
2 packages and 1 specfiles checked; 0 errors, 4 warnings.

As far as I understand, that is OK.
Comment 1 Laurent Rineau 2010-03-06 09:25:51 EST
New release. The upstream author had made changes, after my remarks.

Spec URL: http://www.rineau.fr/k4dirstat.spec
SRPM URL: http://www.rineau.fr/k4dirstat-0-0.3.20100304gitec01dd42.fc12.src.rpm

* Sat Mar  6 2010  <Laurent.Rineau__fedora@normalesup.org> - 0-0.3.20100304gitec01dd42%{?dist}
- New upstream version.
- New doc files (added upstream). Among them: COPYING, README, AUTHORS.
- Patch0 is merged upstream.


I have also made a mock build successfully. And verified that the package does not conflict with kdirstat (the version for kDE3).
Comment 2 Kalev Lember 2010-03-11 05:39:05 EST
Taking for review.

Build for current rawhide fails with the following error:
/usr/bin/ld: CMakeFiles/k4dirstat.dir/kdirtreecache.o: undefined reference to symbol 'gzclose'
/usr/bin/ld: note: 'gzclose' is defined in DSO /lib/libz.so.1 so try adding it to the linker command line
/lib/libz.so.1: could not read symbols: Invalid operation
collect2: ld returned 1 exit status

This is caused by a change in the default linker behaviour, see http://fedoraproject.org/wiki/Features/ChangeInImplicitDSOLinking for details.


You might want to use globs in %files section to make the package easier to maintain. It's entirely up to you, tough, if you want to use globs in there. For example,
%{_kde4_iconsdir}/hicolor/16x16/apps/k4dirstat.png
%{_kde4_iconsdir}/hicolor/32x32/apps/k4dirstat.png
%{_kde4_iconsdir}/hicolor/48x48/apps/k4dirstat.png

could be replaced by:
%{_kde4_iconsdir}/hicolor/*/apps/k4dirstat.png

and
%{_kde4_docdir}/HTML/en/k4dirstat/common
%{_kde4_docdir}/HTML/en/k4dirstat/index.cache.bz2
%{_kde4_docdir}/HTML/en/k4dirstat/index.docbook

could be replaced by:
%{_kde4_docdir}/HTML/en/k4dirstat/*


Fix %changelog: don't use %{?dist} macro in there, specify your name and use a valid email address.
* Tue Mar  2 2010  <Laurent.Rineau__fedora@normale> - 0-0.2.20100223gitd3b530af3%{?dist}
should be changed to:
* Tue Mar  2 2010 Laurent Rineau <laurent.rineau__fedora@normalesup.org> - 0-0.2.20100223gitd3b530af3


Use %global instead of %define as per https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define


> Source0:        %{name}-%{preversion}.tar.gz
You'll need to add a comment how to generate the git snapshot tarball. See https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
Comment 3 Kalev Lember 2010-03-23 17:01:56 EDT
ping?
Comment 4 Kalev Lember 2010-04-13 15:41:25 EDT
ping

Are you still interested in getting this package included in Fedora?
Comment 5 Laurent Rineau 2010-04-13 16:04:59 EDT
Yes. But my daily job is not friendly with me for one month. Maybe I should postpone that package submission for a while.

What is more, I do not know how to deal with the new link behavior of F-13. I am not used to it.
Comment 6 Laurent Rineau 2010-06-08 18:00:03 EDT
Spec URL: http://www.rineau.fr/k4dirstat.spec
SRPM URL: 
http://www.rineau.fr/k4dirstat-0-0.4.20100304gitec01dd42.fc13.src.rpm

Changelog:
* Tue Jun  8 2010  <Laurent.Rineau__fedora@normalesup.org> - 0-0.6.20100304gitec01dd42
- %%{_kde4_docdir}/HTML/en/k4dirstat/ must be owned.
- use %%global instead of %%define

* Tue Jun  8 2010  <Laurent.Rineau__fedora@normalesup.org> - 0-0.5.20100304gitec01dd42
- Fix the changelog (bad email and bad use of %%{?dist}).

* Tue Jun  8 2010  <Laurent.Rineau__fedora@normalesup.org> - 0-0.4.20100304gitec01dd42
- Patch0 for F-13: link explicitly with zlib.
- Add a comment on Source0.

(In reply to comment #2)
> Taking for review.
> 
> Build for current rawhide fails with the following error:
> /usr/bin/ld: CMakeFiles/k4dirstat.dir/kdirtreecache.o: undefined reference to
> symbol 'gzclose'

My machines now run F-13. I have been able to test the issue, and patched k4dirstat so that it links with zlib explicitly.

> You might want to use globs in %files section to make the package easier to
> maintain. It's entirely up to you, tough, if you want to use globs in there.
> For example,
> %{_kde4_iconsdir}/hicolor/16x16/apps/k4dirstat.png
> %{_kde4_iconsdir}/hicolor/32x32/apps/k4dirstat.png
> %{_kde4_iconsdir}/hicolor/48x48/apps/k4dirstat.png
> 
> could be replaced by:
> %{_kde4_iconsdir}/hicolor/*/apps/k4dirstat.png
> 
> and
> %{_kde4_docdir}/HTML/en/k4dirstat/common
> %{_kde4_docdir}/HTML/en/k4dirstat/index.cache.bz2
> %{_kde4_docdir}/HTML/en/k4dirstat/index.docbook
> 
> could be replaced by:
> %{_kde4_docdir}/HTML/en/k4dirstat/*

I prefer not to use globbing to much, to avoid surprises when I update the spec file with a new upstream version. Is it required by the guidelines to minimize the size of %file?
Anyway, %{_kde4_docdir}/HTML/en/k4dirstat/ must be owned, and my previous %file did not own it. It is fixed now. As for the icons, I prefer to specify the files list.

> Use %global instead of %define as per
> https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

In this context, it is the same. But I agree that %global should be used blindly, unless the specific semantic of %define is wanted. Fixed.

> > Source0:        %{name}-%{preversion}.tar.gz
> You'll need to add a comment how to generate the git snapshot tarball. See
> https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control    

I have added such a comment. Strangely, the md5sum of the tarball has changed, but not its content. Maybe I used non-default compression level for gzip in March.

Note that the upstream Git repository has not evolved since April, and the used Git revision is the same.
Comment 7 Laurent Rineau 2010-06-08 18:01:52 EDT
(In reply to comment #6)
> SRPM URL: http://www.rineau.fr/k4dirstat-0-0.4.20100304gitec01dd42.fc13.src.rpm

Ooops! The right one is this one:

SRPM URL: http://www.rineau.fr/k4dirstat-0-0.6.20100304gitec01dd42.fc13.src.rpm
Comment 8 Kalev Lember 2010-06-15 21:25:56 EDT
(In reply to comment #6)
> * Tue Jun  8 2010  <Laurent.Rineau__fedora@normalesup.org> - 0-0.6.20100304gitec01dd42

Sorry for picking on the changelog format so much, but it's supposed to be
* date name <email> - version
You are missing the name.


> - Patch0 for F-13: link explicitly with zlib.

Did you submit the patch for upstream inclusion too?


> I prefer not to use globbing to much, to avoid surprises when I update the spec
> file with a new upstream version. Is it required by the guidelines to minimize
> the size of %file?

Certainly not, it's up to you to use globbing if you think it makes your life easier.


> Anyway, %{_kde4_docdir}/HTML/en/k4dirstat/ must be owned, and my previous %file
> did not own it.

{_kde4_appsdir}/k4dirstat/ isn't owned either.


> I have added such a comment. Strangely, the md5sum of the tarball has changed,
> but not its content. Maybe I used non-default compression level for gzip in
> March.

As long as there is a way to regenerate tarball's content, it's all good.
Comment 9 Kalev Lember 2010-06-15 21:54:05 EDT
How did you determine this is GPLv2? The source files say either:
> License:   GPL - See file COPYING for details.
or
> License:   LGPL - See file COPYING.LIB for details.

According to http://fedoraproject.org/wiki/Licensing , it should be GPL+ if the sources or the accompanying documentation doesn't specify a version.

I'd suggest to ask the upstream author for clarification and suggest him to use proper GPL/LGPL header in every source file, as explained in http://www.gnu.org/licenses/old-licenses/gpl-2.0.html#SEC4
Comment 10 Kalev Lember 2010-06-15 22:00:22 EDT
> Version:        0
Why 0? In http://grumpypenguin.org/index.php?/archives/3-KDirStat-coming-to-KDE4.html, the author comments "As you can see in the about dialog, it currently is set at version 2.7.0 /.../"

Also, in main.cpp there's a line:
static const char version[] = "2.7.0";
Comment 11 Kalev Lember 2010-08-04 06:49:17 EDT
Laurent, are you still interested in finishing this review?
Comment 12 Kalev Lember 2010-08-16 17:19:27 EDT
ping?
Comment 13 Kalev Lember 2010-08-28 09:58:18 EDT
Laurent, could you please respond?
Comment 14 Kalev Lember 2010-09-25 17:03:38 EDT
This review appears to be stalled. As per http://fedoraproject.org/wiki/Policy_for_stalled_package_reviews I am going to close this ticket in one week if the submitter doesn't respond.

The intention is to close the ticket so that the package can be submitted by someone else in a separate ticket.
Comment 15 Dmitrij S. Kryzhevich 2010-10-10 03:09:34 EDT
I could (and want to) open new review on k4dirstat, but I need sponsor (I try to get it in BR #640889).
Comment 16 Kalev Lember 2010-10-10 04:43:28 EDT
Sure, go ahead. I'm closing this ticket so you can submit a new review request yourself.
Comment 17 Laurent Rineau 2010-10-10 05:36:03 EDT
Thanks guys. I am sorry I could not give more. My daily life is too busy to get time for Fedora, for the moment.
Comment 18 Dmitrij S. Kryzhevich 2010-10-10 06:33:29 EDT
Opend here: https://bugzilla.redhat.com/show_bug.cgi?id=641690