Обсуждение: Useless dependency assumption libxml2 -> libxslt in MSVC scripts

Hi all,

Looking at the MSVC scripts for some stuff I have noticed the following thing:
    if ($options->{xml})
    {
        if (!($options->{xslt} && $options->{iconv}))
        {
            die "XML requires both XSLT and ICONV\n";
        }
    }
But I don't understand the reason behind such a restriction to be
honest because libxml2 does not depend on libxslt. The contrary is
true: libxslt needs libxml2. Note as well that libxml2 does depend on
ICONV though. So I think that this condition should be relaxed as
follows:
    if ($options->{xml} && !$options->{iconv})
    {
        die "XML requires ICONV\n";
    }
And we also need to be sure that when libxslt is specified, libxml2 is
here to have the build working correctly.

Relaxing that would allow people to compile contrib/xml2 with just a
dependency to libxml2, without libxslt, something possible on any *nix
systems. As far as I can see this restriction comes from 9 years ago
in 2007 and commit 7f58ed1a. So nobody has complained about that so
far :)

Attached is a patch to address both issues.
Comments are welcome.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> But I don't understand the reason behind such a restriction to be
> honest because libxml2 does not depend on libxslt. The contrary is
> true: libxslt needs libxml2.

Right.

> Note as well that libxml2 does depend on ICONV though.

Hm, is that true either?  I don't see any sign of such a dependency
on Linux:

$ ldd /usr/lib64/libxml2.so.2.7.6       linux-vdso.so.1 =>  (0x00007fff98f6b000)       libdl.so.2 => /lib64/libdl.so.2
(0x000000377a600000)      libz.so.1 => /lib64/libz.so.1 (0x000000377ae00000)       libm.so.6 => /lib64/libm.so.6
(0x0000003779e00000)      libc.so.6 => /lib64/libc.so.6 (0x0000003779a00000)       /lib64/ld-linux-x86-64.so.2
(0x0000003779600000)

        regards, tom lane

On Thu, Sep 8, 2016 at 9:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> But I don't understand the reason behind such a restriction to be
>> honest because libxml2 does not depend on libxslt. The contrary is
>> true: libxslt needs libxml2.
>
> Right.
>
>> Note as well that libxml2 does depend on ICONV though.
>
> Hm, is that true either?  I don't see any sign of such a dependency
> on Linux:
>
> $ ldd /usr/lib64/libxml2.so.2.7.6
>         linux-vdso.so.1 =>  (0x00007fff98f6b000)
>         libdl.so.2 => /lib64/libdl.so.2 (0x000000377a600000)
>         libz.so.1 => /lib64/libz.so.1 (0x000000377ae00000)
>         libm.so.6 => /lib64/libm.so.6 (0x0000003779e00000)
>         libc.so.6 => /lib64/libc.so.6 (0x0000003779a00000)
>         /lib64/ld-linux-x86-64.so.2 (0x0000003779600000)

Peaking into the libxml2 code there is a configure switch
--with-iconv, so that's an optional dependency. And the same exists
for Windows in their win32/stuff. So I am mistaken and this could be
just removed.
-- 
Michael

On Thu, Sep 8, 2016 at 9:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Pretty sure this goes back to *our* XML support requiring both. As in you
> couldn't turn on/off xslt independently, so the "xml requires xslt" comment
> is that *our* xml support required both.
>
> This may definitely not be true anymore, and that check has just not been
> updated.
>
> Also this was 10 years ago, so I'm of course not 100% sure, but I think it
> was something like that...

Was it for contrib/xml2? For example was it because this module could
not be compiled with just libxml2?
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Sep 8, 2016 at 9:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> Pretty sure this goes back to *our* XML support requiring both. As in you
>> couldn't turn on/off xslt independently, so the "xml requires xslt" comment
>> is that *our* xml support required both.
>> 
>> This may definitely not be true anymore, and that check has just not been
>> updated.
>> 
>> Also this was 10 years ago, so I'm of course not 100% sure, but I think it
>> was something like that...

> Was it for contrib/xml2? For example was it because this module could
> not be compiled with just libxml2?

The core code has never used xslt at all.  Some quick digging in the git
history suggests that contrib/xml2 wasn't very clean about this before
2008:

commit eb915caf92a6805740e949c3233ee32bc9676484
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Thu May 8 16:49:37 2008 +0000
   Fix contrib/xml2 makefile to not override CFLAGS, and in passing make it   auto-configure properly for libxslt
presentor not.
 

or even 2010, depending on how large a value of "work" you want:

commit d6a6f8c6be4b6d6a9e90e92d91a83225bfe8d29d
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Mon Mar 1 18:07:59 2010 +0000
   Fix contrib/xml2 so regression test still works when it's built without libxslt.      This involves modifying the
moduleto have a stable ABI, that is, the   xslt_process() function still exists even without libxslt.  It throws a
runtimeerror if called, but doesn't prevent executing the CREATE FUNCTION   call.  This is a good thing anyway to
simplifycross-version upgrades.
 


Both of those fixes postdate our native-Windows port, though I'm not sure
of the origin of the specific test you're wondering about.
        regards, tom lane

On Thu, Sep 8, 2016 at 10:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The core code has never used xslt at all.

Yes.

> Some quick digging in the git
> history suggests that contrib/xml2 wasn't very clean about this before
> 2008:
> [...]
> Both of those fixes postdate our native-Windows port, though I'm not sure
> of the origin of the specific test you're wondering about.

Thanks. I was a bit too lazy to look at the history to get this piece
of history out... And so the code that is presently in the MSVC
scripts should have been updated at the same time as those
compilations have been relaxed, but it got forgotten on the way I
guess. Then it seemt to me that the attached patch would do things as
they should be done: removal of the condition for iconv, which is an
optional flag for libxml2, and reversing the check for libxslt <->
libxml2.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> Thanks. I was a bit too lazy to look at the history to get this piece
> of history out... And so the code that is presently in the MSVC
> scripts should have been updated at the same time as those
> compilations have been relaxed, but it got forgotten on the way I
> guess. Then it seemt to me that the attached patch would do things as
> they should be done: removal of the condition for iconv, which is an
> optional flag for libxml2, and reversing the check for libxslt <->
> libxml2.

Looks sane to me, will push.
        regards, tom lane

Michael Paquier <michael.paquier@gmail.com> writes:
> Thanks. I was a bit too lazy to look at the history to get this piece
> of history out... And so the code that is presently in the MSVC
> scripts should have been updated at the same time as those
> compilations have been relaxed, but it got forgotten on the way I
> guess. Then it seemt to me that the attached patch would do things as
> they should be done: removal of the condition for iconv, which is an
> optional flag for libxml2, and reversing the check for libxslt <->
> libxml2.

Actually, wait a minute.  Doesn't this bit in Mkvcbuild.pm need adjusted?
if ($solution->{options}->{xml}){    $contrib_extraincludes->{'pgxml'} = [        $solution->{options}->{xml} .
'/include',       $solution->{options}->{xslt} . '/include',        $solution->{options}->{iconv} . '/include' ];
 
    $contrib_extralibs->{'pgxml'} = [        $solution->{options}->{xml} . '/lib/libxml2.lib',
$solution->{options}->{xslt}. '/lib/libxslt.lib' ];}
 

It might accidentally fail to fail as-is, but likely it would be better
not to be pushing garbage paths into extraincludes and extralibs when
some of those options aren't set.
        regards, tom lane

On Sun, Sep 11, 2016 at 1:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It might accidentally fail to fail as-is, but likely it would be better
> not to be pushing garbage paths into extraincludes and extralibs when
> some of those options aren't set.

Right, we need to correct something here. But this block does not need
any adjustment: it can just be deleted. A contrib module is added via
AddProject() in Solution.pm, and if the build is done with xml, xslt
or iconv their libraries and includes are added in any case, for any
declared project. So declaring a dependency with xml or xslt is just
moot work, and actually this would be broken without AddProject
because it is missing to list iconv.lib... At the same time, I think
that we could fix the inclusions with uuid for uuid-ossp, and just put
them as well in AddProject with the rest. Please see the updated
cleanup patch attached.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sun, Sep 11, 2016 at 1:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It might accidentally fail to fail as-is, but likely it would be better
>> not to be pushing garbage paths into extraincludes and extralibs when
>> some of those options aren't set.

> Right, we need to correct something here. But this block does not need
> any adjustment: it can just be deleted. A contrib module is added via
> AddProject() in Solution.pm, and if the build is done with xml, xslt
> or iconv their libraries and includes are added in any case, for any
> declared project. So declaring a dependency with xml or xslt is just
> moot work, and actually this would be broken without AddProject
> because it is missing to list iconv.lib... At the same time, I think
> that we could fix the inclusions with uuid for uuid-ossp, and just put
> them as well in AddProject with the rest. Please see the updated
> cleanup patch attached.

Looks reasonable to me, we'll soon see what the buildfarm thinks.
        regards, tom lane

On Mon, Sep 12, 2016 at 1:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Looks reasonable to me, we'll soon see what the buildfarm thinks.

Thanks for the commit. I am seeing green statuses on the buildfarm.
-- 
Michael