Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 186566
Summary: | Review Request: bsdiff - binary diff/patch utility | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jindrich Novy <jnovy> |
Component: | Package Review | Assignee: | Matthias Saou <matthias> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | davidf, matthias, pknirsch |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-04-22 07:07:16 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Jindrich Novy
2006-03-24 14:19:00 UTC
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. |