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

Sat Apr 23 15:42:52 UTC 2011

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

Hans de Goede <hdegoede at redhat.com> changed:

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

--- Comment #28 from Hans de Goede <hdegoede at redhat.com> 2011-04-23 11:42:50 EDT ---
Full review done, results below:

Good:
=====
- rpmlint checks return:
openCOLLADA.src: W: spelling-error Summary(en_US) Collada -> Collard
openCOLLADA.src: W: spelling-error %description -l en_US opensource -> open
source, open-source, outsource
openCOLLADA.src: W: spelling-error %description -l en_US validator ->
lavatorial
openCOLLADA.src: W: spelling-error %description -l en_US xml -> XML, ml, x ml
openCOLLADA.src:21: W: macro-in-comment %{name}
openCOLLADA.src:21: W: macro-in-comment %{AGE}
openCOLLADA.src: W: invalid-url Source0: openCOLLADA-svn838.tar.xz
openCOLLADA.x86_64: W: spelling-error Summary(en_US) Collada -> Collard
openCOLLADA.x86_64: W: spelling-error %description -l en_US opensource -> open
source, open-source, outsource
openCOLLADA.x86_64: W: spelling-error %description -l en_US validator ->
lavatorial
openCOLLADA.x86_64: W: spelling-error %description -l en_US xml -> XML, ml, x
ml
openCOLLADA.x86_64: W: incoherent-version-in-changelog 0.0-3
['0-3.svn838.fc15', '0-3.svn838']
openCOLLADA-devel.x86_64: W: no-documentation
  These can all be ignored.
- package meets naming guidelines
- package meets packaging guidelines
- license (MIT) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok

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.

  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

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

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

- please add -p to the invocation of cp for the doc files, to keep the original
  timestamps. Also why do you cp LICENSE, where as for AUTHORS you reference
  it with its full path ? This seems inconsistent.

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

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

- # copy CHANGES.txt
  install -p -m 0644 %{S:1} ./

  This belongs in %prep IMHO

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

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

- I don't like the way the changelog is formatted, here is a copy paste from 
  another review with similar issues:
  "Please put a blank line in the changelog before each data-name-version line
  (except for the first). And prefix / itemize items below the
  data-name-version line with "- ". IE change:
   * Sun Sep 26 2010 Elad Alfassa <el.il at doom.co.il> - 0.6-3
   Fix typo.
   * Sun Sep 26 2010 Elad Alfassa <el.il at doom.co.il> - 0.6-2
   Mark schemas file as config file
   * Sun Sep 26 2010 Elad Alfassa <el.il at doom.co.il> - 0.6-1
   More spec file fixes
   * Thu Sep 23 2010 Elad Alfassa <elad at macron.co.il> - 0.6-0
   Version update, some general spec file fixes.
   * Sun Jul 25 2010 Elad Alfassa <elad at macron.co.il> - 0.3-1
   initial build
  to:
   * Sun Sep 26 2010 Elad Alfassa <el.il at doom.co.il> - 0.6-3
   - Fix typo.

   * Sun Sep 26 2010 Elad Alfassa <el.il at doom.co.il> - 0.6-2
   - Mark schemas file as config file

   * Sun Sep 26 2010 Elad Alfassa <el.il at doom.co.il> - 0.6-1
   - More spec file fixes

   * Thu Sep 23 2010 Elad Alfassa <elad at macron.co.il> - 0.6-0
   - Version update, some general spec file fixes.

   * Sun Jul 25 2010 Elad Alfassa <elad at macron.co.il> - 0.3-1
   -initial build"

***

And then now the large things.

1) You're using a soname of lib*.so.0 for all the libs, given that this is all
c++ code and not with official releases from upstream, I think it would be good
to embed the svn snapshot nr into the soname. This means that any packages
using openCOLLADA will need to be rebuilt each time you rebase to a new svn
snapshot, but that seems the safest as I don't believe upstream will keep a
stable ABI. So instead of adding:
-Wl,--soname=libOpenCOLLADAFoo.so.0
As LINKFLAGS you should add (and install the files as):
-Wl,--soname=libOpenCOLLADAFoo-$(svn-version).so
(This is using libtool's convention for embedding an exact version into the
soname)

2) openCOLLADA-buildflags.patch is an unacceptable hack, the reason it is
unacceptable is that the RPM_OPT_FLAGS can differ per arch, so you cannot
hardcode them like this. Take a look at this spec file for another way to deal
with this:
http://pkgs.fedoraproject.org/gitweb/?p=rafkill.git;a=blob_plain;f=rafkill.spec;hb=refs/heads/f15/master
Note their might be an easier way ...

Also I wonder why exactly libOpenCOLLADASaxFrameworkLoader cannot build with
-O2 only -O0 ?

3) libUTF, libbuffer and libftoa are very generic names, I would like to see
these renamed to libopenCOLLADAFoo names.

4) And then there is the issue with the unresolved symbols I've run out of
steam (and time) for now to look into that (blergh scons), maybe I'll look into
it later today. If not feel free to do a new version in the mean time
addressing all the other concerns.

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