Bug 678955 - Review Request: opencsg - Library for Constructive Solid Geometry using OpenGL
Review Request: opencsg - Library for Constructive Solid Geometry using OpenGL
Status: CLOSED CANTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jerry James
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 678980
  Show dependency treegraph
 
Reported: 2011-02-20 23:41 EST by Jeff Moe (jebba)
Modified: 2012-10-12 22:33 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 825489 (view as bug list)
Environment:
Last Closed: 2011-12-29 13:31:53 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
loganjerry: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Jeff Moe (jebba) 2011-02-20 23:41:29 EST
Spec URL: http://repos.fedorapeople.org/repos/jebba/reprap/opencsg.spec

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

Description:
OpenCSG is a library that does image-based CSG rendering using OpenGL.

CSG is short for Constructive Solid Geometry and denotes an approach to model
complex 3D-shapes using simpler ones. I.e., two shapes can be combined by
taking the union of them, by intersecting them, or by subtracting one shape
of the other. The most basic shapes, which are not result of such a CSG
operation, are called primitives. Primitives must be solid, i.e., they must
have a clearly defined interior and exterior. By construction, a CSG shape is
also solid then.

Image-based CSG rendering (also z-buffer CSG rendering) is a term that denotes
algorithms for rendering CSG shapes without an explicit calculation of the
geometric boundary of a CSG shape. Such algorithms use frame-buffer settings
of the graphics hardware, e.g., the depth and stencil buffer, to compose CSG
shapes. OpenCSG implements a variety of those algorithms, namely the
Goldfeather algorithm and the SCS algorithm, both of them in several variants.
Comment 1 Ralf Corsepius 2011-02-21 06:01:30 EST
Lots of issues with this package:

rpmlint opencsg-*rpm
..
opencsg.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libopencsg.so.1.3.1 ['../lib']
opencsg.x86_64: E: library-without-ldconfig-postin /usr/lib64/libopencsg.so.1.3.1
opencsg.x86_64: E: library-without-ldconfig-postun /usr/lib64/libopencsg.so.1.3.1
opencsg.x86_64: E: non-standard-executable-perm /usr/lib64/libopencsg.so.1.3.1 0775L
opencsg-debuginfo.x86_64: E: debuginfo-without-sources

All of them are serious and need to be fixed.
Comment 2 Jeff Moe (jebba) 2011-02-21 14:21:23 EST
* Mon Feb 21 2011 Jeff Moe <moe@alephobjects.com> - 1.3.1-3
- Add ldconfig to %%post and %%postun
- Fix rpath
- Fix library permissions

-------------------------

I'm not sure what is causing the "debuginfo-without-sources". That may take a bit longer to fix (suggestions welcome).

Thanks!
Comment 3 Ralf Corsepius 2011-02-22 05:15:33 EST
(In reply to comment #2)
> I'm not sure what is causing the "debuginfo-without-sources". 
The origin of this is this package's makefiles not honouring RPM_OPT_FLAGS and them applying strip on binaries.

Both issues can be worked around by re-generating this packages's makefiles.
Comment 4 Jeff Moe (jebba) 2011-02-26 19:22:16 EST
I just blow out the Makefiles and qmake creates new ones which fixes both rpath and -debuginfo issues.

Also, it includes a glew 1.5.2 sub directory. I just remove this to make certain it uses Fedora's glew 1.5.7 instead.


* Sat Feb 26 2011 Jeff Moe <moe@alephobjects.com> - 1.3.1-4
- Regenerate Makefiles to fix rpath and debuginfo-without-sources
- Use Fedora's glew instead of included copy
- Remove tab from .spec

====================
Latest files:

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

Info on adding it as a repo here:
http://repos.fedorapeople.org/repos/jebba/reprap/readme.html
Comment 5 Jerry James 2011-03-04 16:23:26 EST
I'll review this package.
Comment 6 Jerry James 2011-03-04 16:54:50 EST
MUST items:
- rpmlint output:
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGenOcclusionQueriesNV
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXGetFBConfigAttribSGIX
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXQueryGLXPbufferSGIX
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewFramebufferTexture2DEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDepthBoundsEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDeleteOcclusionQueriesNV
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGenFramebuffersEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_NV_occlusion_query
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_ARB_depth_texture
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDeleteFramebuffers
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewRenderbufferStorageEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGetQueryObjectuivARB
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXChooseFBConfigSGIX
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_ARB_framebuffer_object
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewEndOcclusionQueryNV
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGenRenderbuffers
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_ARB_texture_env_dot3
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_ATI_texture_float
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_ARB_occlusion_query
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_NV_float_buffer
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_NV_texture_rectangle
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLXEW_SGIX_fbconfig
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXDestroyGLXPbufferSGIX
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewBindFramebufferEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewFramebufferRenderbufferEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXGetCurrentDisplay
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLXEW_SGIX_pbuffer
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDeleteQueriesARB
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLXEW_NV_float_buffer
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewFramebufferRenderbuffer
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewBindRenderbuffer
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewEndQueryARB
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGenRenderbuffersEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewBeginOcclusionQueryNV
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_EXT_packed_depth_stencil
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGetOcclusionQueryuivNV
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDeleteRenderbuffersEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXCreateGLXPbufferSGIX
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewRenderbufferStorage
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_EXT_depth_bounds_test
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDeleteFramebuffersEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewCheckFramebufferStatusEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewBeginQueryARB
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGenQueriesARB
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewXCreateContextWithConfigSGIX
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewDeleteRenderbuffers
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewBindFramebuffer
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewGenFramebuffers
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewBindRenderbufferEXT
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __GLEW_EXT_framebuffer_object
opencsg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libopencsg.so.1.3.1 __glewCheckFramebufferStatus
opencsg.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopencsg.so.1.3.1 /usr/lib64/libGLU.so.1
opencsg.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopencsg.so.1.3.1 /usr/lib64/libQtGui.so.4
opencsg.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopencsg.so.1.3.1 /usr/lib64/libQtCore.so.4
opencsg.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopencsg.so.1.3.1 /lib64/libm.so.6
opencsg-devel.x86_64: W: no-documentation
3 packages and 1 specfiles checked; 0 errors, 56 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.
- package name: OK
- spec file name: OK
- packaging guidelines:
  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.
- license: OK
- license field: the license file lists an explicit exception to GPLv2, so the license field should read "GPLv2 with exceptions".
- license file in %doc: no.  You must add license.txt to %doc.
- spec file in American English: OK
- spec file legible: OK
- source file matches upstream: OK, both have md5sum 6d2f91e72ebc0af55ea2a606059b1a79
- package builds: OK, checked on Fedora 14 x86_64
- appropriate use of ExcludeArch: N/A
- all build requirements in BuildRequires: OK
- locale handling: N/A
- ldconfig in %post and %postun: OK
- no copies of system libraries: OK
- relocatable package: N/A
- package owns all dirs it creates: N/A
- no duplicates in %files: OK
- proper permissions on files: OK
- consistent use of macros: OK, although I personally detest the %{__mkdir}, %{__mv}, and %{__rm} macros :-)
- code or permissible content: OK
- large documentation in -doc: N/A
- no runtime dependencies in %doc: OK
- header files in -devel: OK
- static libraries in -static: N/A
- .so files in -devel: OK
- -devel requires main package: OK, although you should consider using %{?_isa} for multilib safety, like so:
Requires: %{name}%{?_isa} = %{version}-%{release}
- No libtool archives: OK
- desktop file for GUI applications: N/A
- do not own already-owned files/dirs: OK
- filenames are valid UTF-8: OK

SHOULD items:
- query upstream to include license text: N/A
- description and summary translations: N/A
- package builds in mock: OK
- package builds on all supported arches: did not check
- package functions as described: reviewer has no easy way to check
- sane scriptlets: OK
- subpackages depend on main package: OK
- pkgconfig files: N/A
- file dependencies: N/A
- man pages: N/A

Note that the BuildRoot tag and the %clean section in the spec file are unnecessary on currently supported versions of Fedora.  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.
Comment 7 Jeff Moe (jebba) 2011-03-05 16:46:08 EST
* Sat Mar  5 2011 Jeff Moe <moe@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! :)
Comment 8 Jeff Moe (jebba) 2011-03-06 16:36:48 EST
I have built and added to the repo x86_64 builds for Fedora-15 dev branch. These didn't show any of the rpmlint you show above either. Are those rawhide builds?

The package does need `make -j1`, otherwise it tries to build examples before the library is done.
Comment 9 Jerry James 2011-03-07 13:47:49 EST
(In reply to comment #8)
> I have built and added to the repo x86_64 builds for Fedora-15 dev branch.
> These didn't show any of the rpmlint you show above either. Are those rawhide
> builds?

No, that was from a build on x86_64 Fedora 14.  I just tried building on Fedora 15 and got the same result.  Are you running rpmlint on the binary rpm files, or on the installed packages?  I don't believe rpmlint can do the undefined non-weak symbol checks on the former.

> The package does need `make -j1`, otherwise it tries to build examples before
> the library is done.

That can be fixed by adding an explicit dependency to the Makefile.  Put this line just after the %{_qt4_qmake} line in your spec file:

sed -i 's/^sub-example-make_default:/sub-example-make_default: sub-src-make_default/' Makefile

I built with -j5 on a quad-core machine with that line in place and with %{?_smp_mflags} added and didn't have a problem.  There is probably some way to tweak the project file so that the Makefile is generated with that dependency in the first place, but I know pretty much nothing about qmake.
Comment 10 Jerry James 2011-04-18 18:02:56 EDT
Jeff, do you intend to continue with this review?
Comment 11 Jerry James 2011-04-26 16:10:58 EDT
Jeff, do you intend to continue with this review?  Please respond.
Comment 12 Jeff Moe (jebba) 2011-04-27 16:18:25 EDT
I most certainly do, I hugely apologize for the delay. I am actively using the packages themselves, I just haven't done any upgrades for them. Thank you.
Comment 13 Jerry James 2011-04-28 15:12:08 EDT
That's fine, I just wanted to check whether I should keep this on my radar.  I'll wait patiently now. :-)
Comment 14 Jerry James 2011-11-21 12:49:42 EST
Months have passed.  Do you still intend to move this review forward?  If so, can you give me an expected time frame?  Thanks.
Comment 15 Jeff Moe (jebba) 2011-11-21 13:07:07 EST
The answer is that I don't know how to workaround the "undefined-non-weak-symbol" and that is what has blocked this package the whole time.  :(
Comment 16 Jerry James 2011-11-23 17:20:50 EST
Hmmm, I still don't understand qmake.  But there's another solution.  The original makefiles just need a few small tweaks to solve this problem.  Also, %{?_smp_mflags} made rpmbuild try to build the examples before the library was built, so I removed it.  I replaced your %prep and %build sections with these:

------------------------------------------------------------------------------
%prep
%setup -q -n OpenCSG-%{version}

# Use Fedora's glew
rm -rf glew/
sed -i 's/glew //' Makefile

# Get rid of an undesirable rpath, link with libGLEW, and use Fedora's CFLAGS
sed -e "s/,libopencsg\.so\.1.*/,libopencsg.so.1 -Wl,--as-needed/" \
    -e "s/-pipe -Wall -W -O2/$RPM_OPT_FLAGS/" \
    -e "s/-lGLU/-lGLU -lGLEW/" \
    -i src/Makefile
sed -i "s/-pipe -Wall -W -O2/$RPM_OPT_FLAGS/" example/Makefile

%build
make
------------------------------------------------------------------------------

and that seemed to work for me.  What do you think of that approach?
Comment 17 Jeff Moe (jebba) 2011-12-29 13:31:53 EST
That looks doable.

I am resigning from this though. I apologize for not getting this done. Feel free to take over. I'll close this out so a new one can get started. Sorry for your time, but it looks like you are making some progress on it. Thanks!
Comment 18 Greg Jurman 2012-05-27 00:42:07 EDT
I have submitted a new review for this package on Bug: #825489

Note You need to log in before you can comment on or make changes to this bug.