Red Hat Bugzilla – Bug 225631
Merge Review: busybox
Last modified: 2007-11-30 17:11:54 EST
Fedora Merge Review: busybox http://cvs.fedora.redhat.com/viewcvs/devel/busybox/ Initial Owner: varekova@redhat.com
* 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
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 >
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).
>> * 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.
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
Thanks for your comments. Fixed in busybox-1.6.1-2.fc8.