Bug 785785

taking this one

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.

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

ok, go ahead ;-)

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!

theres a missing dependency to rfcdiff in specfile.

SPEC: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff.spec
SRPM: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff-1.0-3.fc16.src.rpm

Whoops! Fixed.

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.

hmm... In that case, could we just consider it a fork and leave it alone? Maybe even change the name?

Richard

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.

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

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.

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

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.

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.

(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

Please, get new version here: https://github.com/downloads/pkgdiff/pkgdiff/pkgdiff-1.2.tar.gz

Thanks Andrey!

SPEC: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff.spec
SRPM: http://hobbes1069.fedorapeople.org/pkgdiff/pkgdiff-1.2-1.fc16.src.rpm

looks fine to me, no issues found.

APPROVED

New Package SCM Request
=======================
Package Name: pkgdiff
Short Description: A tool for analyzing changes in Linux software packages
Owners: hobbes1069
Branches: f16 f17
InitialCC:

Git done (by process-git-requests).

Thanks for the review!

pkgdiff-1.2-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pkgdiff-1.2-1.fc16

pkgdiff-1.2-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/pkgdiff-1.2-1.fc17

pkgdiff-1.2-1.fc17 has been pushed to the Fedora 17 testing repository.

pkgdiff-1.2-1.fc17 has been pushed to the Fedora 17 stable repository.

pkgdiff-1.2-1.fc16 has been pushed to the Fedora 16 stable repository.