Bug 386531 - Review Request: kuftp - Graphical FTP Client for KDE
Summary: Review Request: kuftp - Graphical FTP Client for KDE
Keywords:
Status: CLOSED DUPLICATE of bug 433814
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: 417251
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2007-11-16 09:41 UTC by Marcus Taschenberger
Modified: 2013-01-13 12:57 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-02-19 18:10:50 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Marcus Taschenberger 2007-11-16 09:41:33 UTC
Spec URL: http://yasha.alfahosting.org/kuftp.spec
SRPM URL: http://yasha.alfahosting.org/kuftp-0.9.0-1.fc8.src.rpm
Description: KuFtp is a graphical FTP client for the K Desktop Environment. Most notable features is Tab Sessions like Konqueror or Firefox, that is,you can have multiple simultaneous FTP session in separate tabs.


This is my first rpm package. I hope the package is Ok

I'm searching a sponsor

Comment 1 Kevin Kofler 2007-11-16 18:33:17 UTC
Thanks for your submission.

Not a formal review as I don't have sponsorship privileges (and I haven't run 
this through rpmlint and the long review checklist), but here's some things I 
noticed:
* %{_libdir}/libkuftpbookmarks.a -> This one should go away, for several 
reasons:
1. it's a library in what's not a -devel or -static package,
2. it's a static library, so it'd have to be treated specially (-static 
package, ...) and
3. I assume nothing other than kuftp itself uses it, and that links it 
statically, so it's just a convenience library used during build, not something 
worth installing.
Th best way to get rid of unwanted files is to rm -f them at the end 
of %install.
* "License: GPL" is too vague, you have to specify what version, 
e.g. "GPLv2", "GPLv2+" (i.e. >= 2), "GPLv3", "GPLv3+" (note that anything not 
including v2 is NOT a valid license for a KDE package because Qt is still GPL 
v2 only). (I know there's still lots of packages with "License: GPL", but these 
need to have their License tags fixed too.)
* Source0 should be a full URL, there's a section in the guidelines for SF 
URLs.
* Is kdebase-devel really required to build this? Or just kdelibs-devel?
* Please BuildRequire kdebase3-devel or kdelibs3-devel instead of kdebase-devel 
or kdelibs-devel, because Rawhide is going to move to KDE 4 as the default KDE 
soon. The current kdelibs and kdebase packages Provide the kde*3(-devel) names.
* Minor nitpick: any reason you're listing "??x??" and "???x???" rather than 
just "*x*" or even "*"? (That's just cosmetics though, not a requirement.)

Comment 2 Marcus Taschenberger 2007-11-16 19:52:25 UTC
Ok thanks
1,2 and 3 fixed. i hope it =)

License is GPLv2 i have fixed to and addes the copyright file to the %doc

Its kdelibs-devel. fixed to


New .spec and srpm files uploadet. 

Comment 3 Sebastian Vahl 2007-11-17 20:46:11 UTC
(In reply to comment #2)
> New .spec and srpm files uploadet. 

Please increase the %{release} everytime you make changes to your packet. So 
the reviewer could follow. And of course the url to the new packages then must 
be posted here.

Added FE-NEEDSPONSOR



Comment 4 Marcus Taschenberger 2007-11-17 22:22:42 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > New .spec and srpm files uploadet. 
> 
> Please increase the %{release} everytime you make changes to your packet. So 
> the reviewer could follow.
Ok i change it

>And of course the url to the new packages then must be posted here.
I delete the old package and new package have the same name. but of course when
i change the release number the srpm url change it to. 

the new srpm + .spec file can be found here

Spec URL: http://yasha.alfahosting.org/kuftp.spec
SRPM URL: http://yasha.alfahosting.org/kuftp-0.9.0-3.fc8.src.rpm

Comment 5 Rex Dieter 2007-11-20 17:45:04 UTC
2 suggestions:
1.  Group: Applications/Internet
2.  add --disable-static option to %configure
3.  use desktop-file-install
BuildRequires: desktop-file-utils

to %install, add
desktop-file-install \
 --dir=%{buildroot}%{_datadir}/applications/kde \ 
 --delete-original --vendor="" \
 --add-category=Network \
 %{buildroot}%{_datadir}/applnk/Internet/*.desktop
(add-category may or may not be needed depending if that .desktop Category is 
present already)
 
4.  Add icon scriptlets:

%post
touch --no-create %{_datadir}/icons/hicolor ||:
gtk-update-icon-cache -q %{_datadir}/icons/hicolor >& /dev/null ||:

%postun
touch --no-create %{_datadir}/icons/hicolor ||:
gtk-update-icon-cache -q %{_datadir}/icons/hicolor >& /dev/null ||:

5.  Can (probably) simplify BR's to just
BuildRequires: kdelibs3-devel zlib-devel

Comment 6 Rex Dieter 2007-11-20 17:48:32 UTC
and
7.
Summary:        Graphical FTP Client
(lose the "for KDE", users of any desktop environment can use it).

Comment 7 Rex Dieter 2007-11-20 17:48:49 UTC
and
6.  intentionally left blank.
:)

Comment 8 Marcus Taschenberger 2007-11-21 20:00:45 UTC
(In reply to comment #5)
> 5.  Can (probably) simplify BR's to just
> BuildRequires: kdelibs3-devel zlib-devel

I tested it on a new fedora8 installation. I added only rpmdevtools, gcc,
kdelibs3-devel and zlib-devel. Srpmrebuild for me not work, i have configure
error X headers not found.

All other errors fixed

Here the new files:
Spec URL: http://yasha.alfahosting.org/kuftp.spec
SRPM URL: http://yasha.alfahosting.org/kuftp-0.9.0-4.fc8.src.rpm


Comment 9 Rex Dieter 2007-11-21 20:10:22 UTC
On my f7 box:

$ rpm -qR $(rpm -q --whatprovides kdelibs3-devel) | grep devel
arts-devel
libkdnssd-devel
openssl-devel
qt-devel

$ rpm -qR qt-devel | grep devel
fontconfig-devel
freetype-devel
libICE-devel
libSM-devel
libX11-devel
libXcursor-devel
libXext-devel
libXft-devel
libXinerama-devel
libXrandr-devel
libXrender-devel
libXt-devel
libjpeg-devel
libmng-devel
libpng-devel
mesa-libGL-devel
mesa-libGLU-devel
xorg-x11-proto-devel

so you see, by
BuildRequires: kdelibs3-devel
you're getting all of those above (including qt-devel libXt-devel libXext-devel)


Comment 10 Marcus Taschenberger 2007-11-21 21:15:51 UTC
ok ok you win =))))

I have change it.

new files:
Spec URL: http://yasha.alfahosting.org/kuftp.spec
SRPM URL: http://yasha.alfahosting.org/kuftp-0.9.0-5.fc8.src.rpm

Comment 11 Mamoru TASAKA 2007-11-27 14:06:17 UTC
Does someone reviewing this?

Comment 12 Rex Dieter 2007-11-27 14:16:45 UTC
No one yet (formally, I can/will eventually when I have some spare moments), but
go ahead if you've got the itch.

Comment 13 Marcus Taschenberger 2007-11-30 15:41:30 UTC
Update to version 0.9.1 and added install and thanks file to %doc

SPEC URL: http://yasha.alfahosting.org/kuftp.spec
SRPM URL: http://yasha.alfahosting.org/kuftp-0.9.1-1.fc8.src.rpm

Comment 14 Christoph Wickert 2007-11-30 17:46:40 UTC
(In reply to comment #13)
> Update to version 0.9.1 and added install and thanks file to %doc

Remove the INSTALL file please. It only holds generic information about building
this package from source, so it's not needed in the RPM.


Some more issues I found:

License should be GPLv2+. COPYING says it's GPLv2, but if you take a look at the
files source code you will find the following:

 *   This program is free software; you can redistribute it and/or modify  *
 *   it under the terms of the GNU General Public License as published by  *
 *   the Free Software Foundation; either version 2 of the License, or     *
 *   (at your option) any later version.  

"any later version" means GPLv2+, GPLv2 would be "GPL version 2 only".


Group:          /Applications/Internet
should be
Group:          Applications/Internet
(without the leading slash)


The description line is WAY TO LONG, please insert line breaks after 79
characters, so the description will fit on a standard 80x25 terminal.


There are also some errors in the description: 
most notable features ARE ... (Plural).
missing whitespace between "is,you".
The whole second sentence sounds a like bad English to me, but that should
better be decided by a native speaker.


From your spec:
make DESTDIR=$RPM_BUILD_ROOT install
rm -f %{buildroot}%{_libdir}/libkuftpbookmarks.a

Please don't mix different macro styles: Ether use %{buildroot} OR
$RPM_BUILD_ROOT but not both, see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f3d77b27a5d29dfc1f5600ef3fc836f2e317badf


Remove the "--add-category=Network" statement from desktop-file-install, because
the group Network already is in the desktop file.


$ rpmlint Downloads/kuftp-0.9.1-1.fc8.src.rpm 
kuftp.src:63: W: macro-in-%changelog doc
kuftp.src:81: W: macro-in-%changelog doc

You need to escape macros in the changelog with a second %, so %doc becomes %%doc.


kuftp.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 24)

Please only use spaces OR tabs, but not both.


kuftp.src: E: description-line-too-long KuFtp is a graphical FTP client for the
K Desktop Environment. Most notable features is Tab Sessions like Konqueror or
Firefox, that is,you can have multiple simultaneous FTP session in separate tabs.

see above


kuftp.src: W: non-standard-group /Applications/Internet

see above


kuftp.src: W: strange-permission kuftp.spec 0755

the permissions of the spec in the SRPM should be 664 or 644, but the file
should no be executable.

Comment 15 Marcus Taschenberger 2007-11-30 18:57:06 UTC
(In reply to comment #14)

> There are also some errors in the description: 
> most notable features ARE ... (Plural).
> missing whitespace between "is,you".
> The whole second sentence sounds a like bad English to me, but that should
> better be decided by a native speaker.
Mhm sorry my english is not very good. I have the description from the authors
website. But i fixed the errors.... i hope =)

All other erros fixed to 

New files: 

SPEC URL: http://yasha.alfahosting.org/kuftp.spec
SRPM URL: http://yasha.alfahosting.org/kuftp-0.9.1-2.fc8.src.rpm

Comment 16 Mamoru TASAKA 2007-12-09 15:51:59 UTC
For 0.9.1-2:

* URL
  - IMO for sourceforge hosted project, it is prefer to use
    URL: http://kuftp.sourceforge.net.

* autotool call after configure
  - build.log says
-------------------------------------------------------------
   333  Good - your configure finished. Start make now
   334  
   335  + make -j2
   336   cd . && /bin/sh /builddir/build/BUILD/kuftp-0.9.1/admin/missing --run
automake-1.9 --gnu 
   337  /builddir/build/BUILD/kuftp-0.9.1/admin/missing: line 52: automake-1.9:
command not found
   338  WARNING: `automake-1.9' is missing on your system.  You should only need
it if
   339           you modified `Makefile.am', `acinclude.m4' or `configure.in'.
   340           You might want to install the `Automake' and `Perl' packages.
   341           Grab them from any GNU archive site.
--------------------------------------------------------------
    http://koji.fedoraproject.org/koji/getfile?taskID=283496&name=build.log

    Having autotools called automatically is not desired. Generally
    this means that some files have unwilling timestamps and can be fixed
    by "touch"ing some files.

* Directory ownership issue
  - For example
--------------------------------------------------------------
[tasaka1@localhost ~]$ rpm -qf /usr/share/apps/kuftp/kuftpui.rc 
kuftp-0.9.1-2.fc9
[tasaka1@localhost ~]$ rpm -qf /usr/share/apps/kuftp/           
file /usr/share/apps/kuftp is not owned by any package
--------------------------------------------------------------
    Here /usr/share/apps/kuftp is created by kuftp package to
    install kuftpui.rc under the directory, however kuftp does not
    own the directory, which is not right.

    The %files entry
---------------------------------------------------------------
%files
%{_datadir}/apps/kuftp/*
---------------------------------------------------------------
    contains any files/directories/etc under %_datadir/apps/kuftp,
    but does not contain the directory %_datadir/apps/kuftp itself,
    where
---------------------------------------------------------------
%files
%{_datadir}/apps/kuftp/
---------------------------------------------------------------
    contains the directory %_datadir/apps/kuftp _and_ all files/
    directories/etc under %_datadir/apps/kuftp.

* Absolute symlink
----------------------------------------------------------------
$ rpmlint kuftp
kuftp.i386: W: symlink-should-be-relative /usr/share/doc/HTML/en/kuftp/common
/usr/share/doc/HTML/en/common
----------------------------------------------------------------
  - We require that the symlinks must be relative (i.e. ../common),
    not absolute.

As this is SPONSORNEEDED ticket:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------


Comment 17 Rex Dieter 2007-12-09 16:58:59 UTC
The absent/dangling symlink shouldn't be a blocker here (imo), Re: bug #417251

Comment 18 Mamoru TASAKA 2007-12-18 13:35:26 UTC
ping?

Comment 19 Marcus Taschenberger 2007-12-22 21:45:24 UTC
pong! =)

Mhm sorry i get married for 5 days. and now it's goin to holiday. but i don't
forget my request and in the next year i am back.

Comment 20 Mamoru TASAKA 2008-01-20 15:12:44 UTC
ping again?

Comment 21 Mamoru TASAKA 2008-01-29 17:06:44 UTC
ping again?

Comment 22 Mamoru TASAKA 2008-02-11 14:39:48 UTC
I will close this bug as NOTABUG if no response is received from the
reporter within ONE WEEK.

Comment 23 Mamoru TASAKA 2008-02-19 18:10:33 UTC
Once closing.

If someone wants to import this package into Fedora, please file
a new review request and mark this bug a duplicate of the new bug.

Thank you!

Comment 24 Krzysztof Kurzawski 2008-02-21 16:00:42 UTC

*** This bug has been marked as a duplicate of 433814 ***


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