Re: [lmi] [PATCH] Use submodules for, and newer versions of, libxml2 and libxslt

[Greg Chicares]

On 2020-10-03 17:01, Vadim Zeitlin wrote:
> On Fri, 2 Oct 2020 20:19:47 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 2020-10-02 16:37, Vadim Zeitlin wrote:
> GC> > On Fri, 2 Oct 2020 16:22:11 +0000 Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
[...]
> GC> > GC> In file included from /opt/lmi/src/lmi/xml_xslt_wrapp.cpp:39:
> GC> > GC> /opt/lmi/third_party/src/libxml/event_parser.cxx:61:38: error: 
> conflicting declaration of C functio
> GC> > GC> n ‘void xmlSAX2InitDefaultSAXHandler(xmlSAXHandlerV1*, int)’
> GC> > GC>    61 |     #define initxmlDefaultSAXHandler 
> xmlSAX2InitDefaultSAXHandler
> GC> > GC>       |                                      
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> GC> [...]
> GC> > GC> /opt/lmi/local/include/libxml2/libxml/SAX2.h:159:3: note: previous 
> declaration ‘void xmlSAX2InitDef
> GC> > GC> aultSAXHandler(xmlSAXHandler*, int)’

"#define" had seemed odd in that error message, but I didn't
investigate it or even call attention to it.

>  So what happens here is that we #define initxmlDefaultSAXHandler as
> something else, in order to allow to compile the code which calls this
> function using both with the ancient (pre-2.6.0, which was released 17
> years ago) versions of libxml2, which defined this function in SAX.h, and
> newer ones, which define xmlSAX2InitDefaultSAXHandler() in SAX2.h (while
> still providing the old function in SAX.h for compatibility, of course).
> This is ugly, but doesn't create any real problems as long as each file is
> compiled separately.

Okay, now I see at least the proximate cause of the problem...

>  When they're all compiled as a single translation unit in lmi, however, we
> have a problem: event_parser.cxx defines initxmlDefaultSAXHandler(), but
> then tree_parser.cxx, whose contents is compiled later, includes SAX.h
> which defines the real function with this name. But because this function
> was redefined as some other function, with a different declaration, in
> SAX2.h which had been already included from event_parser.cxx before, this
> results in the error.

So perhaps we should include 'SAX.h' earlier? Or maybe it's subtler than that...

>  This much became clear almost immediately, but I couldn't understand why
> has this changed when we upgraded libxml2 because git-diff showed no
> changes neither in SAX.h nor SAX2.h between the old and the new versions.
> And the answer turned out to be that in the old version SAX.h was already
> included long before event_parser.cxx had the opportunity to do its
> mischief because it was implicitly included from the other libxml2 headers,
> via libxml2/globals.h. A couple of years ago (in libxml2 2.9.9), they have
> finally removed this obsolete header from globals.h, which means that now
> we actually include it from tree_parser.cxx for the first time, while
> before we just hit the header guard and didn't really parse the contents of
> this header when compiling it.

I have an urge to look at how you've fixed it...

>  Anyhow, I've fixed this in xmlwrapp master now (and will probably make
> another release of it soon), so we will be able to re-upgrade libxml2 again
> as soon as we integrate the latest xmlwrapp into lmi as a submodule.

...but I'm disciplining myself to focus on 'perf' instead, and you've
apparently solved it already by juggling '-I' paths...

> GC> and instead of using some lmi makefile for that, I'd rather
> GC> just wait for the xmlwrapp submodule.
> 
>  This is actually an important point: I was going to preserve the current
> "unity" build of xmlwrapp which is used right now, i.e. continue using
> xml_xslt_wrapp.cpp file, but just set up the include paths so that it finds
> the files it includes in third_party/xmlwrapp submodule rather than
> somewhere else on the system. But does the above mean that you'd like to
> change this and use xmlwrapp normal build?

That sounds like one question, but let me dichotomize it: xmlwrapp
and xsltwrapp have
 - source code (in lmi today, some tarball, I guess), and
 - a build method (#include everything into one file, then compile that)
and I had thought of those as independent.

For the future lmi, I'm sure we'll want submodules instead of tarballs,
so we'll never want another tarball. I hadn't given any thought to how
it'll be built. The old #include-all-the-things method has the advantage
of working, except when it doesn't; but it's an exotic use case that I
suppose should be extirpated rather than fixed and maintained. Wouldn't
you agree? There's a little extra work involved, e.g., to get rid of
occurrences of
  grep xmlwrapp_objects *make*
At the same time, we should consider addressing this in 'workhorse.make':

# The link command promiscuously mentions libxml2 for all targets.
# Measurements show that this costs one-tenth of a second on
# reasonable hardware, and it saves the trouble of maintaining a list
# of which targets require which libraries.

because it seems likely that places where we remove 'xmlwrapp_objects'
are exactly the places where we should add '-lxml2':

 actuarial_table_test$(EXEEXT): \
   $(boost_filesystem_objects) \
   $(common_test_objects) \
-  $(xmlwrapp_objects) \
   actuarial_table.o \
   actuarial_table_test.o \
   cso_table.o \
   timer.o \
   xml_lmi.o \
+actuarial_table_test$(EXEEXT): LDFLAGS += -lxml2

which IIRC is what 'Makefile.in' already does. And maybe that
lets us simplify 'platform_gnome_xml_libraries' somewhat: i.e.,
if we're going to build it for any platform, shouldn't we build
it for every platform?

If the lmi makefiles remain a riddle to you, I'd be glad to
handle them myself, because I know what they're supposed to mean.

>  I'm probably still going to do it in 2 steps, i.e. add xmlwrapp submodule
> first and change the build system later, if wanted, so this is not very
> urgent, but please let me know if you think the second step is worth doing
> at all. FWIW personally I wouldn't have added xml_xslt_wrapp.cpp from the
> beginning, but now that it's there, I don't see any real problem that
> having it presents.
Um...okay, whichever. We can kick that second step into some
future year: it might prove intricate, and we have other fish
to fry. But ultimately I'm pretty sure we shouldn't have this:

platform_gnome_xml_libraries := \
  -lexslt \
  $(shell xslt-config --libs) \
  $(shell xml2-config --libs) \

in 'posix_fhs.make' and a potentially different library
release for other platforms--all should use either the
"#include whatever*.cpp" technique, or all should run
"./configure && make && make install".