[Bug 710902] Review Request: octave-struct - Structure handling for Octave

[bugzilla at redhat.com]

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


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

--- Comment #4 from José Matos <jamatos at fc.up.pt> 2011-06-13 14:07:13 EDT ---
(In reply to comment #3)
> (In reply to comment #2)
> 
> Thank you for taking the review!

You are welcome.

> > [!] : MUST - Rpmlint output is silent.
> 
> Please compare this to Bug 693798, the review of octave-image.
> 
> >         rpmlint octave-struct-1.0.9-1.fc16.i686.rpm
> >         octave-struct.i686: W: obsolete-not-provided octave-forge
> 
> As Orion Poplawski argued in the above referred bug, the obsoletes is
> necessary.
> 
> >         octave-struct.i686: W: hidden-file-or-dir
> > /usr/share/octave/packages/struct-1.0.9/packinfo/.autoload
> >         octave-struct.i686: E: zero-length
> > /usr/share/octave/packages/struct-1.0.9/packinfo/.autoload
> 
> This is the way the octave packaging system works; octave-image also contains
> such an empty .autoload file
> 
> >         octave-struct.i686: W: dangerous-command-in-%preun rm
> 
> This warning comes from the rpm macros installed by octave, and should be fixed
> there, IMO. That was already discussed in bug 693798.

I am aware that is why I did not consider them to be an issue.

On retrospective I should have placed a note saying exactly that.

> > [!] : SHOULD - SourceX / PatchY prefixed with %{name}.
> >         Patch0:         octave-struct-nostrip.patch
> > (octave-struct-nostrip.patch)
> 
> Changed to:
> Patch0:         %{name}-nostrip.patch
> 
> > Issues:
> > [!] : SHOULD - Patches link to upstream bugs/comments/lists or are otherwise
> > justified.
> 
> Ok, I will take the "otherwise justified" route.
> 
> Upstream made a deliberate choice to strip binaries upon installation.
> This however runs counter to our packaging guidelines (namely that we
> do not want useless debug packages). This patch just patches out the
> stripping on installation. This is, therefore, a fedora specific
> change, that needs not be reported to upstream (they will ignore it anyway
> as it runs counter to their decision)

My point here is that a single comment like:

# Avoid package stripping to have a useful debug package

over the patch is enough. You can rephrase it as you wish, but you get the
idea.

> > * You can remove octave from the BuildRequires field since octave-devel already
> > requires it (as it is usual for all *-devel to depend on the non-devel part)
> 
> Removed BR octave
> 
> > * The description part could be expanded a bit (I am aware that this is what
> > shows the project page).
> 
> I tried to come up with a slightly longer and more informative text.

Usually it is nice to have a link for the new sources. It is not required but
it is nice. :-)

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.