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.
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.
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?
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
Rex, what about the "dandling symlink" stuff? People on #fedora-extras seem to agree that it can be ignored, while not perfect.
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.
I noticed my comment #3 was mis-typed. I meant to say: The symlink thing is not *that* important. ^^^ (forgot the not)
Symlink thing is fixed. Thanks Rex. http://nbecker.dyndns.org:8080/kdiff3-0.9.88-3.src.rpm
(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.
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> - 0.9.88-3 - Fix symlinks (from Rex Dieter) + - release 3 * Fri May 12 2006 Neal Becker <ndbecker2> - 0.9.88-2 + - release 2 + + * Fri May 12 2006 Neal Becker <ndbecker2> - 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
(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?
Yet another comment: you should see http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
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 || :
kdiff doesn't appear to include any shared libs, so /sbin/ldconfig isn't necessary in %%post/%%postun.
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?
Fixed /usr/share/applnk Added %post, %postun http://nbecker.dyndns.org:8080/kdiff3-0.9.88-4.src.rpm
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
the package still owns directories it shouldn't Please post a link to a spec file also
specfile link is in the initial comment.
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
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
patch merged rm applnk rm'd buildrequires fixed http://nbecker.dyndns.org:8080/kdiff3-0.9.88-5.src.rpm
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.
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).
Yes, I need a sponsor. I am also waiting for legal clearance from my employer (I initiated that yesterday).
%{_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.
...! Please ignore comment #25. Obviously it is not for this bug. Where is the cancel button?! :-(
BTW version 0.9.90 is out.
Neal ping on status of your employer ok
My employer legal dept has OK'd this.
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?
Yes, I have several submissions waiting on a sponsor.
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.
URLs updated: Spec URL: http://nbecker.dyndns.org:8080/RPM/kdiff3.spec SRPM URL: http://nbecker.dyndns.org:8080/RPM/kdiff3-0.9.90-5.src.rpm
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.
I believe all of the above are fixed. Please try: http://nbecker.dyndns.org:8080/RPM/kdiff3-0.9.90-6.src.rpm
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.
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 || :
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
(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.
This package has been imported and build, so I see no reason for leaving this ticker open, closing.
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.
Created attachment 308486 [details] add a conditional BR, allowing build in EPEL-5
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]
cvs done.
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).
Created attachment 309861 [details] removing two lines from doc/nl/index.docbook allows build in C4
Package Change Request ====================== Package Name: kdiff3 New Branches: epel7 Owners: nbecker
Git done (by process-git-requests).