[Bug 678955] Review Request: opencsg - Library for Constructive Solid Geometry using OpenGL

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

--- Comment #7 from Jeff Moe (jebba) <moe at blagblagblag.org> 2011-03-05 16:46:08 EST ---
* Sat Mar  5 2011 Jeff Moe <moe at alephobjects.com> - 1.3.1-5
- Enable parallel compiling.
- Corrected license to GPLv2 with exceptions.
- Improved -devel Requires for multilib.
- Remove BuildRoot tag.
- Remove %%clean section
- Change mv and mkdir to direct commands, not macros.

=============================================================

http://repos.fedorapeople.org/repos/jebba/reprap/opencsg.spec

http://repos.fedorapeople.org/repos/jebba/reprap/14/SRPMS/opencsg-1.3.1-5.fc14.src.rpm

=============================================================

(In reply to comment #6)
> MUST items:
> - rpmlint output:
> opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1
> __glewGenOcclusionQueriesNV
> ...

I don't get this when I run rpmlint:
$ rpmlint opencsg-*1.3.1-5*
opencsg-devel.i386: W: no-documentation
opencsg-devel.x86_64: W: no-documentation
7 packages and 0 specfiles checked; 0 errors, 2 warnings.

> This means that you need to change the link command for libopencsg.so so that
> -lglew IS included, and -lGLU, -lQtGui, -lQtCore, and -lm are NOT included.

OK, I will look into that.

>   Why are you passing -j1 to make?  If parallel make does not work (see
> https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make), then please
> include a comment stating so.

For awhile I thought it may have been breaking the build, but it appears to
have been something else, so it is now SMP.

> - license field: the license file lists an explicit exception to GPLv2, so the
> license field should read "GPLv2 with exceptions".

Done.

> - consistent use of macros: OK, although I personally detest the %{__mkdir},
> %{__mv}, and %{__rm} macros :-)

I changed them to just plain mkdir/mv. I just noticed now I missed rm. I fixed
that for the next push.

> - -devel requires main package: OK, although you should consider using %{?_isa}
> for multilib safety, like so:
> Requires: %{name}%{?_isa} = %{version}-%{release}

Done.

> - package functions as described: reviewer has no easy way to check

This is being built as a dependency of OpenSCAD, which is also in my repo, btw.

> Note that the BuildRoot tag and the %clean section in the spec file are
> unnecessary on currently supported versions of Fedora.

Removed.

>  Also, the HTML files
> you put in %doc refer to files in the img directory; please add img to %doc. 
> There may be value in including (parts of) the example directory in %doc as
> well.

Ay, missed this one on this pass. I'll take a look at it next round.

Thanks for your review! :)

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