Bug 165616 - Review Request: krusader - Advanced twin-panel (commander-style) file-manager for KDE
Review Request: krusader - Advanced twin-panel (commander-style) file-manager...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
David Lawrence
http://manta.univ.gda.pl/~mgarski/FE/
: Reopened
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-10 16:24 EDT by Marcin Garski
Modified: 2015-05-26 15:12 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-01-20 06:45:49 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑cvs+


Attachments (Terms of Use)
patch for GCC4 > 4.0.0-8 (1.28 KB, patch)
2005-08-11 18:31 EDT, Michael Schwendt
no flags Details | Diff
krusader.spec patch (1.74 KB, patch)
2005-08-12 17:07 EDT, Dawid Gajownik
no flags Details | Diff

  None (edit)
Description Marcin Garski 2005-08-10 16:24:37 EDT
Spec Name or Url: krusader.spec
SRPM Name or Url: krusader-1.60.0-1.src.rpm
Description: 
Krusader is an advanced twin-panel (commander-style) file-manager for KDE 3.x
(similar to Midnight or Total Commander) but with many extras.
It provides all the file-management features you could possibly want.
Plus: extensive archive handling, mounted filesystem support, FTP, advanced
search module, viewer/editor, directory synchronisation, file content
comparisons, powerful batch renaming and much much more.
It supports the following archive formats: tar, zip, bzip2, gzip, rar, ace,
arj and rpm and can handle other KIOSlaves such as smb:// or fish://
It is (almost) completely customizable, very user friendly, fast and looks
great on your desktop! :-)
Comment 1 Dawid Gajownik 2005-08-11 17:11:57 EDT
Hi!

There's a small problem - this package does not compile on my machine:

if g++ -DHAVE_CONFIG_H -I. -I. -I../.. -I/usr/include/kde
-I/usr/lib/qt-3.3/include -I/usr/X11R6/include   -DQT_THREAD_SUPPORT 
-D_REENTRANT -D_LARGEFILE64_SOURCE -DKDE_NO_COMPAT -DQT_NO_ASCII_CAST 
-Wnon-virtual-dtor -Wno-long-long -Wundef -ansi -D_XOPEN_SOURCE=500
-D_BSD_SOURCE -Wcast-align -Wconversion -Wchar-subscripts -Wall -W
-Wpointer-arith -Wwrite-strings -O2 -O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -m32 -march=i386 -mtune=pentium4 -fasynchronous-unwind-tables
-Wformat-security -Wmissing-format-attribute -fno-exceptions -fno-check-new
-fno-common  -MT kraddbookmarkdlg.o -MD -MP -MF ".deps/kraddbookmarkdlg.Tpo" -c
-o kraddbookmarkdlg.o kraddbookmarkdlg.cpp; \
then mv -f ".deps/kraddbookmarkdlg.Tpo" ".deps/kraddbookmarkdlg.Po"; else rm -f
".deps/kraddbookmarkdlg.Tpo"; exit 1; fi
../Panel/listpanel.h:152: error: ISO C++ forbids declaration of 'ListPanelFunc'
with no type
../Panel/listpanel.h:152: error: expected ';' before '*' token
../Panel/panelfunc.h: In member function 'ListPanelFunc*
ListPanelFunc::otherFunc()':
../Panel/panelfunc.h:85: error: 'class ListPanel' has no member named 'func'
../krslots.h: In member function 'void KRslots::syncPanels()':
../krslots.h:121: error: 'class ListPanel' has no member named 'func'

and at the end compilation fails. Have you tested your package on FC4? With
which compiler? I've got this one:

[y4kk0@X devel]$ rpm -q gcc-c++
gcc-c++-4.0.1-4.fc4
[y4kk0@X devel]$

My (partial) review:
- Summary should not end with dot. You should use rpmlint to be informed about
such a mistakes → http://fedoraproject.org/wiki/PackagingGuidelines#rpmlint
- please provide full URL to the Source0 file:
Source0:        http://prdownloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
- missing BuildRequires: automake
Have you run fedora-rmdevelrpms program?
http://fedoraproject.org/wiki/PackagingGuidelines#BuildRequires
- Since FC3, Fedora has been using gamin instead of fam. Maybe it would be
better to change fam-devel to gamin-devel.
- %setup -q -n %{name}-%{version} ← you don't have to manually specify "-n"
option. It's not a bug but IMHO it's better to have less commands in spec file.
 easy to read = less bugs ;]
- install *.desktop files using desktop-file-install:
http://fedoraproject.org/wiki/Extras/FedoraDesktopEntryGuidelines

I wasn't able to test it but this should be okey:
desktop-file-install --vendor fedora \
  --dir $RPM_BUILD_ROOT%{_datadir}/applications \
  --add-category X-Fedora \
  --delete-original \
  $RPM_BUILD_ROOT%{_datadir}/appl*/*/*.desktop

I think that you can also drop "X-Red-Hat-Base" from patch file.

- "%{_mandir}/man1/krusader.1.gz" change to "%{_mandir}/man1/krusader.1*" on
some systems man pages are not compressed or compressed with bzip2.

On fedora.pl forum people had problems with compiling krusader on 64-bit
systems. You may need to use this workaround →
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=161343#c3

I wasn't able to check anything else :/

Oh, and the last thing. Would you be willing to change the order of the lines in
specfile? It's not need but please compare your spec with the template from
fedora-rpmdevtools package (/usr/share/fedora/spectemplate-minimal.spec). IMHO
it's better to have "Summary:" field after "Name:", "Version:", "Release:" tags.
You may of course not agree with me :D

Thanks,
Dawid
Comment 2 Michael Schwendt 2005-08-11 18:29:51 EDT
There's more incomplete, starting with:

> checking for Qt... configure: error: Qt (>= Qt 3.1 (20021021)) (headers
> and libraries) not found. Please check your installation!
> For more details about this problem, look at the end of config.log.
> error: Bad exit status from /home/misc5/tmp/rpm/tmp/rpm-tmp.63149 (%build)

%build section does not set $QTDIR. It should source /etc/profile.d/qt.sh,
take a look a package "konversation", for instance, in Extras CVS for
how to do it.

With QTDIR set, it fails nevertheless due to missing "Buildrequires: autoconf".
Since the spec file does not run autoconf, most likely the tarball is broken. 
Maybe "Buildrequires: automake" is needed depending on what files the tarball 
tries to regenerate.

> cd . && make -f admin/Makefile.common configure
> make[1]: Entering directory `/home/qa/tmp/rpm/BUILD/krusader-1.60.0'
> /home/qa/tmp/rpm/BUILD/krusader-1.60.0/admin/missing: line 46:
> automake-1.9: command not found
> WARNING: `automake-1.9' is missing on your system.  You should only need it if
>          you modified `Makefile.am', `acinclude.m4' or `configure.in'.
>          You might want to install the `Automake' and `Perl' packages.
>          Grab them from any GNU archive site.
> ./admin/cvs.sh: line 11: autoconf: command not found

Comment 3 Michael Schwendt 2005-08-11 18:31:20 EDT
Created attachment 117665 [details]
patch for GCC4 > 4.0.0-8

These C++ fixes are necessary to make it build for FC4 and newer.
Comment 4 Marcin Garski 2005-08-12 11:33:48 EDT
Spec Name or Url: http://manta.univ.gda.pl/~mgarski/FE/krusader.spec
SRPM Name or Url: http://manta.univ.gda.pl/~mgarski/FE/krusader-1.60.0-2.src.rpm

Improved version. I hope without any errors (especially compile errors) :)
Comment 5 Marcin Garski 2005-08-12 12:00:50 EDT
I've forgot one thing. Dawid I'm still using FC2 with gcc-c++-3.3.3-7, in about
2-4 weeks I'll probably move to FC4.
Comment 6 Dawid Gajownik 2005-08-12 17:07:07 EDT
Created attachment 117691 [details]
krusader.spec patch

Here's my turn :)
- use "--disable-rpath" option
Without it I had this errors:

+ /usr/lib/rpm/check-rpaths /usr/lib/rpm/check-buildroot
ERROR: file '/usr/lib/kde3/kio_iso.so' contains a standard rpath '/usr/lib' in
[/usr/lib:/usr/lib/qt-3.3/lib:/
usr/X11R6/lib]
ERROR: file '/usr/lib/kde3/kio_iso.so' contains a standard rpath
'/usr/X11R6/lib' in [/usr/lib:/usr/lib/qt-3.3
/lib:/usr/X11R6/lib]
ERROR: file '/usr/lib/kde3/kio_krarc.so' contains a standard rpath '/usr/lib'
in [/usr/lib:/usr/lib/qt-3.3/lib
:/usr/X11R6/lib]
ERROR: file '/usr/lib/kde3/kio_krarc.so' contains a standard rpath
'/usr/X11R6/lib' in [/usr/lib:/usr/lib/qt-3
.3/lib:/usr/X11R6/lib]
błąd: Błędny status wyjścia z /var/tmp/rpm-tmp.24856 (%install)

- remove *.la files
- your workaround will not work on machines with different %{_tmppath} variable

- in the attached patch there's a fix to the second rpmlint error:

[rpm-build@X i386]$ rpmlint krusader-1.60.0-2.i386.rpm
W: krusader dangling-symlink /usr/share/doc/HTML/en/krusader/common
/usr/share/doc/HTML/en/common
W: krusader symlink-should-be-relative /usr/share/doc/HTML/en/krusader/common
/usr/share/doc/HTML/en/common
[rpm-build@X i386]$

- you should own %{_datadir}/apps/krusader directory
(%{_datadir}/apps/krusader/ will not work)
- don't own directories which belong to other packages:

%{_datadir}/doc/*
%{_datadir}/icons/*

is wrong.

I haven't spotted more problems. Well, I'm a newbie in Fedora Extras so someone
else would need to review your package, too.
Comment 7 Marcin Garski 2005-08-12 19:18:18 EDT
New package ready:
Spec Name or Url: http://manta.univ.gda.pl/~mgarski/FE/krusader.spec
SRPM Name or Url: http://manta.univ.gda.pl/~mgarski/FE/krusader-1.60.0-2.src.rpm
(I didn't changed release number, but files on server are the new one).

Check this time and report problems :)

One thing maybe I'm wrong and I'll have to read one more time about packaging:
> - you should own %{_datadir}/apps/krusader directory
> (%{_datadir}/apps/krusader/ will not work)

directory with / or without / should work, I have spec with
%{_includedir}/mac/
and if I remove rpm %{_includedir}/mac/ are deleted also, but maybe I'm wrong
and I'm talking about something else?

My rpm is rpm-4.3.1-0.4.1
Comment 8 Michael Schwendt 2005-08-12 23:18:12 EDT
/bar

is the same as

%dir /bar
/bar/*

Both include the directory "bar" and its contents recursively.
So either

%{_datadir}/apps/krusader

or

%{_datadir}/apps/krusader/          (<-- more readable!)

or

%dir %{_datadir}/apps/krusader
%{_datadir}/apps/krusader/*

would work. The trailing '/' on directory entries improves readability.
Else you could think %_datadir/apps/krusader is a data file, not a directory.
Comment 9 Dawid Gajownik 2005-08-13 04:01:52 EDT
Uuups, silly me. Sorry for the confusion.

I have one more question: in the spec template from the old fedora-rpmdevtools
[1] there was such a comment:

# Note: the find_lang macro requires gettext
%find_lang %{name}

Is this still true? I don't have gettext package in my system but
krusader.src.rpm recompiles without a problem.

Marcin, could you revert the change which concerns "%{_datadir}/apps/krusader/"
line?

[1] http://80.55.221.90/~gajownik/linux/OLD-spec.spec
Comment 10 Marcin Garski 2005-08-13 05:38:50 EDT
As I see:
%find_lang      /usr/lib/rpm/find-lang.sh %{buildroot}

and in find-lang.sh only 'sed' and 'find' are used.

In usual place (http://manta.univ.gda.pl/~mgarski/FE/) you can find new package
with "%{_datadir}/apps/krusader/" line (still release number is 2).
Comment 11 Michael Schwendt 2005-08-13 06:41:58 EDT
> checking for msgfmt... msgfmt
> checking for gmsgfmt... msgfmt
> found msgfmt program is not GNU msgfmt; ignore it
> checking for xgettext... :

Whether "BuildRequires: gettext" may be needed depends on whether it must be
present in order to enable/update/build translated message object files. The
configure script checks for it, and sometimes it can happen that a package
builds without translations when gettext is not installed.
Comment 12 Marcin Garski 2005-08-13 08:08:15 EDT
Added BuildRequires: gettext, new files on server.
Comment 13 Michael Schwendt 2005-08-22 17:23:19 EDT
You can now request a Fedora Extras CVS account.

[...]

One thing is left which you can change after CVS import: The second menu entry
"Krusader - root-mode" must be removed. It is irresponsible to include such an
entry by default and effectively encourage users to run krusader as superuser.
No other file manager includes such a second menu entry either.

[...]

At first-time start, this warning looked somewhat odd:

  rename: no compatible rename-programs found. Multiple rename is disabled.

While it may be that this refers to a KDE-specific program, it is very confusing.

[...]

In menu "Konfigurator > Basic Operation", tab Temp Directory, there is a default
temp file name which hopefully is used in a secure way ( /tmp/krusader.tmp ).
Comment 14 Kevin Kofler 2005-08-24 18:37:44 EDT
Well, Konqueror does (or did, at least) include such an entry. And a file 
manager can be a useful system administration tool.
Comment 15 Michael Schwendt 2005-08-25 06:07:39 EDT
That doesn't imply that it is a good default, as it advocates running
a graphical file manager as super user.

Konqueror's "File Manager - Super User Mode" menu entry is in the
"System Tools" folder, not in the "Accessories" folder.

Further, it is "OnlyShowIn=KDE;", because KDE is a strict requirement
for "X-KDE-SubstituteUID=true" to do its job.

So, actually Krusader's root-mode menu entry still is a packaging bug,
because at present it is shown also in GNOME, where an ordinary user
can start it without being prompted by kdesu.

I don't pull back the approval, but something must be done about this.
Comment 16 Marcin Garski 2005-08-25 09:54:28 EDT
- Dawid if kio_krarc.la file is removed, Krusader can't enter into some archives
like .rpm, .zip (it display error window).

- Should I remove "Krusader - root-mode" or move it to "System Tools" folder and
add "OnlyShowIn=KDE;"?

- As I see temp file isn't used in secure way, but I need to check it one more
time, because I could be wrong.

- "rename: no compatible rename-programs found. Multiple rename is disabled."
warning refers to KRename (http://www.krename.net/), apps NOT included in KDE.
Probably I'll add it to FE.
Comment 17 Michael Schwendt 2005-08-25 10:03:43 EDT
KDE applications strictly require libtool archives because KDE's libltdl
is broken/misconfigured and doesn't support *.so DSOs:
https://bugs.kde.org/show_bug.cgi?id=93359

If anybody knows a way how to get a sign of life from the KDE developers
on that issue, would be quite interesting. (Opened: 2004-11-16)
Comment 18 Michael Schwendt 2005-08-25 10:05:03 EDT
"OnlyShowIn=KDE" is a _must_ when "X-KDE-SubstituteUID=true" is used.
Comment 19 Marcin Garski 2005-08-25 13:38:56 EDT
Changes in new release 1.60.0-3
- Include .la files, so entering into archives works
- Include actions_tutorial.txt
- Fix krusader_root-mode.desktop file to show only in KDE and under System
  category
- Fix compile warnings

If there aren't any objections I would like to commit changes to CVS and built
packages in FC3 & 4 branches?
Comment 20 Michael Schwendt 2005-08-25 14:02:46 EDT
Sure, that was the point of the earlier approval, so that moving around
src.rpms is no longer necessary. Once built successfully, this ticket
can be closed.
Comment 21 Marcin Garski 2005-08-25 14:13:00 EDT
Note about temp dir.

I've read over the source code and:
- Krusader create temp dir (default /tmp/krusader.tmp)
- Chmod it 0777
- Get UID
- Create directory (name of new dir == UID) under temp dir
- Chmod it as 0700

So IMHO it is created in secure way :)
Comment 22 Radek Novacek 2014-01-02 07:18:45 EST
Requesting epel6 branch, as requested in bug 1045881, thanks

Package Change Request
======================
Package Name: krusader
New Branches: el6
Owners: rnovacek kkofler
Comment 23 Michael Schwendt 2014-01-20 06:45:49 EST
Radek,

please don't reopen old tickets for an SCM change request, but follow the instructions in the Wiki and only set the fedora-cvs flag.
Comment 24 Radek Novacek 2014-01-20 07:00:46 EST
Sorry, I should read the instructions more carefully next time.
Comment 25 Christopher Meng 2014-01-21 01:35:27 EST
What about epel7? ;)
Comment 26 Jon Ciesla 2014-01-21 08:45:27 EST
Git done (by process-git-requests).
Comment 27 Radek Novacek 2015-05-26 07:46:36 EDT
Requesting epel7 branch, as requested in bug 1224446, thanks

Package Change Request
======================
Package Name: krusader
New Branches: epel7
Owners: rnovacek
Comment 28 Jon Ciesla 2015-05-26 15:12:40 EDT
Git done (by process-git-requests).

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