Bug 186566 - Review Request: bsdiff - binary diff/patch utility
Review Request: bsdiff - binary diff/patch utility
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Saou
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-03-24 09:19 EST by Jindrich Novy
Modified: 2013-07-02 19:15 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-04-22 03:07:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jindrich Novy 2006-03-24 09:19:00 EST
Spec Name or Url: http://people.redhat.com/jnovy/files/bsdiff.spec
SRPM Name or Url: http://people.redhat.com/jnovy/files/bsdiff-4.3-1.src.rpm
Description:

Hi, the bsdiff package can do binary patches of smaller size that generated
by xdelta or similar utilities in most cases so it's IMO a good idea to have it in Fedora Extras.
Comment 1 David Nečas 2006-03-24 16:34:16 EST
The licence of is not GNU GPL, it's something like revised BSD license.
Comment 2 Jindrich Novy 2006-03-26 12:41:39 EST
Confirmed that bsdiff is lincensed according to BSD license as can be seen from
the source header. So switching it to BSD.

The above links now point to updated spec/srpm. Thanks for noticing this.
Comment 3 Matthias Saou 2006-03-29 08:57:54 EST
- The %{!?dist} is wrong, you probably meant %{?dist} :-)
- Passing -n %{name}-%{version} to %setup is not needed, as it's the sane default
- You're missing a "rm -rf ${RPM_BUILD_ROOT}" as the first line of %install
- Why Source0 and Patch1? Would seem more logical to start at Patch0
Comment 4 Jindrich Novy 2006-03-30 10:53:03 EST
%{!?dist} really doesn't look too good ;-)

Thanks for the review, the above links now contains updated spec/rpm versions.
Comment 5 Matthias Saou 2006-03-30 11:26:11 EST
Well, trying to rebuild was yet another story :-)
- Why force CC=gcc? It's the default with GNU make, isn't it?
- You patch the makefile in the install section, but don't use "make install"...
you should document that (with a quick comment), since it's to fix the
Makefile's validity!
- CFLAGS wasn't set to use the optflags.
- bzip2-devel build requirement was missing
Maybe others...
I've modified your patch and made changes to the spec :
http://ftp.es6.freshrpms.net/tmp/extras/bsdiff/

I've taken the approach to "heavily" patch the Makefile instead of manually
installing the files, since if in a later version the Makefile is modified to
build more binaries, they won't get missed, and if it's in a way that the patch
doesn't apply cleanly anymore, you'll instantly know and give it the attention
it requires :-)
One could even go one step further and also create the parent directories inside
the Makefile instead of before calling make install... why not.
Comment 6 Jindrich Novy 2006-03-30 11:55:03 EST
(In reply to comment #5)
> Well, trying to rebuild was yet another story :-)
> - Why force CC=gcc? It's the default with GNU make, isn't it?
It doesn't seem so. The default is CC=cc, which is actually a symlink to gcc, so
I just wanted to be sure gcc is called. I've got no problem to get rid of the
CC=gcc, so I removed it.

> - You patch the makefile in the install section, but don't use "make install"...
Note that the compiling process then fails when the .ifndef/.endif lines are
present within the Makefile with:

Makefile:13: *** missing separator.  Stop.
error: Bad exit status from /var/tmp/rpm-tmp.55595 (%build)

no matter whether the install or an other target is called. So the patch makes
the minimal changes to the Makefile so that it lets you compile it.

I'm more like minimalist so instead of adding huge patches to upstream makefiles
(actually rewriting them in your case) ;-), I'd apply only a minimal change to
the upstream one or make compilation manually in spec since the build process
here is trivial with no dependencies.

> you should document that (with a quick comment), since it's to fix the
> Makefile's validity!
???

> - CFLAGS wasn't set to use the optflags.
Yes, I missed this one. Added.

> - bzip2-devel build requirement was missing
Added.
Comment 7 Matthias Saou 2006-03-30 12:17:06 EST
Good!
What I meant where you answered "???" was that above the "Patch:" line of the
spec, you could put someething like :

# We patch the Makefile, but only to fix the buils since we don't use "make install"

Or something like that, because at first it seems like the patch "fixes"
something in the "install:" part of the Makefile.

Okay, I'm being picky here, but these inlined comments never harm :-)

You'll also need to patch out -O3 from the Makefile, or add CFLAGS=... to the
make line and not forget to add -lbz2 to it.
Comment 8 Jindrich Novy 2006-03-31 05:47:38 EST
Ok, so here we go with the one of the final variants of the spec file ;-) I
decided not to patch the broken upstream Makefile at all and do the build by the
two gcc invocations itself. Please tell me if you are ok with this approach.

Thanks.
Comment 9 Jindrich Novy 2006-04-03 09:44:01 EDT
ping?
Comment 10 Matthias Saou 2006-04-07 09:21:12 EDT
All good to go! Minor point :
- The source doesn't include a separate file containing the license. According
to the packaging guidelines, you should query upstream to include one.
Comment 11 Jindrich Novy 2006-04-07 15:04:30 EDT
Matthias, thanks for the great work reviewing bsdiff!
Comment 12 Matthias Saou 2006-04-21 17:49:04 EDT
I saw you imported it at last... don't forget to add it to owners.list and close
this bugzilla entry once the packages are built.
Comment 13 Jindrich Novy 2006-04-22 03:07:16 EDT
bsdiff is now built, thanks.
Comment 14 Jindrich Novy 2007-12-10 07:16:23 EST
Package listed on the EPEL wishlist:

http://fedoraproject.org/wiki/EPEL/WishList

Package Change Request
======================
Package Name: bsdiff
New Branches: EL-4 EL-5
Comment 15 Kevin Fenzi 2007-12-10 11:44:41 EST
cvs done.

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