Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225631 - Merge Review: busybox
Summary: Merge Review: busybox
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Patrice Dumas
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 17:48 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-09-04 12:46:52 UTC
pertusus: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 17:48:26 UTC
Fedora Merge Review: busybox

http://cvs.fedora.redhat.com/viewcvs/devel/busybox/
Initial Owner: varekova@redhat.com

Comment 1 Patrice Dumas 2007-02-18 12:39:31 UTC
* instead of mv the files to reverse the patch, I suggest
patch -R -p1 < %{PATCH0}

* Is DOLFS really used? I can't find it in the sources

* the man page timestamp should be kept with -p

* buildroot is not the preferred one

* At least the selinux patch should be proposed upstream. Has it 
  been done?

* the .static patch and the .anaconda are unreadable, although they
  bring in important changes. I think there should be a comment 
  explaining verbally what is done

* the whole process should also be commented since it is not trivial.
  For example something along (maybe dispatched where things are done):

# in %prep the .static patch is applied, to have a static busybox
# built. The executable is kept as busybox-static.
# then the .static patch is reverted and the .anaconda patch is 
# applied to generate the busybox especially tailored for anaconda.



Suggestion:
* / between $RPM_BUILD_ROOT/%{_mandir} is not useful

* use %defattr(-,root,root,-) instead of %defattr(-,root,root)

* 
%patch8 -b .gcc111 -p1
should certainly be
%patch8 -b .gcc41 -p1


Comment 2 Ivana Varekova 2007-02-19 15:32:49 UTC
Thanks for your comments.
The fixed version is busybox-1.2.2-6.fc7.
(In reply to comment #1)
> * instead of mv the files to reverse the patch, I suggest
> patch -R -p1 < %{PATCH0}
changed

> * Is DOLFS really used? I can't find it in the sources
removed 

> * the man page timestamp should be kept with -p
fixed 

> * buildroot is not the preferred one
fixed
 
> * At least the selinux patch should be proposed upstream. Has it 
>   been done?
I'm investigating it.

> * the .static patch and the .anaconda are unreadable, although they
>   bring in important changes. I think there should be a comment 
>   explaining verbally what is done

> * the whole process should also be commented since it is not trivial.
>   For example something along (maybe dispatched where things are done):
> 
> # in %prep the .static patch is applied, to have a static busybox
> # built. The executable is kept as busybox-static.
> # then the .static patch is reverted and the .anaconda patch is 
> # applied to generate the busybox especially tailored for anaconda.
changed

> Suggestion:
> * / between $RPM_BUILD_ROOT/%{_mandir} is not useful
> 
> * use %defattr(-,root,root,-) instead of %defattr(-,root,root)
> 
> * 
> %patch8 -b .gcc111 -p1
> should certainly be
> %patch8 -b .gcc41 -p1
> 

Comment 3 Patrice Dumas 2007-02-19 23:29:45 UTC
Suggestion:
install -p docs/BusyBox.1 $RPM_BUILD_ROOT/%{_mandir}/man1/busybox.1
chmod 644 $RPM_BUILD_ROOT/%{_mandir}/man1/busybox.1

may be done in one command

install -p -m644 docs/BusyBox.1 $RPM_BUILD_ROOT/%{_mandir}/man1/busybox.1


In my opinion, a must fix item:
I insist on having a comment explaining what is in the shipped
busybox (that is explaining .static and .anaconda patches
that are basically unreadable).



Comment 4 KaiGai Kohei 2007-07-26 15:48:52 UTC
>> * At least the selinux patch should be proposed upstream. Has it 
>>   been done?
> I'm investigating it.

The upstreamed selinux patch cannot apply busybox 1.2.x as is.

These are implemented for the latest busybox (1.6.x), and not completed yet.

Comment 5 Patrice Dumas 2007-08-27 13:33:25 UTC
The package is in a much better shape. The patches are now
readable, as the spec is. Well done. 

I still have some comments, but they are not blockers.

I spot some remnants from the past:
#SELINUX Patch

%ifarch ppc64
#%patch4 -b .ppc64 -p1
%endif

mkdir -p $RPM_BUILD_ROOT/%{_mandir}/man1


Maybe a comment explaining that the petitboot .config file comes
from a previous version so the depconfig file is recreated using 
make oldconfig non interactively may be added -- or something like 
this.

You could use %__cc instead of hardcoding gcc.

Using other optflags than RPM_OPT_FLAGS (like -Os) is not considered 
right by some reviewers. I personally don't care much, I guess
you have a valid reason to do so. You must add a comment, though:

http://fedoraproject.org/wiki/Packaging/Guidelines#head-8b14098227aebff1cf6188939e9d0877295ac448

Also the build doesn't show the options used during compilation. 
How can they be checked? This deserves a spec file comment.


Nothing is a blocker, except if the compile flags turn out to
be wrong.

APPROVED

Comment 6 Ivana Varekova 2007-09-04 12:46:52 UTC
Thanks for your comments. Fixed in busybox-1.6.1-2.fc8.


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