Bug 191473 - Review Request: kdiff3: Compare + merge 2 or 3 files or directories
Review Request: kdiff3: Compare + merge 2 or 3 files or directories
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-12 06:47 EDT by Neal Becker
Modified: 2014-09-15 08:00 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-28 10:36:40 EDT
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)
Patched spec file for kdiff3-0.9.88-5 (2.98 KB, text/plain)
2006-05-12 11:53 EDT, Laurent Rineau
no flags Details
add a conditional BR, allowing build in EPEL-5 (4.72 KB, application/octet-stream)
2008-06-05 16:43 EDT, manuel wolfshant
no flags Details
removing two lines from doc/nl/index.docbook allows build in C4 (5.00 KB, text/x-rpm-spec)
2008-06-19 12:03 EDT, manuel wolfshant
no flags Details

  None (edit)
Description Neal Becker 2006-05-12 06:47:15 EDT
Spec URL: http://nbecker.dyndns.org:8080/kdiff3.spec
SRPM URL: http://nbecker.dyndns.org:8080/kdiff3-0.9.88-1.src.rpm
Description: KDiff3 is a program that
- compares and merges two or three input files or directories,
- shows the differences line by line and character by character (!),
- provides an automatic merge-facility and
- an integrated editor for comfortable solving of merge-conflicts
- has support for KDE-KIO (ftp, sftp, http, fish, smb)
- and has an intuitive graphical user interface.
Comment 1 Laurent Rineau 2006-05-12 08:40:04 EDT
Not a formal review, but I have several remarks:

* MUST fix: Source0 URL should be 
http://dl.sourceforge.net/sourceforge/kdiff3/%{name}-%{version}.tar.gz

* MUST fix rpmlint warnings and errors:

W: kdiff3 summary-not-capitalized compare + merge 2 or 3 files or directories

Easy to fix.

E: kdiff3 standard-dir-owned-by-package /usr/share/icons
E: kdiff3 standard-dir-owned-by-package /usr/share/man
E: kdiff3 standard-dir-owned-by-package /usr/share/man/man1

See http://fedoraproject.org/wiki/Packaging/Guidelines, section "File and 
Directory Ownership".

W: kdiff3 
dangling-symlink /usr/share/doc/HTML/da/kdiff3/common /usr/share/doc/HTML/da/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/de/kdiff3/common /usr/share/doc/HTML/de/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/en/kdiff3/common /usr/share/doc/HTML/en/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/et/kdiff3/common /usr/share/doc/HTML/et/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/fr/kdiff3/common /usr/share/doc/HTML/fr/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/it/kdiff3/common /usr/share/doc/HTML/it/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/pt/kdiff3/common /usr/share/doc/HTML/pt/common
W: kdiff3 
dangling-symlink /usr/share/doc/HTML/sv/kdiff3/common /usr/share/doc/HTML/sv/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/da/kdiff3/common /usr/share/doc/HTML/da/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/de/kdiff3/common /usr/share/doc/HTML/de/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/en/kdiff3/common /usr/share/doc/HTML/en/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/et/kdiff3/common /usr/share/doc/HTML/et/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/fr/kdiff3/common /usr/share/doc/HTML/fr/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/it/kdiff3/common /usr/share/doc/HTML/it/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/pt/kdiff3/common /usr/share/doc/HTML/pt/common
W: kdiff3 
symlink-should-be-relative /usr/share/doc/HTML/sv/kdiff3/common /usr/share/doc/HTML/sv/common

Seems to be a usual problem with KDE packages. I do not know what should be 
done.


Kdiff3 version 0.9.89 has been release (2006-04-09). Perhaps you should update 
your tarball.
Comment 2 Neal Becker 2006-05-12 08:54:24 EDT
http://kdiff3.sourceforge.net/, says:
Current version: 0.9.89 (2006-04-09) 
 (A serious regression when accessing KDE-KIO was reported. If you need this, 
please use 0.9.88 instead.)
So, I am staying with 0.9.88

Please see http://nbecker.dyndns.org:8080/kdiff3-0.9.88-2.src.rpm

I have fixed the problems you cite, except for 'symlink-should-be-relative'.  
Anyone know how to fix this, or is it important?
Comment 3 Rex Dieter 2006-05-12 09:01:50 EDT
The symlink thing is *that* important, but it's fixable.  I use this snippet in
my KDE rpms: (you already use something close to this)
in %%install:

## File lists
# locale's
%find_lang %{name} || touch %{name}.lang
# HTML (1.0)
HTML_DIR=$(kde-config --expandvars --install html)
if [ -d $RPM_BUILD_ROOT$HTML_DIR ]; then
for lang_dir in $RPM_BUILD_ROOT$HTML_DIR/* ; do
  if [ -d $lang_dir ]; then
    lang=$(basename $lang_dir)
    echo "%lang($lang) $HTML_DIR/$lang/*" >> %{name}.lang
    # replace absolute symlinks with relative ones
    pushd $lang_dir
      for i in *; do
        [ -d $i -a -L $i/common ] && rm -f $i/common && ln -sf ../common $i/common
      done
    popd
  fi
done
fi


Comment 4 Laurent Rineau 2006-05-12 09:07:34 EDT
Rex, what about the "dandling symlink" stuff? People on #fedora-extras seem to 
agree that it can be ignored, while not perfect.
Comment 5 Rex Dieter 2006-05-12 09:11:03 EDT
Dangling symlink = symlink to a file/dir not in *this* pkg.  In the case of kde
apps, %{_docdir}/HTML can be expected to contain lots of them.  It can be ignored.

The snippet I posted will take care of the "symlink-should-be-relative" warnings
only.
Comment 6 Rex Dieter 2006-05-12 09:13:10 EDT
I noticed my comment #3 was mis-typed.  I meant to say:
The symlink thing is not *that* important.
                     ^^^
(forgot the not)
Comment 7 Neal Becker 2006-05-12 09:15:49 EDT
Symlink thing is fixed.  Thanks Rex.
http://nbecker.dyndns.org:8080/kdiff3-0.9.88-3.src.rpm
Comment 8 Laurent Rineau 2006-05-12 09:20:36 EDT
(In reply to comment #5)
> Dangling symlink = symlink to a file/dir not in *this* pkg.

I understood that. Packages kde-i18n-$LANG provide the linked directories.

> In the case of kde
> apps, %{_docdir}/HTML can be expected to contain lots of them.
> It can be ignored.

I agree. And #fedora-extras people too.
Comment 9 Laurent Rineau 2006-05-12 09:24:25 EDT
Your %changelog is strange. The release number is already written after you 
email address.

Proposed patch:

*** /tmp/kdiff3.spec	2006-05-12 15:30:53.000000000 +0200
--- /tmp/kdiff3-0.9.88-3.spec	2006-05-12 15:30:53.000000000 +0200
***************
*** 95,102 ****
--- 95,106 ----
  %changelog
  * Fri May 12 2006 Neal Becker <ndbecker2@gmail.com> - 0.9.88-3
  - Fix symlinks (from Rex Dieter)
+ - release 3
  
  * Fri May 12 2006 Neal Becker <ndbecker2@gmail.com> - 0.9.88-2
+ - release 2
+ 
+ * Fri May 12 2006 Neal Becker <ndbecker2@gmail.com> - 0.9.88-1
  - Fix source0
  - Fix E: kdiff3 standard-dir-owned-by-package /usr/share/icons
    E: kdiff3 standard-dir-owned-by-package /usr/share/man
Comment 10 Laurent Rineau 2006-05-12 09:38:44 EDT
(In reply to comment #7)
> Symlink thing is fixed.  Thanks Rex.
> http://nbecker.dyndns.org:8080/kdiff3-0.9.88-3.src.rpm

Whouah, fast fix!

I notice some owned directories that should not be, IMHO:
  /usr/lib64/kde3
  /usr/share/applications
  /usr/share/applnk
  /usr/share/applnk/Development
  /usr/share/apps
  /usr/share/services

They are all owned by kdelibs, but /usr/share/applnk/Development. As a matter 
of fact, /usr/share/applnk/Development/ does not exists on my FC-5. :-\

Rex, you are more experienced than be, i think, can you confirm that kdiff3 
should not owned these directories?
Comment 11 Laurent Rineau 2006-05-12 10:13:42 EDT
Yet another comment: you should see 
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
Comment 12 Dennis Gilmore 2006-05-12 10:15:00 EDT
indead  the files section is wrong  you need to own the files inside those 
directories.  additionally  you need to use desktop-file-utils to install 
your .desktop files 
the /usr/share/applnk tree should not exist once  you install the .desktop 
file

you also need a %post and %postun section to deal with the icons and to run 
ldconfig something like 

%post
/sbin/ldconfig
touch --no-create %{_datadir}/icons/hicolor || :

%postun
/sbin/ldconfig
touch --no-create %{_datadir}/icons/hicolor || :

Comment 13 Rex Dieter 2006-05-12 10:30:43 EDT
kdiff doesn't appear to include any shared libs, so /sbin/ldconfig isn't
necessary  in %%post/%%postun.
Comment 14 Laurent Rineau 2006-05-12 10:38:01 EDT
What about %{_libdir}/kde3/libkdiff3part.so ?

About the GTK+ icon cache, kdiff3 will be shown in Gnome menus. %post and 
%postun should call gtk-update-icon-cache, no?
Comment 15 Neal Becker 2006-05-12 11:01:43 EDT
Fixed /usr/share/applnk

Added %post, %postun

http://nbecker.dyndns.org:8080/kdiff3-0.9.88-4.src.rpm
Comment 16 Rex Dieter 2006-05-12 11:03:09 EDT
libkdiff3part is a runtime loadable KPart, not a shared library (ie, it's not in
%{_libdir})

Re: GTK+ icon cache: probably, though some would argue that kde apps shouldn't
have to mess with gtk-only crud like this.  (:  See bug #170335
Comment 17 Dennis Gilmore 2006-05-12 11:16:13 EDT
the package still owns directories  it shouldn't 

Please post a link to a spec file also
Comment 18 Rex Dieter 2006-05-12 11:17:28 EDT
specfile link is in the initial comment.
Comment 19 Dennis Gilmore 2006-05-12 11:43:56 EDT
dsirectories  you shouldn't own are 

/usr/share/apps
/usr/lib64/kde3
/usr/share/services

there is also no need to rm the applnk dir as rpm will not include it when it 
is empty.  which  happens  when you install the .desktop file

you also  do not need to requires or buildrequires kdelibs kdelibs-devel  
kdebase and kdebase-devel pull them in 
Comment 20 Laurent Rineau 2006-05-12 11:53:35 EDT
Created attachment 128943 [details]
Patched spec file for kdiff3-0.9.88-5

I attach a patched spec file.
I have corrected several items:
  - no % in %changelog: use %% instead,
  - fixed the %files section,
  - removed the Requires: tag
Comment 21 Neal Becker 2006-05-12 12:57:22 EDT
patch merged
rm applnk rm'd
buildrequires fixed

http://nbecker.dyndns.org:8080/kdiff3-0.9.88-5.src.rpm
Comment 22 Laurent Rineau 2006-05-12 14:14:38 EDT
Well, this package would need now a formal review.

Neal, in the next release, feel free to remove my name/email from the spec 
file. You can merge the two item 0.9.88-5 in %changelog. My editor (GNU/Emacs) 
automatically create the changelog entry.

I cannot find you in the owners.list. Perhaps do you need sponsorship, too.
Comment 23 Laurent Rineau 2006-05-16 05:24:27 EDT
Since today I can do official reviews. I would be pleased to do my first 
officiel review with kdiff3.

However, I do think that you have no sponsor. Can you confirm, Neal? I cannot 
sponsor someone, I can just do review.

Please read again http://fedoraproject.org/wiki/Extras/Contributors and search 
for the word sponsor. If you have no sponsor, and it seems not, you have to 
add FE-NEEDSPONSOR in the "blocks" list of all your review requests. You 
should have tell that in the description of your review (I made the same 
mistake with my first review request).
Comment 24 Neal Becker 2006-05-16 06:25:20 EDT
Yes, I need a sponsor.

I am also waiting for legal clearance from my employer (I initiated that 
yesterday).
Comment 25 Laurent Rineau 2006-05-16 08:36:53 EDT
%{_datadir}/doc/qt4-devel-4.1.2/ is not owned by any package, actually.

But qt4-doc fills it by creating %{_datadir}/doc/qt4-devel-4.1.2/html/. Maybe 
something is wrong, actually. But I think that the symlink %{qtdir}/doc should 
be in -doc, because the directory pointed by it (whatever it is) is filled 
only by -doc.
Comment 26 Laurent Rineau 2006-05-16 08:52:46 EDT
...! Please ignore comment #25. Obviously it is not for this bug.

Where is the cancel button?! :-(
Comment 27 Jose Pedro Oliveira 2006-05-17 22:39:25 EDT
BTW version 0.9.90 is out.
Comment 28 Dennis Gilmore 2006-08-17 15:13:55 EDT
Neal ping on status of your employer ok
Comment 29 Neal Becker 2006-08-21 07:10:55 EDT
My employer legal dept has OK'd this.
Comment 30 Laurent Rineau 2006-08-21 07:23:49 EDT
Well, ASSIGNED is not really the correct status of this package. But i do not 
see what could be best, now that is can no longer be NEW. Maybe VERIFIED.

Do you still need a sponsor?
Comment 31 Neal Becker 2006-08-21 07:27:07 EDT
Yes, I have several submissions waiting on a sponsor.  
Comment 32 Laurent Rineau 2006-08-21 07:38:31 EDT
Neal, please try to answer to 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=191492#c3 where Hans 
offer you an opporunity to get sponsored. You silence could interpreted badly.

I would like to review the current request (kdiff3), but I cannot give 
sponsorship. Your future sponsor has priority.
Comment 34 Hans de Goede 2006-08-27 04:21:56 EDT
As promised a formal review, one the must fixes are fixed we can do tihs for one
or two of your other packages, once I'm convinced that you've got the hang of
things I'll sponsor you.

MUST:
=====
O rpmlint output is:
W: kdiff3 mixed-use-of-spaces-and-tabs
W: kdiff3 incoherent-version-in-changelog 0.9.88-5 0.9.90-5
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/de/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/fr/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/nl/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/en/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/pt/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/it/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/da/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/es/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/sv/kdiff3/common ../common
W: kdiff3 dangling-relative-symlink /usr/share/doc/HTML/et/kdiff3/common ../common
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok (but license file not included!)
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
* BR: ok
* Locales handled as required
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files(except for the plugin).
* .desktop file as required, but not properly installed, see below


MUST Fix
========
* The following rpmlint output:
W: kdiff3 mixed-use-of-spaces-and-tabs
W: kdiff3 incoherent-version-in-changelog 0.9.88-5 0.9.90-5
* The desktop-file-install command is missing "--add-category X-Fedora" from
  its argument list
* The %post / %postun scriptlets are not updating the gtk-icon-cache, causing
  the icon to not appear in the gnome-panel menu, please use the full scriptlets
  for this as given here:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda
* Remove this (already commented) bogus line from %files: "#%{_datadir}/locale"
* Replace the empty %doc with:
  %doc AUTHORS COPYING ChangeLog README TODO
  and move the line to directly below the %defattr line. Also remove the
  empty line between %{_datadir}/services/* and %{_mandir}/man1/kdiff3*
  a manfile is treated as a normal file.
Comment 35 Neal Becker 2006-08-27 09:20:57 EDT
I believe all of the above are fixed.  Please try:

http://nbecker.dyndns.org:8080/RPM/kdiff3-0.9.90-6.src.rpm
Comment 36 Hans de Goede 2006-08-27 15:13:03 EDT
Almost perfect, but you're not using the gtk icon cache scriptlets as provided
on the wiki, your current setup will cause errors to scroll on the console
(although no further harm) on 100% kde setups (iow no gtk installed).

Please use the scriptlets from the wiki where ever possible. If you disagree
with a scriptlet discuss this on f-e-l. These scriplets are to be concidered
best of breed and should always be used where possible.
Comment 37 Neal Becker 2006-08-27 15:55:07 EDT
Did I miss something?  I added the gtk-update-icon-cache as shown on the wiki 
to %post and %postun.  Did you mean something else?

%post
touch --no-create %{_datadir}/icons/hicolor || :
%{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :

%postun
touch --no-create %{_datadir}/icons/hicolor || :
%{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
Comment 38 Dennis Gilmore 2006-08-27 16:04:09 EDT
the gtk cruft is not needed for kde packages  
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28Packaging%29#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

states "For KDE, just 'touch'ing the top-level icon directory is enough."

there is no need to discusss this it is perfectly acceptable to not have the gtk
stuff in kde packages. 

gtk-update-icon-cache is not part of the freedesktop.org specs  and really
doesn't belong in any spec see bug #170335 for more info
Comment 39 Hans de Goede 2006-08-27 16:10:23 EDT
(In reply to comment #37)
> Did I miss something?  I added the gtk-update-icon-cache as shown on the wiki 
> to %post and %postun.  Did you mean something else?
My bad, it looks like the adviced scriptlets have changed. Thus the package is
fine as is. Once I sponsor you I'll approve it and you can import it.

Please select a second package for me to review and to get an idea of how
familiar you are with FE's guidelines (this one went smotth, but had a bit of a
head start).

(In reply to comment #38)
> the gtk cruft is not needed for kde packages  
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28Packaging%29#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda
> 
> states "For KDE, just 'touch'ing the top-level icon directory is enough."
> 
> there is no need to discusss this it is perfectly acceptable to not have the gtk
> stuff in kde packages. 
> 

For kdebase maybe, but for KDE packages which also are show in the gnome
applications menu it must still be there. since kdiff3 is shown in the gnome
applications menu, the update icon cache is needed.
Comment 40 Hans de Goede 2006-09-28 10:36:40 EDT
This package has been imported and build, so I see no reason for leaving this
ticker open, closing.
Comment 41 manuel wolfshant 2008-06-05 16:40:54 EDT
Neal, could you please consider branching this package for EPEL? I've tweaked a
bit the spec available in rawhide and it builds and runs without any problems in
Centos-5.
Comment 42 manuel wolfshant 2008-06-05 16:43:54 EDT
Created attachment 308486 [details]
add a conditional BR, allowing build in EPEL-5
Comment 43 Neal Becker 2008-06-05 19:33:25 EDT
Package Change Request
======================
Package Name:
[New Branches: EL-5]
[Updated Fedora Owners: ] 
[Updated Fedora CC: ] 
[Updated EPEL Owners: ] 
[Updated EPEL CC: ] 
[Updated Description: ] 
[Updated Cvsextras Commits: ] 
[add any required explanatory text here] 
Comment 44 Kevin Fenzi 2008-06-06 11:35:12 EDT
cvs done.
Comment 45 manuel wolfshant 2008-06-19 12:01:29 EDT
Quick and dirty patched spec which allows build for EL-4. Works for me in C4.6/i386.

Explanation: build fails due to doxygen failing to understand "something" in
doc/nl/docindex. I've just removed the offending two lines. I apologize towards
.nl users and I would be happy to see a proper fix. I simply was in need of a
functional C4 version, I have no idea about proper doxygen usage and this hack
looked cleaner than removing the whole translation (in this language).
Comment 46 manuel wolfshant 2008-06-19 12:03:17 EDT
Created attachment 309861 [details]
removing two lines from doc/nl/index.docbook allows build in C4
Comment 47 Neal Becker 2014-09-12 18:23:08 EDT
Package Change Request
======================
Package Name: kdiff3
New Branches: epel7
Owners: nbecker
Comment 48 Jon Ciesla 2014-09-15 08:00:54 EDT
Git done (by process-git-requests).

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