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
Attachments:
msvc_xml_relax.patchapplication/x-download; name=msvc_xml_relax.patchDownload
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 9cb1ad3..a6a490f 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -37,12 +37,13 @@ sub _new
unless exists $options->{float8byval};
die "float8byval not permitted on 32 bit platforms"
if $options->{float8byval} && $bits == 32;
- if ($options->{xml})
+ if ($options->{xml} && !$options->{iconv})
{
- if (!($options->{xslt} && $options->{iconv}))
- {
- die "XML requires both XSLT and ICONV\n";
- }
+ die "XML requires ICONV\n";
+ }
+ if ($options->{xslt} && !$options->{xml})
+ {
+ die "XSLT requires XML\n";
}
$options->{blocksize} = 8
unless $options->{blocksize}; # undef or 0 means default
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 8, 2016 at 2: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.
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...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 8, 2016 at 2:53 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:
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?
Yes, that's what I'm referring to.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
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 present or 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 module to have a stable ABI, that is, the
xslt_process() function still exists even without libxslt. It throws a
runtime error if called, but doesn't prevent executing the CREATE FUNCTION
call. This is a good thing anyway to simplify cross-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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
msvc_xml_relax_v2.patchapplication/x-download; name=msvc_xml_relax_v2.patchDownload
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 9cb1ad3..97e5c20 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -37,12 +37,9 @@ sub _new
unless exists $options->{float8byval};
die "float8byval not permitted on 32 bit platforms"
if $options->{float8byval} && $bits == 32;
- if ($options->{xml})
+ if ($options->{xslt} && !$options->{xml})
{
- if (!($options->{xslt} && $options->{iconv}))
- {
- die "XML requires both XSLT and ICONV\n";
- }
+ die "XSLT requires XML\n";
}
$options->{blocksize} = 8
unless $options->{blocksize}; # undef or 0 means default
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
msvc_depend_cleanup.patchtext/x-diff; charset=US-ASCII; name=msvc_depend_cleanup.patchDownload
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index b3ed1f5..93dfd24 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -381,18 +381,7 @@ sub mkvcbuild
$zic->AddDirResourceFile('src/timezone');
$zic->AddReference($libpgcommon, $libpgport);
- 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' ];
- }
- else
+ if (!$solution->{options}->{xml})
{
push @contrib_excludes, 'xml2';
}
@@ -402,14 +391,7 @@ sub mkvcbuild
push @contrib_excludes, 'sslinfo';
}
- if ($solution->{options}->{uuid})
- {
- $contrib_extraincludes->{'uuid-ossp'} =
- [ $solution->{options}->{uuid} . '/include' ];
- $contrib_extralibs->{'uuid-ossp'} =
- [ $solution->{options}->{uuid} . '/lib/uuid.lib' ];
- }
- else
+ if (!$solution->{options}->{uuid})
{
push @contrib_excludes, 'uuid-ossp';
}
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 9cb1ad3..8217d06 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -37,12 +37,9 @@ sub _new
unless exists $options->{float8byval};
die "float8byval not permitted on 32 bit platforms"
if $options->{float8byval} && $bits == 32;
- if ($options->{xml})
+ if ($options->{xslt} && !$options->{xml})
{
- if (!($options->{xslt} && $options->{iconv}))
- {
- die "XML requires both XSLT and ICONV\n";
- }
+ die "XSLT requires XML\n";
}
$options->{blocksize} = 8
unless $options->{blocksize}; # undef or 0 means default
@@ -555,6 +552,11 @@ sub AddProject
$proj->AddIncludeDir($self->{options}->{xslt} . '\include');
$proj->AddLibrary($self->{options}->{xslt} . '\lib\libxslt.lib');
}
+ if ($self->{options}->{uuid})
+ {
+ $proj->AddIncludeDir($self->{options}->{uuid} . '\include');
+ $proj->AddLibrary($self->{options}->{uuid} . '\lib\uuid.lib');
+ }
return $proj;
}
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers