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

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

Created attachment 117665 [details]
patch for GCC4 > 4.0.0-8

These C++ fixes are necessary to make it build for FC4 and newer.

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) :)

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.

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.

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

/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.

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

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).

> 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.

Added BuildRequires: gettext, new files on server.

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 ).

Well, Konqueror does (or did, at least) include such an entry. And a file 
manager can be a useful system administration tool.

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.

- 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.

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)

"OnlyShowIn=KDE" is a _must_ when "X-KDE-SubstituteUID=true" is used.

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?

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.

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 :)

Requesting epel6 branch, as requested in bug 1045881, thanks

Package Change Request
======================
Package Name: krusader
New Branches: el6
Owners: rnovacek kkofler

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.

Sorry, I should read the instructions more carefully next time.

What about epel7? ;)

Git done (by process-git-requests).

Requesting epel7 branch, as requested in bug 1224446, thanks

Package Change Request
======================
Package Name: krusader
New Branches: epel7
Owners: rnovacek

Git done (by process-git-requests).