Created attachment 68134 [details]
patch

@rene:  A few things come to mind:
1  libs.mk CPPUNITLIB:  For one, shouldn't the check for SYSTEM_CPPUNIT come before the check for WNT 
(so that WNT could, at least in principle, also use SYSTEM_CPPUNIT)?  For another, the "+=" in "CPPUNITLIB 
+= $(CPPUNIT_LIBS)" is probably a typo.  (And for a third, wouldn't it be better to replace CPPUNITLIB with 
CPPUNIT_LIBS everywhere and set it in libs.mk only if not SYSTEM_CPPUNIT?)
2  What looks odd is that only sal/cppunittester/makefile.mk mentions CPPUNIT_CFLAGS, not the other 
makefiles that reference cppunit.  (And again, wouldn't it be better if the CFLAGS+=CPPUNIT_CFLAGS 
would be unconditional, and CPPUNIT_CFLAGS were set empty for the non-SYSTEM_CPPUNIT case in some 
solenv/inc/*.mk?)

> 1  libs.mk CPPUNITLIB:  For one, shouldn't the check for SYSTEM_CPPUNIT come >
before the check for WN (so that WNT could, at least in principle, also use
SYSTEM_CPPUNIT)? 

no system-* will probably work on windows, nor would it make sense, so I
wouldn't bother

> For  another, the "+=" in "CPPUNITLIB  += $(CPPUNIT_LIBS)" is probably a typo.

Yes.

> (And for a third, wouldn't it be better to replace CPPUNITLIB with 
> CPPUNIT_LIBS everywhere and set it in libs.mk only if not SYSTEM_CPPUNIT?)

Wanted to do it minimal-invasive, but we can do, yes.

> 2  What looks odd is that only sal/cppunittester/makefile.mk mentions
> CPPUNIT_CFLAGS, not the other makefiles that reference cppunit.

I only noticed that one, will grep for the rest.
they are for example?

> (And again, wouldn't it be better if the CFLAGS+=CPPUNIT_CFLAGS 
> would be unconditional, and CPPUNIT_CFLAGS were set empty for the
> non-SYSTEM_CPPUNIT case in some  solenv/inc/*.mk?)

I don't think so, libs != includes, and we don't do that for any
other system-* either

> Wanted to do it minimal-invasive, but we can do, yes.

Besides that, we do that for other system-*, too (see the libxslt right below
cppunit), so for consistency we should do it like in the initial patch IMHO

> 2  What looks odd is that only sal/cppunittester/makefile.mk mentions
> CPPUNIT_CFLAGS, not the other makefiles that reference cppunit.

Do they need the includes (And it would for me either way as /usr/include is
a standard path and this /usr/include/cppunit/* is found anyway without -I);
cppunittester of course needs headers, the other occurances in sal are just
linkages afaics.

> Do they need the include

answer to myself: yes, sal/qa/rtl/*.cxx....

"I only noticed that one, will grep for the rest. they are for example?": e.g., o3tl/qa/makefile.mk;  "Do they 
need the includes": yes, they do

"Wanted to do it minimal-invasive, but we can do, yes.", "I don't think so, libs != includes, and we don't do 
that for any other system-* either", "so for consistency we should do it like in the initial patch IMHO":  
True, we would start something new with this.  But IMO something much better than the current 
conventions (e.g., I would have loved to have CXX/LDFLAGS_STLPORT available in the new 
cppunit/makefile.mk, and had to re-engineer them from the solenv/inc/*.mk mess).  So, IMO, I would not 
mind deviating from old conventions here...

Created attachment 68135 [details]
new patch with typo fixes and adding the other makefiles

s/libwpd/cppunit/ in configure.in of course, will not attach a new file for just
that :-) .oO ( cut'n'waste )

sb: any objection to push it like it is (+/- stuff I will find, a build runs
currently) and do finetuning/improvements later?

"any objection to push it like it is [...] and do finetuning/improvements
later?"  Fine with me.

Two further points:
1  CPPUNIT_CFLAGS should probably always only be added to CXXFLAGS, never to CFLAGS.
2  From what I had written down at
<http://wiki.services.openoffice.org/wiki/Test_Cleanup#CppUnit_1.12.1> a while
ago, "TODO: OOo configure --with-system-cppunit (only if also
--with-system-stl!); minimal required version?"  That is, configure should fail
for --with-system-cppunit/--without-system-stl combination.  I am not (yet) sure
what the minimal required version is, though.

> 1  CPPUNIT_CFLAGS should probably always only be added to CXXFLAGS, never to
> CFLAGS.

maybe, but CFLAGS works and is consistent with the rest.

> 2  From what I had written down at
> <http://wiki.services.openoffice.org/wiki/Test_Cleanup#CppUnit_1.12.1> a while
> ago, "TODO: OOo configure --with-system-cppunit (only if also
> --with-system-stl!); minimal required version?"

ARGH. Not again.

>  That is, configure should fail
> for --with-system-cppunit/--without-system-stl combination.  I am not (yet)
> sure what the minimal required version is, though.

Well, I guess we can just use 1.12, no? Shouldn't matter much as we didn't have
that flag before anywy.

Wrt STLport: no way, if you really have a STL conflict (like we ha(d/ve) with
graphite and mysqlc) apply a workaround like the one caolan did for graphite
already (see issue 106157);  it is bad to use system-cppunit on everything
except i386. Better all or none....

Ccing cmc

mumble, we should really consider dropping using stlport in OOo itself, and just
build it and bundle it as an extra into the install sets for compatibility with
existing 3rd party binary extensions. I think that'd probably work as I don't
think it would modify the public abi to build it against just libstdc++.

>> 1  CPPUNIT_CFLAGS should probably always only be added to CXXFLAGS, never to
>> CFLAGS.
> 
> maybe, but CFLAGS works and is consistent with the rest.

Technically, CFLAGS works (as CFLAGS is added to C++ compilation in addition to
CXXFLAGS), but CXXFLAGS would be more correct.  Anyway, even if you want to keep
using CFLAGS for consistency, please clean up the places that now have both,
CFLAGS+=CPPUNIT_CFLAGS and CXXFLAGS+=CPPUNIT_CFLAGS.

>>  That is, configure should fail
>> for --with-system-cppunit/--without-system-stl combination.  I am not (yet)
>> sure what the minimal required version is, though.
> 
> Well, I guess we can just use 1.12, no? Shouldn't matter much as we didn't have
> that flag before anywy.

Probably good, yes.  If somebody ever has a need for a lower version, they can
check to modify this then.

> Wrt STLport: no way, if you really have a STL conflict (like we ha(d/ve) with
> graphite and mysqlc) apply a workaround like the one caolan did for graphite
> already (see issue 106157);  it is bad to use system-cppunit on everything
> except i386. Better all or none....

You unfortunately cannot work around that here.  The interface of CppUnit is
full of std-types, so the OOo test code calling CppUnit and CppUnit itself have
to use the same STL implementation.  However, the OOo test code also shall
access OOo code that has std-types in its respective interface, so those two
need to use the same STL implementation, too.

We can only improve things here when we eventually drop the STLport-requirement
(and become URE-incompatible on the affected platforms).

@cmc:  Might work (problematic could be the ABIs of cppuhelper and salhelper,
incl. passing objects through void* and throwing std-based exceptions).  Anyway,
I would prefer moving that to another issue or thread.

> We can only improve things here when we eventually drop the STLport-requirement
> (and become URE-incompatible on the affected platforms).

one more reason we should have done it with 3.0.0 (major version which has other
structure because of the 3-layers anyway, why not breking this then) :-(

sb: ok, added the check :-(

OK to set to FIXED, I assume (I want to pass CWS sb118 to QA soon)

.

close issue.