Bug 994434 - (kompare) Review Request: kompare - Diff tool
Review Request: kompare - Diff tool
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
Depends On:
Blocks: kde-reviews 990444
  Show dependency treegraph
 
Reported: 2013-08-07 05:42 EDT by Jan Grulich
Modified: 2013-08-15 17:25 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-15 03:32:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jan Grulich 2013-08-07 05:42:58 EDT
Spec URL: http://jgrulich.fedorapeople.org/kompare.spec
SRPM URL: http://jgrulich.fedorapeople.org/kompare-4.10.97-1.fc19.src.rpm
Description: Tool to visualize changes between two versions of a file
Fedora Account System Username: jgrulich

Successful build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5788909

This package was previously part of kdesdk. Now it's distributed separately in KDE 4.11.
Comment 1 Christopher Meng 2013-08-07 05:45:08 EDT
Please add virtual provides

Provides:  mergetool
Comment 2 Jan Grulich 2013-08-07 05:55:06 EDT
Provides: mergetool

Spec URL: http://jgrulich.fedorapeople.org/kompare.spec
SRPM URL: http://jgrulich.fedorapeople.org/kompare-4.10.97-1.fc19.src.rpm
Comment 3 Mario Blättermann 2013-08-11 16:51:42 EDT
Just a few initial comments:

Don't use hardcoded paths in the file lists.

/usr/include/kde4/kompare/kompareinterface.h

has to be

 %{_kde4_includedir}/kompare/kompareinterface.h

See https://fedoraproject.org/wiki/SIGs/KDE/Packaging/BestPractices?rd=SIGs/KDE/Packaging/Guidelines#Macros


Obsoletes:      kdesdk-kompare < 4.10.80

doesn't include all versions prior to the current one. Either use

Obsoletes:      kdesdk-kompare <= 4.10.80

or refer to the current package version:

Obsoletes:      kdesdk-kompare < %{version}

Well, in this certain case there won't be a kompare-4.10.97 due to the new release policy, but we would be on the safer side in any case.


It's not useful to repeat the summary in the descriptions. My suggestions:

%description libs
This package contains shared libraries for %{name}.

%description    devel
The %{name}-devel package contains libraries and header files for
developing applications that use %{name}.


Moreover, rpmlint gives me the following error message for your spec file:

kompare.spec:26: W: unversioned-explicit-provides mergetool
The specfile contains an unversioned Provides: token, which will match all
older, equal, and newer versions of the provided thing.  This may cause update
problems and will make versioned dependencies, obsoletions and conflicts on
the provided thing useless -- make the Provides versioned if possible.

Would be be useful here to add the version number, as you already did in the Obsoletes: and Conflicts: tags?
Comment 4 Kevin Kofler 2013-08-11 17:18:56 EDT
> Obsoletes:      kdesdk-kompare < 4.10.80
>
> doesn't include all versions prior to the current one. Either use
>
> Obsoletes:      kdesdk-kompare <= 4.10.80
>
> or refer to the current package version:
>
> Obsoletes:      kdesdk-kompare < %{version}
>
> Well, in this certain case there won't be a kompare-4.10.97 due to the new
> release policy, but we would be on the safer side in any case.

I think it is actually the best practice to not Obsolete more than necessary, and also, Obsoletes with <= is usually a bad idea. (It may work with Version only (as here), but if you try using <= with a Version-Release, you'll have trouble with the disttags.) And < %{version} in Obsoletes is also not that nice, it will unnecessarily Obsolete versions that were never released in the first place.

I'd keep < 4.10.80 for the Obsoletes version here.


> It's not useful to repeat the summary in the descriptions.

Well, that's what we use here in the KDE SIG when we cannot think of a better description. ;-)
Comment 5 Jan Grulich 2013-08-13 04:25:38 EDT
Fixed description and hardcoded path, but I'm not sure what to do with virtual providing of mergetool, because I think it should be unversioned.

Spec URL: http://jgrulich.fedorapeople.org/kompare.spec
SRPM URL: http://jgrulich.fedorapeople.org/kompare-4.10.97-1.fc19.src.rpm
Comment 6 Christopher Meng 2013-08-13 04:26:57 EDT
(In reply to Jan Grulich from comment #5)
> Fixed description and hardcoded path, but I'm not sure what to do with
> virtual providing of mergetool, because I think it should be unversioned.


???Isn't it unversioned now?
Comment 7 Jan Grulich 2013-08-13 04:45:35 EDT
Yes, it is, but Mario Blättermann wrote, that should be versioned and I think it should stay as it is.
Comment 8 Michael Schwendt 2013-08-13 05:54:57 EDT
"mergetool" is a non-versioned capability and not a virtual package that will ever be obsoleted.
https://bugzilla.redhat.com/buglist.cgi?quicksearch=mergetool
Comment 9 Mario Blättermann 2013-08-13 15:17:31 EDT
Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5812705

$ rpmlint -i -v *
kompare.src: I: checking
kompare.src: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare.src:26: W: unversioned-explicit-provides mergetool
The specfile contains an unversioned Provides: token, which will match all
older, equal, and newer versions of the provided thing.  This may cause update
problems and will make versioned dependencies, obsoletions and conflicts on
the provided thing useless -- make the Provides versioned if possible.

kompare.src: I: checking-url http://download.kde.org/unstable/4.10.97/src/kompare-4.10.97.tar.xz (timeout 10 seconds)
kompare.armv7hl: I: checking
kompare.armv7hl: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare.armv7hl: W: devel-file-in-non-devel-package /usr/lib/libkomparediff2.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

kompare.armv7hl: W: gzipped-svg-icon /usr/share/icons/hicolor/scalable/apps/kompare.svgz
Not all desktop environments that support SVG icons support them gzipped
(.svgz).  Install the icon as plain uncompressed SVG.

kompare.armv7hl: W: devel-file-in-non-devel-package /usr/lib/libkomparedialogpages.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

kompare.armv7hl: W: no-manual-page-for-binary kompare
Each executable in standard binary directories should have a man page.

kompare.i686: I: checking
kompare.i686: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare.i686: W: devel-file-in-non-devel-package /usr/lib/libkomparediff2.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

kompare.i686: W: gzipped-svg-icon /usr/share/icons/hicolor/scalable/apps/kompare.svgz
Not all desktop environments that support SVG icons support them gzipped
(.svgz).  Install the icon as plain uncompressed SVG.

kompare.i686: W: devel-file-in-non-devel-package /usr/lib/libkomparedialogpages.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

kompare.i686: W: no-manual-page-for-binary kompare
Each executable in standard binary directories should have a man page.

kompare.x86_64: I: checking
kompare.x86_64: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare.x86_64: W: gzipped-svg-icon /usr/share/icons/hicolor/scalable/apps/kompare.svgz
Not all desktop environments that support SVG icons support them gzipped
(.svgz).  Install the icon as plain uncompressed SVG.

kompare.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libkomparediff2.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

kompare.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libkomparedialogpages.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

kompare.x86_64: W: no-manual-page-for-binary kompare
Each executable in standard binary directories should have a man page.

kompare-debuginfo.armv7hl: I: checking
kompare-debuginfo.armv7hl: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-debuginfo.i686: I: checking
kompare-debuginfo.i686: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-debuginfo.x86_64: I: checking
kompare-debuginfo.x86_64: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-devel.armv7hl: I: checking
kompare-devel.armv7hl: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-devel.armv7hl: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

kompare-devel.i686: I: checking
kompare-devel.i686: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-devel.i686: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

kompare-devel.x86_64: I: checking
kompare-devel.x86_64: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-devel.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

kompare-libs.armv7hl: I: checking
kompare-libs.armv7hl: W: spelling-error Summary(en_US) Runtime -> Run time, Run-time, Rudiment
The value of this tag appears to be misspelled. Please double-check.

kompare-libs.armv7hl: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-libs.armv7hl: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

kompare-libs.i686: I: checking
kompare-libs.i686: W: spelling-error Summary(en_US) Runtime -> Run time, Run-time, Rudiment
The value of this tag appears to be misspelled. Please double-check.

kompare-libs.i686: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-libs.i686: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

kompare-libs.x86_64: I: checking
kompare-libs.x86_64: W: spelling-error Summary(en_US) Runtime -> Run time, Run-time, Rudiment
The value of this tag appears to be misspelled. Please double-check.

kompare-libs.x86_64: I: checking-url http://www.kde.org/ (timeout 10 seconds)
kompare-libs.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

kompare.spec:26: W: unversioned-explicit-provides mergetool
The specfile contains an unversioned Provides: token, which will match all
older, equal, and newer versions of the provided thing.  This may cause update
problems and will make versioned dependencies, obsoletions and conflicts on
the provided thing useless -- make the Provides versioned if possible.

kompare.spec: I: checking-url http://download.kde.org/unstable/4.10.97/src/kompare-4.10.97.tar.xz (timeout 10 seconds)
13 packages and 1 specfiles checked; 0 errors, 23 warnings.


A lot of the warnings:

"no-documentation"
There's no documentation which you could add. Well, you could move the license file from the main to the libs package, but this doesn't make sense either and would only be for getting rpmlint silent. Keep it as is.

"unversioned-explicit-provides"
Has been discussed already.

"devel-file-in-non-devel-package"
Should be investigated. Due to the wildcards in the -libs filelist, it could happen that unversioned libraries which have associated versioned ones have been landed in the wrong package. Have a look at the file lists:

$ rpm -qpl kompare-libs-4.10.97-1.fc20.x86_64.rpm
/usr/lib64/libkomparedialogpages.so.4
/usr/lib64/libkomparedialogpages.so.4.11.0
/usr/lib64/libkomparediff2.so.4
/usr/lib64/libkomparediff2.so.4.11.0
/usr/lib64/libkompareinterface.so.4
/usr/lib64/libkompareinterface.so.4.11.0

[mariobl@localhost kompare]$ rpm -qpl kompare-4.10.97-1.fc20.x86_64.rpm
/usr/bin/kompare
/usr/lib64/kde4/komparenavtreepart.so
/usr/lib64/kde4/komparepart.so
/usr/lib64/libkomparedialogpages.so
/usr/lib64/libkomparediff2.so
/usr/share/applications/kde4/kompare.desktop
/usr/share/doc/HTML/en/kompare
/usr/share/doc/HTML/en/kompare/common
/usr/share/doc/HTML/en/kompare/dock.png
/usr/share/doc/HTML/en/kompare/index.cache.bz2
/usr/share/doc/HTML/en/kompare/index.docbook
/usr/share/doc/HTML/en/kompare/settings-diff1.png
/usr/share/doc/HTML/en/kompare/settings-diff2.png
/usr/share/doc/HTML/en/kompare/settings-diff3.png
/usr/share/doc/HTML/en/kompare/settings-diff4.png
/usr/share/doc/HTML/en/kompare/settings-view1.png
/usr/share/doc/HTML/en/kompare/settings-view2.png
/usr/share/doc/HTML/en/kompare/undock.png
/usr/share/doc/kompare
/usr/share/doc/kompare/COPYING
/usr/share/doc/kompare/README
/usr/share/icons/hicolor/128x128/apps/kompare.png
/usr/share/icons/hicolor/16x16/apps/kompare.png
/usr/share/icons/hicolor/22x22/apps/kompare.png
/usr/share/icons/hicolor/32x32/apps/kompare.png
/usr/share/icons/hicolor/48x48/apps/kompare.png
/usr/share/icons/hicolor/scalable/apps/kompare.svgz
/usr/share/kde4/apps/kompare
/usr/share/kde4/apps/kompare/komparepartui.rc
/usr/share/kde4/apps/kompare/kompareui.rc
/usr/share/kde4/services/komparenavtreepart.desktop
/usr/share/kde4/services/komparepart.desktop
/usr/share/kde4/servicetypes/komparenavigationpart.desktop
/usr/share/kde4/servicetypes/kompareviewpart.desktop

"gzipped-svg-icon"
"no-manual-page-for-binary"
Can be ignored, also the spelling errors.
Comment 10 Rex Dieter 2013-08-13 18:20:17 EDT
naming: ok

1. better url:
https://projects.kde.org/projects/kde/kdesdk/kompare

2. licensing, main pkg should be:
License: GPLv2+ and GFDL
add
%doc COPYING.DOC

(some parts of libdiff2 and headers are LGPLv2+, but they're combined with GPLv2+, so aggregate is GPLv2+)

3. obsoletes/provides not ok
-libs subpkg missing
Obsoletes: kdesdk-kompare-libs < 4.10.80

macros: ok

4. %files not ok, move these from main to -devel pkg:
%{_kde4_libdir}/libkomparedialogpages.so
%{_kde4_libdir}/libkomparediff2.so
or delete/omit these symlinks from packaging altogether, as there is apparently no api (ie, headers) associated with them (Kevin, correct me if you think this is inaccurate).
Comment 12 Rex Dieter 2013-08-14 08:39:03 EDT
Thanks, looks good.  APPROVED
Comment 13 Jan Grulich 2013-08-14 08:45:34 EDT
New Package SCM Request
=======================
Package Name: kompare
Short Description: Diff tool
Owners: than rdieter kkofler ltinkl jgrulich
Branches: f18 f19
InitialCC:
Comment 14 Gwyn Ciesla 2013-08-14 10:21:07 EDT
Git done (by process-git-requests).
Comment 15 Kevin Kofler 2013-08-15 17:25:48 EDT
The idea is that libkomparediff2 will be used by KDevelop (or actually kdevplatform, IIRC) in the future (it currently bundles a fork of that code), but of course that cannot happen until it starts installing header files. So there will be -devel material in the (near) future, but for now, the symlinks are just useless and can be deleted.

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