Apache OpenOffice (AOO) Bugzilla – Full Text Issue Listing |
Summary: | [cws sb118] system-cppunit | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | Build Tools | Reporter: | rene | ||||||
Component: | code | Assignee: | rene | ||||||
Status: | CLOSED FIXED | QA Contact: | issues@tools <issues> | ||||||
Severity: | Trivial | ||||||||
Priority: | P3 | CC: | caolanm, issues, stephan.bergmann.secondary | ||||||
Version: | current | ||||||||
Target Milestone: | OOo 3.3 | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Issue Type: | PATCH | Latest Confirmation in: | --- | ||||||
Developer Difficulty: | --- | ||||||||
Attachments: |
|
Description
rene
2010-03-03 21:07:27 UTC
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. |