[Bug 694287] Review Request: openCOLLADA - 3D import and export libraries

[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=694287

--- Comment #33 from Richard Shaw <hobbes1069 at gmail.com> 2011-04-26 16:09:25 EDT ---
(In reply to comment #28)
> Needs work:
> ===========
> 
> Ok, I'm going to divide this into 2 lists, one with small things and one with
> large things. First the small things:
> - rpmlint checks return:
>   openCOLLADA.src:108: W: mixed-use-of-spaces-and-tabs (spaces: line 9, tab:
> line 108)
>   Please don't use tabs in spec files.

I couldn't find the tab but I ran across 'expand' which seemed to work quite
nicely to fix the problem.


>   openCOLLADA.x86_64: W: file-not-utf8 /usr/share/doc/openCOLLADA-0/AUTHORS
>   You will need to convert this to utf8, add the following to %prep:
>   iconv -f ISO_8859-1 -t utf-8 COLLADAStreamWriter/AUTHORS > \
>     COLLADAStreamWriter/AUTHORS.tmp
>   touch -r COLLADAStreamWriter/AUTHORS COLLADAStreamWriter/AUTHORS.tmp
>   mv COLLADAStreamWriter/AUTHORS.tmp COLLADAStreamWriter/AUTHORS

Done.


> - please drop the following comment (it is the same as Source0, so I see no
>   use for it):
>   #Source0:        %{name}-svn%{AGE}.tar.xz

Already gone. Sorry, I left that in there when I was playing with different
compression (gz, xz, bz2)


> - please drop the following (already commented) command from the spec file:
>   #strip --strip-debug *.so.*
>   Stripping should *never* be done inside the spec file.

That's left over from Suse. It was always commented out for me. Fixed.


> - please add -p to the invocation of cp for the doc files, to keep the original

Fixed.


>   timestamps. Also why do you cp LICENSE, where as for AUTHORS you reference
>   it with its full path ? This seems inconsistent.

Not sure, unless Dave just wanted AUTHORS in the subdir and LICENSE in the
root. I don't see why AUTHORS couldn't be moved to the install root (is that
the right terminology?)


> - please add -k to the invocation of dos2unix to keep the original timestamps
>   on the files

Done.


> - Group: Applications/Productivity
>   Is not really appropriate, please use:
>   Group: System Environment/Libraries
>   For the main package instead.

Apparently Suse has A LOT MORE groups than we do. I guessed on this one but
that was before I really knew what I was doing :)


> - # copy CHANGES.txt
>   install -p -m 0644 %{S:1} ./
>   This belongs in %prep IMHO

Done.


> - Add -p to invocation of cp for installation of header files to preserve the
>   timestamps on the files

This is already recursive. Would it be appropriate to just use -a here?


> - devel does not requires base package n-v-r 
>   Please change the:
>   Requires:       %{name} = %{version}
>   For the -devel sub-package into:
>   Requires:       %{name} = %{version}-%{release}

Done.


> - I don't like the way the changelog is formatted, here is a copy paste from 
>   another review with similar issues:

Done.

Richard

-- 
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.