[Bug 225631] New: Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

Summary: Merge Review: busybox
           Product: Fedora Extras
           Version: devel
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: normal
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail]

Fedora Merge Review: busybox

http://cvs.fedora.redhat.com/viewcvs/devel/busybox/
Initial Owner: [e-mail]

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

------- Additional Comments From [e-mail]  2007-02-18 07:39 EST -------
* 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

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |MODIFIED
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

------- Additional Comments From [e-mail]  2007-02-19 10:32 EST -------
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

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

------- Additional Comments From [e-mail]  2007-02-19 18:29 EST -------
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).

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |ASSIGNED

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEEDINFO
               Flag|                            |needinfo?

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |medium
           Priority|normal                      |medium
            Product|Fedora Extras               |Fedora

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |ASSIGNED
               Flag|needinfo?                   |

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

------- Additional Comments From [e-mail]  2007-07-26 11:48 EST -------
> > 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.

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|[e-mail]         |[e-mail]

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

------- Additional Comments From [e-mail]  2007-08-27 09:33 EST -------
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-8b14098227aebff1cf61...

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

[Bug 225631] Merge Review: busybox

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: busybox

https://bugzilla.redhat.com/show_bug.cgi?id=225631

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |NEXTRELEASE

------- Additional Comments From [e-mail]  2007-09-04 08:46 EST -------
Thanks for your comments. Fixed in busybox-1.6.1-2.fc8.