[Bug 665733] New: Review Request: Coin3 - High-level 3D visualization library

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: Coin3 - High-level 3D visualization library

https://bugzilla.redhat.com/show_bug.cgi?id=665733

Summary: Review Request: Coin3 - High-level 3D visualization
                    library
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora

Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/Coin3-3.1.3-1.fc15.src.rpm
Description: 
Coin is a 3D graphics library with an Application Programming Interface
based on the Open Inventor 2.1 API.

Note: This package applies "alternatives" to allow parallel installation of
Coin3-devel to Coin2-devel.

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

Richard Shaw  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

Richard Shaw  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #1 from Richard Shaw  2011-11-01 09:34:42 EDT ---
1. Are you planning on building for EL5? If not then the following can go away.

BuildRoot:, rm -rf $RPM_BUILD_ROOT from %install, %clean entirely, %defarrt
from %files

2. I had managed to get a build before getting your email response. Just to see
how minimal I could go I stripped out a bunch of stuff including:

# bogus permissions
find . ( -name '*.h' -o -name '*.cpp' -o -name '*.c' ) -a -executable -exec
chmod -x {} ;

rpmlint only complained about files in one directory so I replaced the above
with:

chmod -x src/xml/expat/*.c

It reduced the build time quite a bit. Nothing that would matter for koji, but
significant for experimental building on my own machine :)

3. Ok, in %configure I made quite a few changes. I went ahead and took
advantage of all the options that seemed appropriate. Here's my version, keep
in mind I removed all the alternative stuff for my personal build:

%configure \
        --includedir=%{_includedir}/Coin3 \
        --htmldir=%{_datadir}/doc/Coin3 \
        --disable-dependency-tracking \
        --enable-shared \
        --disable-dl-libbzip2 \
        --disable-dl-glu \
        --disable-dl-zlib \
        --disable-dl-freetype \
        --disable-dl-fontconfig \
        --disable-spidermonkey \
        --enable-man \
        --enable-html \
        --enable-3ds-import \
        CPPFLAGS=$(pkg-config --cflags freetype2)

The htmldir environment variable worked but since it offered a --htmldir option
I went ahead and used it. If this is truly documentation, should it not go in
/usr/share/doc/... and not /usr/share/...?

I also got rid of the coin_includedir and coin_htmldir. They're not used very
much and if you're not familiar with the package and you're looking in %files
you have to go back to the top of the spec to see how they're defined.

4. %files:

%doc AUTHORS COPYING README* LICENSE* THANKS FAQ*

- The README* also grabs readme's for windows and mac so change to:

%doc AUTHORS COPYING README README.UNIX LICENSE* THANKS FAQ*

Nit-picks:

5. I like %{buildroot} but the guidelines say just be consistent :)

6. There's a blank line in the middle of your BuildRequires. It looks like
you're separating the GL/X stuff from everything else. If you're not going to
put in a comment, it would be better to remove the blank line.

Other:

The alternatives doesn't bother me but you'll have to explain the i18n problems
to me...

Thanks,
Richard

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

Richard Shaw  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |458975

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #2 from Ralf Corsepius  2011-11-04 13:17:59 EDT ---
(In reply to comment #1)
> Are you planning on building for EL5?

Well, note the age of this submission (~ 1 year) - It predates this change in
Fedora - and the history of this package (Coin[12] packages's spec. This
rpm'specs origins are much older than Fedora - Can now be cleaned up ;)
> I had managed to get a build before getting your email response. Just to see
>
> how minimal I could go I stripped out a bunch of stuff including:
>
> # bogus permissions
> find . ( -name '*.h' -o -name '*.cpp' -o -name '*.c' ) -a -executable -exec
> chmod -x {} ;
>
> rpmlint only complained about files in one directory so I replaced the above
> with:

The files with bogus permission repeatedly changed in Coin's history. 
I once decided to use the "generic permission fix axe", because it turned
tiresome to chase permissions ;)
> Ok, in %configure I made quite a few changes. I went ahead and took
>
> advantage of all the options that seemed appropriate. Here's my version, keep
> in mind I removed all the alternative stuff for my personal build:
>
> %configure \
>         --includedir=%{_includedir}/Coin3 \
>         --htmldir=%{_datadir}/doc/Coin3 \
>         --disable-dependency-tracking \
>         --enable-shared \
>         --disable-dl-libbzip2 \
>         --disable-dl-glu \
>         --disable-dl-zlib \
>         --disable-dl-freetype \
>         --disable-dl-fontconfig \
>         --disable-spidermonkey \
>         --enable-man \
>         --enable-html \
>         --enable-3ds-import \
>         CPPFLAGS=$(pkg-config --cflags freetype2)
>
> The htmldir environment variable worked but since it offered a --htmldir option
> I went ahead and used it. If this is truly documentation, should it not go in
> /usr/share/doc/... and not /usr/share/...?

I don't recall the details - Could be an artefact from Coin1 and Coin2 or a
side effect of coin-config's habit to hard-code paths.

To be checked.
> I also got rid of the coin_includedir and coin_htmldir.

Your preference - mine is different, because these reduce size of the diffs
between Coin1, Coin2 and Coin3 and eases comparing the specs.
> %files:
>
> %doc AUTHORS COPYING README* LICENSE* THANKS FAQ*
> The README* also grabs readme's for windows and mac so change to:
>
> %doc AUTHORS COPYING README README.UNIX LICENSE* THANKS FAQ*

OK, I'll look into this.
> Nit-picks:
> I like %{buildroot} but the guidelines say just be consistent :)

I hate %{buildroot} and am not using it anywhere inside of my specs ;)
> There's a blank line in the middle of your BuildRequires. It looks like
>
> you're separating the GL/X stuff from everything else. If you're not going to
> put in a comment, it would be better to remove the blank line.

OK, will check.
> Other:
>
> The alternatives doesn't bother me but you'll have to explain the i18n problems
> to me...

The source code is written using some ISO8859 variant as being used in Norway.

This causes doxygen to generate ISO8859 encoded docs and causes doxygen to get
confused on some characters. IIRC, there also where some doxygen constructs
inside of doxygen comments in the source-code, which newer doxygen handles
differently than older doxygens. - I'll try to check.

An updated package to come sometime next week.

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #3 from Richard Shaw  2011-11-05 10:01:57 EDT ---
Ok, everything looks good otherwise, but if you don't mind, can you explain why
we need a Coin2 and Coin3?

Is there software in Fedora that relies on Coin2 still? In my pursuit to build
FreeCAD I was working on Pivy which wanted Coin3, so I ended up building it
from your SRPM and also rebuilding SoQt and SIMVoleon against it successfully.

Thanks,
Richard

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #4 from Ralf Corsepius  2011-11-07 22:47:27 EST ---
(In reply to comment #2)
> (In reply to comment #1)
> > Are you planning on building for EL5?
>
> Well, note the age of this submission (~ 1 year) - It predates this change in
> Fedora - and the history of this package (Coin[12] packages's spec. This
> rpm'specs origins are much older than Fedora - Can now be cleaned up ;)

Done.
> > Ok, in %configure I made quite a few changes. I went ahead and took
> >
> > advantage of all the options that seemed appropriate. Here's my version, keep
> > in mind I removed all the alternative stuff for my personal build:
> >
> > %configure \
> >         --includedir=%{_includedir}/Coin3 \
> >         --htmldir=%{_datadir}/doc/Coin3 \
> >         --disable-dependency-tracking \
> >         --enable-shared \
> >         --disable-dl-libbzip2 \
> >         --disable-dl-glu \
> >         --disable-dl-zlib \
> >         --disable-dl-freetype \
> >         --disable-dl-fontconfig \
> >         --disable-spidermonkey \
> >         --enable-man \
> >         --enable-html \
> >         --enable-3ds-import \
> >         CPPFLAGS=$(pkg-config --cflags freetype2)
> >
> > The htmldir environment variable worked but since it offered a --htmldir option

The configure script treats --htmldir equivalently to passing 
htmldir to configure. As htmldir also works for Coin < 3, while --htmldir
doesn't, I'll continue using "htmldir=...".
> > If this is truly documentation, should it not go in
> > /usr/share/doc/... and not /usr/share/...?
>
> I don't recall the details - Could be an artefact from Coin1 and Coin2 or a
> side effect of coin-config's habit to hard-code paths.
> To be checked.

It's not an artefact. Coin spans up a documentation system of its own for Coin
and its frontends (SoQt, SoGtk, SoWin, etc.). I.e. it expects a common
documentation root directory to install its frontends' html documentation into.

This doesn't fit into rpm's /usr/share/doc usage.
> > %files:
> >
> > %doc AUTHORS COPYING README* LICENSE* THANKS FAQ*
> > The README* also grabs readme's for windows and mac so change to:
> >
> > %doc AUTHORS COPYING README README.UNIX LICENSE* THANKS FAQ*
>
> OK, I'll look into this.

Done. All README.*'s are gone (Comprising README.UNIX, which only contains an
"install howto")
> An updated package to come sometime next week.

Here it is:

http://corsepiu.fedorapeople.org/packages/Coin3.spec
 http://corsepiu.fedorapeople.org/packages/Coin3-3.1.3-2.fc17.src.rpm

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #5 from Richard Shaw  2011-11-08 09:37:39 EST ---
Ralf,

Can you respond to comment #3? I did a quick search:

# repoquery --whatrequires Coin2
Coin2-devel-0:2.5.0-10.fc15.i686
Coin2-devel-0:2.5.0-10.fc15.x86_64
SIMVoleon-0:2.0.1-11.fc15.i686
SIMVoleon-0:2.0.1-11.fc15.x86_64
SoQt-0:1.5.0-2.fc15.i686
SoQt-0:1.5.0-2.fc15.x86_64

And it appears that only SoQt and SIMVoleon require Coin2 and I've successfully
built them against Coin3.

Is there a reason we can just make this package "Coin" and obsolete Coin2?

Thanks,
Richard

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #6 from Richard Shaw  2011-12-02 10:02:58 EST ---
Ping?

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #7 from Ralf Corsepius  2011-12-02 10:29:33 EST ---
(In reply to comment #6)
> Ping?

Why are you pinging? The packages from comment #4 are current.

And no, I do not want to rename this package into Coin and want to keep
Coin2/Coin3 in parallel, because they are not 100% compatible (as you
experienced with Pivy) and because of licensing issues (Coin1 was LGPL,
Coin2/Coin3 are GPLv2 (Note GPLv2, not GPLv2+; The next Coin likely will be
BSD).

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #8 from Richard Shaw  2011-12-02 12:15:35 EST ---
(In reply to comment #7)
> (In reply to comment #6)
> > Ping?
>
> Why are you pinging? The packages from comment #4 are current.

Because it wasn't a rhetorical question and because you didn't answer the
question until now :)
> And no, I do not want to rename this package into Coin and want to keep
> Coin2/Coin3 in parallel, because they are not 100% compatible (as you
> experienced with Pivy) and because of licensing issues (Coin1 was LGPL,
> Coin2/Coin3 are GPLv2 (Note GPLv2, not GPLv2+; The next Coin likely will be
> BSD).

That's all I was looking for was an explanation.

I build packages on F15 x86_64 which have the following rpmlint output:

$ rpmlint *.rpm
Coin3.src:57: W: unversioned-explicit-provides pkgconfig(Coin)
Coin3.x86_64: W: shared-lib-calls-exit /usr/lib64/libCoin.so.60.1.3
exit@GLIBC_2.2.5
Coin3.x86_64: E: incorrect-fsf-address /usr/share/doc/Coin3-3.1.3/LICENSE.GPL
Coin3-devel.x86_64: W: read-error /usr/lib64/pkgconfig/Coin.pc [Errno 2] No
such file or directory:
'/tmp/rpmlint.Coin3-devel-3.1.3-2.fc15.x86_64.rpm.oOzNpI//usr/lib64/pkgconfig/Coin.pc'
Coin3-devel.x86_64: W: dangerous-command-in-%post rm
4 packages and 0 specfiles checked; 1 errors, 4 warnings.

I think most of this can be ignored either because it's an artifact of
alternatives (unversioned-explicit-provides, read-error,
dangerous-command-in-%post) or upstream is currently unlikely to to care
(shared-lib-calls-exit, incorrect-fsf-address).

The only change I feel pretty strongly about is creating a separate doc
sub-package. Currently the devel package is 55MB, most of which is
documentation and  the most common use is more likely to be from someone, like
myself, that just needs it as a build requirement and have no intention of
doing any direct development using Coin3.

Full review to follow.

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #9 from Richard Shaw  2011-12-02 12:49:51 EST ---
+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment.
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: GPLv2+
[+] license field matches the actual license: Actual source is GPLv2.
[+] license file is included in %doc: LICENSE.GPL
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum matches (1538682f8d92cdf03e845c786879fbea)
[+] package builds on at least one primary arch: Tested F15 x86_64
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[+] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[=] large documentation in -doc
[+] no runtime dependencies in %doc
[+] header files in -devel
[N] static libraries in -static
[+] .so in -devel
[?] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[+] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches: Tested x86_64
[?] package functions as described: Used as BR for two other programs.
[+] sane scriptlets
[+] subpackages require the main package
[+] placement of pkgconfig files
[N] file dependencies versus package dependencies
[+] package contains man pages for binaries/scripts

Only 3 items remain:

1. Spec file says GPLv2+ not GPLv2.
2. The question about a doc package
3. The Require of the main package from the devel package is not arch specific
but the -devel package is not "noarch".

In %package devel change:
Requires: %{name} = %{version}-%{release}
to
Requires: %{name}%{?_isa} = %{version}-%{release}

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #10 from Richard Shaw  2011-12-27 11:39:52 EST ---
Ralf,

I just wanted to check in with you. I've gotten OCE accepted at RPM Fusion and
will soon submit smesh for Fedora so Coin3 is my last major requirement for
FreeCAD.

Thanks,
Richard

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #11 from Richard Shaw  2012-01-30 16:12:48 EST ---
FYI,

SMESH has now been accepted at RPM Fusion.

Thanks,
Richard

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #12 from Richard Shaw  2012-02-27 16:16:32 EST ---
Created attachment 566143
  --> https://bugzilla.redhat.com/attachment.cgi?id=566143
Updated Coin3 spec file

Ralf,

I've fixed the 3 concerns I had and I'm attaching the new spec file for your
review.

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

--- Comment #13 from Richard Shaw  2012-04-06 17:20:11 EDT ---
Ralf,

Can I assume that you're ignoring me since you're obviously active on the
mailing list but have not responded on this review request in over 4 months?

[Bug 665733] Review Request: Coin3 - High-level 3D visualization library

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

Richard Shaw  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|458975(Pivy)                |