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

(In reply to comment #1)
> 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 ;)

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

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 ;)

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

> 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*
OK, I'll look into this.
 
> Nit-picks:
> 
> 5. I like %{buildroot} but the guidelines say just be consistent :)
I hate %{buildroot} and am not using it anywhere inside of my specs ;)
 
> 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.
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.

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

(In reply to comment #2)
> (In reply to comment #1)
> > 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.

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

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.

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

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

Ping?

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

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

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

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

FYI,

SMESH has now been accepted at RPM Fusion.

Thanks,
Richard

Created attachment 566143 [details]
Updated Coin3 spec file

Ralf,

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

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?

I have managed to build Pivy with Coin 2 so I no longer need this and you don't appear to either.

(In reply to comment #13) 
> 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?

No, I initially did not respond, because I do not agree with our review and your proposal.

Well it's supposed to be a back and forth discussion. I haven't made any demands. I simply made arguments for certain things and was expecting you to do the same, otherwise what's the point of peer review? 

I just don't see how ignoring me benefited either one of us or the review and only caused unnecessary frustration and delay.

(In reply to comment #16)
> I just don't see how ignoring me benefited either one of us or the review and
> only caused unnecessary frustration and delay.
Sure, but there are situations, when people turn away and leave others alone. In this case because I had wanted to avoid silly and non-helpfule disputes.

Now, this review request was closed, with me noticing too late, because I was busy with other subjects and had not looked into the mails related to this review.

This certainly is an incident, which improves my moot towards Fedora in general ;)

Hi Ralf, if you're not interested in having this package reviewed, I'd like to walk the package through review myself and become the maintainer.  Any objections?  Thanks-

    John

(In reply to John Morris from comment #18)
> Hi Ralf, if you're not interested in having this package reviewed,
I am interested in having this package reviewed.

Hi Ralf, thanks, very glad!  I'll look at it this weekend.  Thanks-

    John

Builds are choking on freetype.cpp on fc19 and fc20 (el6 and fc18 ok):

freetype.cpp:1394:1: warning: unused parameter 'vertex_data' [-Wunused-parameter]
 flwft_combineCallback(GLdouble coords[3], GLvoid * vertex_data, GLfloat weight[4], int **dataOut)
 ^
freetype.cpp:1394:1: warning: unused parameter 'weight' [-Wunused-parameter]
make[4]: Leaving directory `/builddir/build/BUILD/Coin-3.1.3/src/fonts'
make[4]: *** [freetype.lo] Error 1
make[3]: Leaving directory `/builddir/build/BUILD/Coin-3.1.3/src'
make[3]: *** [all-recursive] Error 1
make[2]: Leaving directory `/builddir/build/BUILD/Coin-3.1.3/src'
make[2]: *** [all] Error 2
make[1]: Leaving directory `/builddir/build/BUILD/Coin-3.1.3'
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

http://koji.fedoraproject.org/koji/taskinfo?taskID=5782168
http://koji.fedoraproject.org/koji/taskinfo?taskID=5782181

I was unable to complete a build using 'fedora-review'.

I had fixed this build breakdown back in Spring, but seem to have missed to update the package on fedorapeople.org ;)

Anyway, here's a another update:
Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/Coin3-3.1.3-4.fc20.src.rpm

Scratch-build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5835545

# rpmlint results/Coin3-*x86_64*
Coin3.x86_64: W: shared-lib-calls-exit /usr/lib64/libCoin.so.60.1.3 exit.5
Coin3.x86_64: E: incorrect-fsf-address /usr/share/doc/Coin3/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-4.fc20.x86_64.rpm.0V_Ey7//usr/lib64/pkgconfig/Coin.pc'
Coin3-devel.x86_64: W: manual-page-warning /usr/share/man/man3/SoCamera.3coin3.gz 458: warning: macro `Jitter'' not defined
Coin3-devel.x86_64: W: dangerous-command-in-%post rm
3 packages and 0 specfiles checked; 1 errors, 4 warnings.

All these are bogus rsp. neglible.

I just want to know if I can add copyright header in the spec?

I want to add such things to all packages I submitted.

Ralf, do you still want to pursue this review? The lack of this package in Fedora is becoming a serious problem.

(In reply to Ralf Corsepius from comment #22)
> I had fixed this build breakdown back in Spring, but seem to have missed to
> update the package on fedorapeople.org ;)
> 
> Anyway, here's a another update:
> Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
> SRPM URL:
> http://corsepiu.fedorapeople.org/packages/Coin3-3.1.3-4.fc20.src.rpm
> 
> Scratch-build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5835545
> 
> # rpmlint results/Coin3-*x86_64*
> Coin3.x86_64: W: shared-lib-calls-exit /usr/lib64/libCoin.so.60.1.3
> exit.5

Should be reported upstream, but no, not a blocker.


> Coin3.x86_64: E: incorrect-fsf-address /usr/share/doc/Coin3/LICENSE.GPL

Same


> 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-4.fc20.x86_64.rpm.0V_Ey7//usr/lib64/
> pkgconfig/Coin.pc'

Weird...

If you want to move forward I suggest we do a re-review just to be safe as the packaging guidelines have changed quite a bit.

(In reply to Richard Shaw from comment #24)
> Ralf, do you still want to pursue this review?
Thanks for your interest - Yes, I am. I am still using Coin3 and would be interested to see Coin3 in Fedora.

I am also aware, the packaging I have on fedorapeople.org has issues with newer Fedoras, but I due to the disinterest Fedora had exposed, I haven't updated these packages and am using local packages, instead.

Likely I'll revisit my packaging and publish updated versions later this week.

(In reply to Ralf Corsepius from comment #25)
> Likely I'll revisit my packaging and publish updated versions later this
> week.
Sorry, took a little longer than a week to get back to it:

New versions:
Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/Coin3-3.1.5-1.fc23.src.rpm

(In reply to Ralf Corsepius from comment #26)
> (In reply to Ralf Corsepius from comment #25)
> > Likely I'll revisit my packaging and publish updated versions later this
> > week.
> Sorry, took a little longer than a week to get back to it:
> 
> New versions:
> Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
> SRPM URL:
> http://corsepiu.fedorapeople.org/packages/Coin3-3.1.5-1.fc23.src.rpm
Oops, typo:

Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/Coin3-3.1.3-5.fc23.src.rpm

Ok, first a couple of non-blockers from a spec review:

1. Group tags have not been required for some time.
2. Fedora 20 is the oldest supported release so the conditional really isn't needed, but if you're supporting older Fedora installations locally then let's keep it.

One blocker from a spec review:

Recently the use of the %license macro for marking what part of the documentation is the license became part of the packaging guidelines.

Detailed review:

There are some mixed licenses found by licensecheck but most of them don't look like they make it into the library or binaries so can be ignored.

One I'm unsure of is the bundled boost headers. The readme in the include directory indicated they're used for the testsuite and don't make it into the binaries but mentions they could at a future date, however, grepping through the sources I'm not so sure that hasn't already happened:

$ grep -r boost . | grep include
./engines/SoHeightMapToNormalMap.cpp:#include <boost/scoped_array.hpp>
./profiler/SoProfilingReportGenerator.cpp:#include <boost/scoped_array.hpp>
./profiler/SoScrollingGraphKit.cpp:#include <boost/scoped_ptr.hpp>
./profiler/SoScrollingGraphKit.cpp:#include <boost/scoped_array.hpp>
./profiler/SoScrollingGraphKit.cpp:#include <boost/intrusive_ptr.hpp>
./profiler/SoProfilerVisualizeKit.cpp:#include <boost/scoped_ptr.hpp>
./profiler/SoProfilerTopKit.cpp:#include <boost/intrusive_ptr.hpp>
./profiler/SoNodeVisualize.cpp:#include <boost/scoped_ptr.hpp>
./scxml/SoScXMLStateMachine.cpp:#include <boost/scoped_ptr.hpp>
./scxml/SoScXMLStateMachine.cpp:#include <boost/intrusive_ptr.hpp>
./scxml/ScXMLStateMachine.cpp:#include <boost/scoped_ptr.hpp>
./scxml/ScXMLState.cpp:#include <boost/scoped_ptr.hpp>
./scxml/ScXMLHistory.cpp:#include <boost/scoped_ptr.hpp>
./scxml/ScXMLInitial.cpp:#include <boost/scoped_ptr.hpp>
./coindefs.h:#include <boost/detail/workaround.hpp> /* For BOOST_WORKAROUND */
./glue/dl.cpp:  Boost. The boost headers can be found under \c include/ in the Coin
./actions/SoGLRenderAction.cpp:#include <boost/scoped_ptr.hpp>
./actions/SoGLRenderAction.cpp:#include <boost/scoped_array.hpp>
./xml/document.cpp:#include <boost/scoped_array.hpp>
./xml/document.cpp:#include <boost/scoped_array.hpp>
./nodes/SoVertexAttribute.cpp:#include <boost/scoped_ptr.hpp>
./navigation/SoScXMLRotateUtils.cpp:#include <boost/scoped_ptr.hpp>
./navigation/SoScXMLRotateUtils.cpp:#include <boost/intrusive_ptr.hpp>
./navigation/SoScXMLPanUtils.cpp://#include <boost/intrusive_ptr.hpp>
./navigation/SoScXMLSpinUtils.h:#include <boost/intrusive_ptr.hpp>

Not all of those look like they're related to the testsuite but you're far more familiar with the package.

Alternatively it looks like all of the include uses system include formatting instead of pointing specifically to the bundled ones so I wonder if we could add a BuildRequires for boost and rm -rf the bundled headers?

FWIW, I was taking a break from something else I was working on and decided to try adding boost-devel and rm -rf include/boost and was able to build packages for rawhide. 

I also watched the build since rawhide is using gcc 5.0.0 now and I didn't see a bunch of warnings so that's good.

The only other thing fedora-review complained about directly was the size of the documentation in the -devel package. It should (but is not required to) be moved to a -doc noarch package.

(In reply to Richard Shaw from comment #28)
> Ok, first a couple of non-blockers from a spec review:
> 
> 1. Group tags have not been required for some time.
I prefer keeping Group tags.

> 2. Fedora 20 is the oldest supported release so the conditional really isn't
> needed, but if you're supporting older Fedora installations locally then
> let's keep it.
The freetype2 f20 conditional _is_ required to be able to support fc20:
...
%if 0%{?fedora} > 20
# Incompatibility:  
# Fedora > 20 has /usr/include/freetype2/
# Fedora <= 20 has /usr/include/freetype2/freetype
sed -i -e 's,freetype/,freetype2/,' src/glue/freetype.{h,cpp}
%endif
...
The package does not build on fc20 without it, because the freetype header location has changed in Fedora.
 
> One blocker from a spec review:
> 
> Recently the use of the %license macro for marking what part of the
> documentation is the license became part of the packaging guidelines.
Well, I repeatedly pronounced my opinion on this topic in hopefully non-misunderstandable ways, so I am not going to repeat them here, again.

> Detailed review:
> 
> There are some mixed licenses found by licensecheck but most of them don't
> look like they make it into the library or binaries so can be ignored.
Like many other packages, Coin uses the GPLv2 as an umbrella-license. The package as a whole currently is GPLv2'ed, even though it contains files under other licenses.

> One I'm unsure of is the bundled boost headers. The readme in the include
> directory indicated they're used for the testsuite and don't make it into
> the binaries but mentions they could at a future date, however, grepping
> through the sources I'm not so sure that hasn't already happened:
Coin3 uses some boost-templates (headers) internally, but the installed headers don't carry any deps on the boost-headers nor do the coin libraries carry and reference to the boost-runtime libs.

> Alternatively it looks like all of the include uses system include
> formatting instead of pointing specifically to the bundled ones so I wonder
> if we could add a BuildRequires for boost and rm -rf the bundled headers?
Well, it's a double-edged sword, with pros and cons, each. Buying-in upstream boost bug-fixes, vs. upstream boost introducing incompatibilities and breaking the package.

Anyway, with current boost, removing the bundled boost headers doesn't seem to cause any immediate malfunction or break down.

(In reply to Ralf Corsepius from comment #30)
> (In reply to Richard Shaw from comment #28)
> > 2. Fedora 20 is the oldest supported release so the conditional really isn't
> > needed, but if you're supporting older Fedora installations locally then
> > let's keep it.
> The freetype2 f20 conditional _is_ required to be able to support fc20:
> ...
> %if 0%{?fedora} > 20
> # Incompatibility:  
> # Fedora > 20 has /usr/include/freetype2/
> # Fedora <= 20 has /usr/include/freetype2/freetype
> sed -i -e 's,freetype/,freetype2/,' src/glue/freetype.{h,cpp}
> %endif
> ...
> The package does not build on fc20 without it, because the freetype header
> location has changed in Fedora.

Whoops, I read that as >= for some reason. 

  
> Anyway, with current boost, removing the bundled boost headers doesn't seem
> to cause any immediate malfunction or break down.

Ok sounds good, so if upstream does start using boost more extensively we stay out of trouble as far as bundling goes we just have to make sure a future boost update doesn't break things.

I wasn't able to build earlier today, seemingly because mirrormanager once more wasn't able to sync the Fedora 22 and rawhide repos (yum in mock was sending me around the globe without being able to find a functional mirror).
Meanwhile, at least some mirrors seem to have synced.

New, updated version (*-6):
Spec URL: http://corsepiu.fedorapeople.org/packages/Coin3.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/Coin3-3.1.3-6.fc23.src.rpm

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: Coin3-3.1.3-6.fc22.x86_64.rpm
          Coin3-devel-3.1.3-6.fc22.x86_64.rpm
          Coin3-3.1.3-6.fc22.src.rpm
Coin3.x86_64: W: shared-lib-calls-exit /usr/lib64/libCoin.so.60.1.3 exit.5
Coin3.x86_64: E: incorrect-fsf-address /usr/share/licenses/Coin3/LICENSE.GPL
Coin3-devel.x86_64: E: useless-provides pkgconfig(Coin)
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-6.fc22.x86_64.rpm.LJrRz4/usr/lib64/pkgconfig/Coin.pc'
Coin3-devel.x86_64: W: only-non-binary-in-usr-lib
Coin3-devel.x86_64: W: dangerous-command-in-%post rm
Coin3.src:73: W: unversioned-explicit-provides pkgconfig(Coin)
Coin3.src: W: invalid-url Source0: https://bitbucket.org/Coin3D/coin/downloads/Coin-3.1.3.tar.gz HTTP Error 403: Forbidden
3 packages and 0 specfiles checked; 2 errors, 6 warnings.


Requires
--------
Coin3 (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libGL.so.1()(64bit)
    libGLU.so.1()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libbz2.so.1()(64bit)
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libz.so.1()(64bit)
    rtld(GNU_HASH)

Coin3-devel (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/pkg-config
    /usr/sbin/alternatives
    Coin3(x86-64)
    bzip2-devel
    fontconfig-devel
    freetype-devel
    libGLU-devel
    pkgconfig
    zlib-devel



Provides
--------
Coin3:
    Coin3
    Coin3(x86-64)
    libCoin.so.60()(64bit)

Coin3-devel:
    Coin3-devel
    Coin3-devel(x86-64)
    pkgconfig(Coin)
    pkgconfig(Coin3)



Source checksums
----------------
https://bitbucket.org/Coin3D/coin/downloads/Coin-3.1.3.tar.gz :
  CHECKSUM(SHA256) this package     : 583478c581317862aa03a19f14c527c3888478a06284b9a46a0155fa5886d417
  CHECKSUM(SHA256) upstream package : 583478c581317862aa03a19f14c527c3888478a06284b9a46a0155fa5886d417


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/bin/fedora-review -b 665733 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

It would be nice to move the documentation to a noarch package but not a blocker so:
*** APPROVED ***

Thank you very much for the review, Richard! 

... 4+ years after initial submission ... 

New Package SCM Request
=======================
Package Name: Coin3
Short Description: High-level 3D visualization librar
Upstream URL: https://bitbucket.org/Coin3D/coin/wiki/Home
Owners: corsepiu
Branches: f22 f21 f20
InitialCC:

Arg, cut'n'pasto ...

New Package SCM Request
=======================
Package Name: Coin3
Short Description: High-level 3D visualization library
Upstream URL: https://bitbucket.org/Coin3D/coin/wiki/Home
Owners: corsepiu
Branches: f22 f21 f20
InitialCC:

Coin3-3.1.3-6.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/Coin3-3.1.3-6.fc20

Coin3-3.1.3-6.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/Coin3-3.1.3-6.fc21

Coin3-3.1.3-6.fc20 has been pushed to the Fedora 20 testing repository.

Coin3-3.1.3-7.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/Coin3-3.1.3-7.fc20

Coin3-3.1.3-7.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/Coin3-3.1.3-7.fc21

Coin3-3.1.3-7.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/Coin3-3.1.3-7.fc22

Coin3-3.1.3-7.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.

Coin3-3.1.3-7.fc22 has been pushed to the Fedora 22 stable repository.  If problems still persist, please make note of it in this bug report.

Coin3-3.1.3-7.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.