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@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review? --- Comment #28 from Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> - 0.6-3 Fix typo. * Sun Sep 26 2010 Elad Alfassa <el.il@xxxxxxxxxx> - 0.6-2 Mark schemas file as config file * Sun Sep 26 2010 Elad Alfassa <el.il@xxxxxxxxxx> - 0.6-1 More spec file fixes * Thu Sep 23 2010 Elad Alfassa <elad@xxxxxxxxxxxx> - 0.6-0 Version update, some general spec file fixes. * Sun Jul 25 2010 Elad Alfassa <elad@xxxxxxxxxxxx> - 0.3-1 initial build to: * Sun Sep 26 2010 Elad Alfassa <el.il@xxxxxxxxxx> - 0.6-3 - Fix typo. * Sun Sep 26 2010 Elad Alfassa <el.il@xxxxxxxxxx> - 0.6-2 - Mark schemas file as config file * Sun Sep 26 2010 Elad Alfassa <el.il@xxxxxxxxxx> - 0.6-1 - More spec file fixes * Thu Sep 23 2010 Elad Alfassa <elad@xxxxxxxxxxxx> - 0.6-0 - Version update, some general spec file fixes. * Sun Jul 25 2010 Elad Alfassa <elad@xxxxxxxxxxxx> - 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review