Bug 454482 - Review Request: vbindiff - Visual binary diff
Summary: Review Request: vbindiff - Visual binary diff
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Till Maas
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-08 18:53 UTC by Nicoleau Fabien
Modified: 2008-12-30 23:52 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-12-30 23:49:16 UTC
Type: ---
Embargoed:
opensource: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
build log (9.42 KB, application/octet-stream)
2008-07-25 22:12 UTC, Nicoleau Fabien
no flags Details

Description Nicoleau Fabien 2008-07-08 18:53:48 UTC
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/vbindiff.spec
SRPM URL: http://nicoleau.fabien.free.fr/rpms/srpms.fc9/vbindiff-3.0-0.1.beta3.fc9.src.rpm
Description: 
VBinDiff (Visual Binary Diff) displays files in hexadecimal and ASCII (or EBCDIC). It can also display two files at once, and highlight the differences between them. Unlike diff, it works well with large files (up to 4 GB).

VBinDiff was inspired by the Compare Files function of the ProSel utilities by Glen Bredon, for the Apple II.

The single-file mode was inspired by the LIST utility of 4DOS and friends. While less provides a good line-oriented display, it has no equivalent to LIST's hex display. (True, you can pipe the file through hexdump, but that's incredibly inefficient on multi-gigabyte files.)

rebuild under mock is OK for : fedora-rawhide-i386,fedora-9-i386,fedora-8-i386,epel-4-i386,epel-5-i386

rpmlint output : 
[builder@FEDOBOX tmp]$ rpmlint vbindiff-3.0-0.1.beta3.fc9.i386.rpm vbindiff-3.0-0.1.beta3.fc9.src.rpm vbindiff-debuginfo-3.0-0.1.beta3.fc9.i386.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 1 Tim Colles 2008-07-25 12:54:08 UTC
Hi. This is just an informal review with some comments you might
find helpful.

The README mentions that the license can be found in the file COPYING
but that file does not exist anywhere in the extracted archive. The
source files in "curses" (and equivalents under win32) do not contain
any license statement within them (nor does tables.h at top level). The
COPYING file (or equivalent) should also be included in %doc. It would
be a good idea to push these changes upstream if possible.

The license for the putty.src file (modified from terminfo/ncurses) is
not GPL but probably some MIT variant, this should be correctly clarified
in the License: value and ideally in the package README, see
https://fedoraproject.org/wiki/Licensing for details - ideally this
change should be pushed upstream to the original author.

There is a message during creating debuginfo:

  cpio: vbindiff-3.0_beta3/<built-in>: No such file or directory

Although it doesn't seem to break the debug package.

OK Rpmlint is silent
OK Spec file name.
NO Licensing.
NO License: field matches sw.
NO License included in doc.
OK Spec file in American English
OK Spec file legible
OK Source MD5sum: 86904b2394e56089878695415121cc28 upstream and in src.rpm
OK Builds on i386
OK See no reason to exclude architecture(s)
?? All build dependencies listed  (mock test ??)
   DIDN'T TRY A MOCK BUILD
OK Locales management - this package is not localized.
OK Libraries (no libraries in this package)
OK Not relocateble.
OK Owns it's directories.
OK No %file duplicates
OK File permissions
OK %clean target
OK Consistent macro usage
OK Code/permissive content (this is just code).
OK No large documentation
OK Doc's don't affect runtime.
OK No header files
OK No static libraries
OK No pkgconfig file(s)
OK No libraries
OK No devel package
OK Not a GUI application
OK Does not own other package's file/dirs.
OK rm -rf %{buildroot} in %install
OK Valid filenames (just ASCII)
OK No scriplets
OK No subpackages
OK No pkgconfig files
OK No file dependencies


Comment 2 Nicoleau Fabien 2008-07-25 22:07:00 UTC
Hi, thanks a lot for this informal review.
- Licence : I've contacted upstream about the licensing blur. 
- cpio error : I don't have any messages about cpio (see build log in attachment).
- mock : package build succesfully on mock (tried for
fedora-rawhide-i386,fedora-9-i386,fedora-8-i386,epel-4-i386 and epel-5-i386)
- also build on rawhide. see
http://koji.fedoraproject.org/koji/taskinfo?taskID=740007


Comment 3 Nicoleau Fabien 2008-07-25 22:12:38 UTC
Created attachment 312689 [details]
build log

build log

Comment 4 Nicoleau Fabien 2008-07-27 21:36:46 UTC
Update for 3.0 beta 4 version :
Spec URL: http://nicoleau.fabien.free.fr/rpms/SPECS/vbindiff.spec
SRPM URL:
http://nicoleau.fabien.free.fr/rpms/srpms.fc9/vbindiff-3.0-0.1.beta4.fc9.src.rpm

COPYING file is now included in the package (added by upstream). Source files in
"curses", equivalent under win32 and table.h now contains a licence statement
(added by upstream). 
For putty.src file, here is upstream note :
As explained in the ncurses dist, there's no clear copyright on the terminfo
file.  I did add a note to README that putty.src is not part of VBinDiff and
should be considered under the ncurses terms.

Rebuild under mock still ok.
rpmint output : 
[builder@FEDOBOX tmp]$ rpmlint vbindiff-3.0-0.1.beta4.fc9.i386.rpm
vbindiff-3.0-0.1.beta4.fc9.src.rpm vbindiff-debuginfo-3.0-0.1.beta4.fc9.i386.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
[builder@FEDOBOX tmp]$ 



Comment 5 Till Maas 2008-12-09 23:29:55 UTC
[OK] rpmlint output:silent
[OK] Spec in %{name}.spec format

[OK] license allowed: GPLv2+
[OK] license matches shortname in License: tag
[OK] license in tarball and included in %doc:

[OK] package is code or permissive content: code

{N/A} patches sent to upstream and commented
[OK] Source0 is a working URL
{N/A} Sourceforge URL is Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
<N/A> SourceX / PatchY prefixed with %{name}

[OK] Source0 matches Upstream:
dbda80ef580e1a0975ef50b9aaa5210e  vbindiff-3.0_beta4.tar.gz

[OK] Package builds on all platforms:
http://koji.fedoraproject.org/koji/taskinfo?taskID=989950
[N/A] ExcludeArch bugs are filed and commented:
[OK] BuildRequires are complete (mock builds)
(OK) No file dependencies outside of /etc /bin /sbin /usr/bin /usr/sbin 

[N/A] %find_lang used for locales

[N/A] Every (sub)package containing libraries runs ldconfig
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
[N/A] .h (header) files are in -devel subpackage
[N/A] .a (static libraries) are in -static subpackage
[N/A] contains .pc (pkgconfig) files and has Requires: pkgconfig
(N/A) .pc files are in -devel subpackage
[N/A] contains .so.X(.Y) files and .so is in -devel
[N/A] -devel subpackage has Requires: %{name} = %{version}-%{release}
[N/A] .la files (libtool) are not included


[N/A] Has GUI and includes %{name}.desktop
[N/A] Follows desktop entry spec
[N/A] Valid .desktop Name
[N/A] Valid .desktop GenericName
[N/A] Valid .desktop Categories

[N/A] Valid .desktop StartupNotify
[N/A] .desktop file installed with desktop-file-install in %install

[OK] Prefix: /usr not used (not relocatable)

[OK] Owns all created directories
[OK] no duplicates in %files
[OK] %defattr(-,root,root,-) is in every %files section
[OK] Does not own files or dirs from other packages
[OK] included filenames are in UTF-8

[OK] %clean is rm -rf %{buildroot} or $RPM_BUILD_ROOT 
[OK] %install starts with rm -rf %{buildroot} or $RPM_BUILD_ROOT 

[OK] Consistent macro usage

[N/A] large documentation is -doc subpackage
[OK] %doc does not affect runtime

{OK} no pre-built binaries (.a, .so*, executable)
{OK} well known BuildRoot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

{OK} PreReq not used
{OK} RPM_OPT_FLAGS honoured
{OK} Useful debuginfo generated
{OK} no duplication of system libraries
{OK} no rpath
{NEEDSWORK} Timestamps preserved with cp and install
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
Consider using this to keep the timestamp of the manpage:

%configure INSTALL="install -p"

But imho this should be done directly in %configure, I have sent a mail about
this to fedora-packaging.

{OK} Uses parallel make (%{?_smp_mflags})
{OK} Requires(pre,post) style notation not used
{OK} no Conflicts
{OK} nothing installed in /srv
{OK} Changelog in allowed format
{OK} does not use Scriptlets
<N/A> Architecture independent packages have: BuildArch: noarch
<OK> Sane Provides: and Requires:

{GOOD ENOUGH} Follows Naming Guidelines
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages

According to the changelog/Review ticket, this is the second prelease package
with the same version. Therefore the release should be '0.2.beta4' and not '0.1.beta4'.
The digit needs to be always increased to avoid possible problems with rpms
version-release compare algorithm.


This package is APPROVED. Please consider the extra argument to %configure before you import this package into cvs and do not forget to increase the release properly.

Comment 6 Nicoleau Fabien 2008-12-10 17:59:27 UTC
Hi, thank you very much for the review. I'll fix release tag and configure call before I import the package.


New Package CVS Request
=======================
Package Name: vbindiff
Short Description: Visual binary diff
Owners: eponyme
Branches: F-9 F-10
InitialCC:

Comment 7 Nicoleau Fabien 2008-12-10 22:19:14 UTC
Oups, I forgot EPEL 5 branche :

New Package CVS Request
=======================
Package Name: vbindiff
Short Description: Visual binary diff
Owners: eponyme
Branches: F-9 F-10 EL-5
InitialCC:

Comment 8 Kevin Fenzi 2008-12-14 04:57:44 UTC
cvs done.

Comment 9 Fedora Update System 2008-12-14 16:51:52 UTC
vbindiff-3.0-0.2.beta4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/vbindiff-3.0-0.2.beta4.fc10

Comment 10 Fedora Update System 2008-12-14 16:53:24 UTC
vbindiff-3.0-0.2.beta4.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/vbindiff-3.0-0.2.beta4.fc9

Comment 11 Fedora Update System 2008-12-18 00:32:20 UTC
vbindiff-3.0-0.2.beta4.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update vbindiff'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11342

Comment 12 Fedora Update System 2008-12-18 00:32:34 UTC
vbindiff-3.0-0.2.beta4.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update vbindiff'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-11408

Comment 13 Fedora Update System 2008-12-30 23:49:11 UTC
vbindiff-3.0-0.2.beta4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 14 Fedora Update System 2008-12-30 23:52:24 UTC
vbindiff-3.0-0.2.beta4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


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