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

[Red Hat Bugzilla]

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