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.
The licence of is not GNU GPL, it's something like revised BSD license.
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.
- 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
%{!?dist} really doesn't look too good ;-) Thanks for the review, the above links now contains updated spec/rpm versions.
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.
(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.
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.
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.
ping?
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.
Matthias, thanks for the great work reviewing bsdiff!
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.
bsdiff is now built, thanks.
Package listed on the EPEL wishlist: http://fedoraproject.org/wiki/EPEL/WishList Package Change Request ====================== Package Name: bsdiff New Branches: EL-4 EL-5
cvs done.