Bug 785785 - Review Request: pkgdiff - A tool for analyzing changes in Linux software packages
Summary: Review Request: pkgdiff - A tool for analyzing changes in Linux software pack...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Matthias Runge
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-01-30 15:53 UTC by Richard Shaw
Modified: 2012-03-08 05:06 UTC (History)
5 users (show)

Fixed In Version: pkgdiff-1.2-1.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-03-06 20:29:12 UTC
Type: ---
mrunge: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Richard Shaw 2012-01-30 15:53:11 UTC
Spec URL: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff.spec
SRPM URL: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff-1.0-1.fc16.src.rpm
Description:
Package Changes Analyzer (pkgdiff) is a tool for analyzing changes
in Linux software packages (RPM, DEB, TAR.GZ, etc). The tool is
intended for Linux maintainers who are interested in ensuring
compatibility of old and new versions of packages.

$ rpmlint SRPMS/pkgdiff-1.0-1.fc16.src.rpm RPMS/noarch/pkgdiff-1.0-1.fc16.noarch.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 1 Matthias Runge 2012-02-06 18:57:39 UTC
taking this one

Comment 2 Matthias Runge 2012-02-06 19:13:04 UTC
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[!]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generates any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.

rpmlint pkgdiff-1.0-1.fc17.noarch.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint pkgdiff-1.0-1.fc17.src.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/mrunge/785785/pkgdiff-1.0.tar.gz :
  MD5SUM this package     : 55a856bdfed4781f55980ac813f63bc8
  MD5SUM upstream package : 55a856bdfed4781f55980ac813f63bc8

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[-]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: rfcdiff seems bundled from another site

http://tools.ietf.org/tools/rfcdiff/

It is not packaged for fedora and it's a simple shell script, but it violates imho the no bundled libs policy. You should ask on the fedora-packaging list, what to do. If they say, it's ok, I'd approve this package.

Comment 3 Richard Shaw 2012-02-06 20:29:51 UTC
It's probably easier to go ahead and do a quick review request for the bundled app as long as you're willing to review it :)

Richard

Comment 4 Matthias Runge 2012-02-06 20:33:07 UTC
ok, go ahead ;-)

Comment 5 Richard Shaw 2012-02-10 15:24:36 UTC
SPEC: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff.spec
SRPM: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff-1.0-2.fc16.src.rpm

Ok, rfcdiff unbundled and review request submitted!

Comment 6 Matthias Runge 2012-02-14 08:17:16 UTC
theres a missing dependency to rfcdiff in specfile.

Comment 8 Andrey Ponomarenko 2012-02-15 19:37:43 UTC
PkgDiff requires changed version of rfcdiff tool and almost every future version of PkgDiff will add more and more changes and improvements to the bundled rfcdiff tool (modules/Internals/Tools/rfcdiff).

So, consider applying these changes to the package that will provide rfcdiff if you'll decide to make it to be a separate package.

Comment 9 Richard Shaw 2012-02-15 19:46:04 UTC
hmm... In that case, could we just consider it a fork and leave it alone? Maybe even change the name?

Richard

Comment 10 Andrey Ponomarenko 2012-02-15 20:02:34 UTC
I'm not familiar enough with Fedora policies, but change the name of bundled rfcdiff tool is not a problem for me if it will be necessary. Licenses of both programs are GPL.

Also, the PkgDiff tool is now under very active development. The next version (1.2) will be released in two days.

The 1.0 version of the tool contains a bug that prevent the tool from working correctly if /tmp system directory is a symbolic link. And I think this bug may affect Fedora 17. This bug has been fixed in 1.1 and I recommend you to upgrade to 1.1 or newer versions.

Comment 11 Richard Shaw 2012-02-15 20:10:57 UTC
I would argue that since the alterations are necessary for pkgdiff to function, it's already technically a fork, changing the name just makes it more obvious.

If that works for you, can it be included in the 1.2 release? If so, I'll just wait to update the package for Fedora until then.

Thanks,
Richard

Comment 12 Andrey Ponomarenko 2012-02-15 20:31:10 UTC
Ok, bundled rfcdiff tool will be renamed to rfcdiff-1.41-ROSA.sh or so in the 1.2 version of PkgDiff. It's scheduled on Fri, 17 Feb.

Comment 13 Matthias Runge 2012-02-16 08:45:00 UTC
(In reply to comment #12)
> Ok, bundled rfcdiff tool will be renamed to rfcdiff-1.41-ROSA.sh or so in the
> 1.2 version of PkgDiff. It's scheduled on Fri, 17 Feb.

Andrey, are you going to maintain rfcdiff included in pkgdiff?

If yes, I'd tend to view rfcdiff as fundamental part of pkgdiff; it may be allowed to leave it in pkgdiff. Then I'd view pkgdiff as total package and would drop rfcdiff as package. (so suggesting to package only pkgdiff, including a modified version of rfcdiff.)

Comment 14 Andrey Ponomarenko 2012-02-16 12:13:04 UTC
Yes, I am going to maintain it because I am an upstream developer of pkgdiff. Please consider rfcdiff included in pkgdiff as a fork of rfcdiff.

But I also recommend you to leave a separate package for rfcdiff in Fedora because it's a very useful tool. I think it's a best visual HTML diff tool for text files among existing alternatives.

Thanks.

Comment 15 Matthias Runge 2012-02-16 12:41:23 UTC
Andrey, thanks, this is great to hear.

We need to take care, both (rfcdiff) and rfcdiff-1.41-ROSA.sh don't mix with each other. 
If we separate both rfcdiffs, it fine with me, and I will approve this package.

Richard, are you going to wait until tomorrow and updating your package to version 1.2? I'll then  take another look and like to finish this review.

Comment 16 Richard Shaw 2012-02-16 14:22:37 UTC
(In reply to comment #15)
> Andrey, thanks, this is great to hear.
> 
> We need to take care, both (rfcdiff) and rfcdiff-1.41-ROSA.sh don't mix with
> each other. 

That shouldn't be a problem since now they're named differently, although it wouldn't have mattered anyway since it was being installed in /usr/share/pkgdiff/... not in /usr/bin.

> If we separate both rfcdiffs, it fine with me, and I will approve this package.
> 
> Richard, are you going to wait until tomorrow and updating your package to
> version 1.2? I'll then  take another look and like to finish this review.

That's the plan!

Thanks,
Richard

Comment 17 Andrey Ponomarenko 2012-02-17 08:35:06 UTC
Please, get new version here: https://github.com/downloads/pkgdiff/pkgdiff/pkgdiff-1.2.tar.gz

Comment 19 Matthias Runge 2012-02-17 19:27:50 UTC
looks fine to me, no issues found.

APPROVED

Comment 20 Richard Shaw 2012-02-17 21:10:36 UTC
New Package SCM Request
=======================
Package Name: pkgdiff
Short Description: A tool for analyzing changes in Linux software packages
Owners: hobbes1069
Branches: f16 f17
InitialCC:

Comment 21 Gwyn Ciesla 2012-02-19 20:42:55 UTC
Git done (by process-git-requests).

Comment 22 Richard Shaw 2012-02-19 20:55:22 UTC
Thanks for the review!

Comment 23 Fedora Update System 2012-02-27 17:36:48 UTC
pkgdiff-1.2-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pkgdiff-1.2-1.fc16

Comment 24 Fedora Update System 2012-02-27 17:36:59 UTC
pkgdiff-1.2-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/pkgdiff-1.2-1.fc17

Comment 25 Fedora Update System 2012-02-27 22:30:14 UTC
pkgdiff-1.2-1.fc17 has been pushed to the Fedora 17 testing repository.

Comment 26 Fedora Update System 2012-03-06 20:29:12 UTC
pkgdiff-1.2-1.fc17 has been pushed to the Fedora 17 stable repository.

Comment 27 Fedora Update System 2012-03-08 05:06:29 UTC
pkgdiff-1.2-1.fc16 has been pushed to the Fedora 16 stable repository.


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