[Bug 694287] Review Request: openCOLLADA - 3D import and export libraries

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

--- Comment #46 from Hans de Goede <hdegoede at redhat.com> 2011-04-27 11:47:37 EDT ---
Hi again,

(In reply to comment #45)
> (In reply to comment #44)

<snip>

> > * Remove %{?_smp_mflags} from your make invocation, otherwise the build fails
> >   at least it does so consistently on my quad core.
> >   Funny how you kept the comment from me snippet saying that the build breaks,
> >   but re-added the %{?_smp_mflags} :)
> 
> When you said it didn't work I didn't think you meant failed to build, just
> that it didn't do anything. It works for me on my dual core with -j4. Maybe we
> should spec a max number of threads conditional?

No, there is some race condition in there somehow, so anything > 1 is too much.
Maybe it only gets triggered with a newer cmake version, who knows ?

Anyways it is best to just not use %{?_smp_mflags} in this case to avoid build
failures on for example the buildsystem.

> 
> 
> > * You're still installing header files from the common dir, resulting in
> >   unittest and performance test headers ending up under /usr/include, but
> >   see the next item for a more radical suggestion for re-arranging the
> >   headers.
> 
> Should I remove common in addition to the below suggestion?

s/remove/not install headers from/ -> yes

> What about Externals/MathMLSolver?
> 

I think it is ok to include the headers for that.

> 
> > * And last, one slightly larger issue (which I should have checked before).
> >   I'm not really happy with putting a bunch of the .h files directly under
> >   /usr/include. Ideally (IMHO) COLLADAfoo/include/* should end up as
> >   /usr/include/COLLADAfoo/* for all variants of foo
> 
> Hmm. This sounds like a job for bash and my bashfoo is not that strong :)
> 
> I'm thinking some sort of for loop that either inverts foo/include to
> include/foo or that strips the ./include off before copying the files.
> 

Something like this should do the trick:

for i in COLLADABaseUtils COLLADAFramework COLLADAMax COLLADAMaya \
         COLLADASaxFrameworkLoader COLLADAStreamWriter GeneratedSaxParser; do
    mkdir -p $RPM_BUILD_ROOT%{_includedir}/$i
    cp -a $i/include/* $RPM_BUILD_ROOT%{_includedir}/$i
done

And then "manually" do Externals/MathMLSolver since that is a subdir of a
subdir

> Another option would be to create a /usr/include/openCOLLADA and then just dump
> all the files and/or directories under that...
> What do you think?

I think it is good to keep the headers grouped in the way they are grouped
already in the sources. You could replace %{_includedir} with
%{_includedir}/openCOLLADA in the above script, but I don't think that is
necessary.

Regards,

Hans


p.s.

One other thing, it might be good to also package the COLLADAValidator package
in a -utils subpackage, as that may be a useful utility to have.

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