Thread: pgsql: Make hstore_plperl's build even more like plperl's

pgsql: Make hstore_plperl's build even more like plperl's

From
Peter Eisentraut
Date:
Make hstore_plperl's build even more like plperl's

Combine the two places that set CPPFLAGS into one.  Also, some settings
should be restricted to Windows only.  More precisely, -Wno-comment is
a GCC-only option, but Windows in a makefile implies GCC at the moment.

Also, since -Wno-comment is more properly a preprocessor option, move it
to CPPFLAGS to simplify things a bit.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/0fd764647a9910a340359bb319929b70317b2ae4

Modified Files
--------------
contrib/hstore_plperl/Makefile |   10 ++++++----
src/pl/plperl/GNUmakefile      |    2 +-
2 files changed, 7 insertions(+), 5 deletions(-)


Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Andrew Dunstan
Date:



On 05/01/2015 10:20 PM, Peter Eisentraut wrote:
> Make hstore_plperl's build even more like plperl's
>
> Combine the two places that set CPPFLAGS into one.  Also, some settings
> should be restricted to Windows only.  More precisely, -Wno-comment is
> a GCC-only option, but Windows in a makefile implies GCC at the moment.
>
> Also, since -Wno-comment is more properly a preprocessor option, move it
> to CPPFLAGS to simplify things a bit.
>

Did you actually test this? On my system you have just managed to break
what I had unbroken. I'm not pleased.

cheers

andrew


Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Peter Eisentraut
Date:
On 5/2/15 4:32 AM, Andrew Dunstan wrote:
>
>
>
>
> On 05/01/2015 10:20 PM, Peter Eisentraut wrote:
>> Make hstore_plperl's build even more like plperl's
>>
>> Combine the two places that set CPPFLAGS into one.  Also, some settings
>> should be restricted to Windows only.  More precisely, -Wno-comment is
>> a GCC-only option, but Windows in a makefile implies GCC at the moment.
>>
>> Also, since -Wno-comment is more properly a preprocessor option, move it
>> to CPPFLAGS to simplify things a bit.
>>
>
> Did you actually test this? On my system you have just managed to break
> what I had unbroken.

I have re-fixed my fix.



Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Andrew Dunstan
Date:
On 05/02/2015 08:05 AM, Peter Eisentraut wrote:
> On 5/2/15 4:32 AM, Andrew Dunstan wrote:
>>
>>
>>
>> On 05/01/2015 10:20 PM, Peter Eisentraut wrote:
>>> Make hstore_plperl's build even more like plperl's
>>>
>>> Combine the two places that set CPPFLAGS into one.  Also, some settings
>>> should be restricted to Windows only.  More precisely, -Wno-comment is
>>> a GCC-only option, but Windows in a makefile implies GCC at the moment.
>>>
>>> Also, since -Wno-comment is more properly a preprocessor option, move it
>>> to CPPFLAGS to simplify things a bit.
>>>
>> Did you actually test this? On my system you have just managed to break
>> what I had unbroken.
> I have re-fixed my fix.
>
>

No you haven't. Putting the CORE directory at the end of PG_CPPFLAGS
doesn't work. Using the override to put it at the end of CPPFLAGS does
work. Please either completely fix this mess you've created for mingw
builds or stop meddling with my fixes WHICH I AM ACTUALLY TESTING.

cheers

andrew






Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Peter Eisentraut
Date:
On 5/2/15 10:20 AM, Andrew Dunstan wrote:
> Putting the CORE directory at the end of PG_CPPFLAGS doesn't work. Using
> the override to put it at the end of CPPFLAGS does work.

It's strange that the include directory order should matter.  What is
the explanation for that?  I don't see this documented anywhere.  I
don't see any evidence in the buildfarm that this makes a difference.

The intent of my changes was to unbreak other platforms that had been
broken by these Windows-related changes (which did succeed).



Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Andrew Dunstan
Date:
On 05/02/2015 10:40 AM, Peter Eisentraut wrote:
> On 5/2/15 10:20 AM, Andrew Dunstan wrote:
>> Putting the CORE directory at the end of PG_CPPFLAGS doesn't work. Using
>> the override to put it at the end of CPPFLAGS does work.
> It's strange that the include directory order should matter.  What is
> the explanation for that?  I don't see this documented anywhere.  I
> don't see any evidence in the buildfarm that this makes a difference.
>
> The intent of my changes was to unbreak other platforms that had been
> broken by these Windows-related changes (which did succeed).
>
>

What failures on other platforms? There's not a single buildfarm failure
I can see attributable to this.  All the recent failures are listed
here: <http://www.pgbuildfarm.org/cgi-bin/show_failures.pl>

The evidence in favor of my changes is in my testing. When I do it your
way I get errors, when I do it my way I don't.

Note that the override is EXACTLY what is done unconditionally in the
plperl GNUmakefile:

    override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
    -I$(perl_archlibexp)/CORE


So, contrary to your claims, you have not made this makefile more like
plperl's - you've made it less like plperl's, and detrimentally so.

cheers

andrew


Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Andrew Dunstan
Date:
On 05/02/2015 11:05 AM, Andrew Dunstan wrote:
>
> On 05/02/2015 10:40 AM, Peter Eisentraut wrote:
>> On 5/2/15 10:20 AM, Andrew Dunstan wrote:
>>> Putting the CORE directory at the end of PG_CPPFLAGS doesn't work.
>>> Using
>>> the override to put it at the end of CPPFLAGS does work.
>> It's strange that the include directory order should matter. What is
>> the explanation for that?  I don't see this documented anywhere.  I
>> don't see any evidence in the buildfarm that this makes a difference.
>>
>> The intent of my changes was to unbreak other platforms that had been
>> broken by these Windows-related changes (which did succeed).
>>
>>
>
> What failures on other platforms? There's not a single buildfarm
> failure I can see attributable to this.  All the recent failures are
> listed here: <http://www.pgbuildfarm.org/cgi-bin/show_failures.pl>
>
> The evidence in favor of my changes is in my testing. When I do it
> your way I get errors, when I do it my way I don't.
>
> Note that the override is EXACTLY what is done unconditionally in the
> plperl GNUmakefile:
>
>    override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
>    -I$(perl_archlibexp)/CORE
>
>
> So, contrary to your claims, you have not made this makefile more like
> plperl's - you've made it less like plperl's, and detrimentally so.


FYI, the attached patch allows jacana to build and test hstore_plperl
without error.

cheers

andrew



Attachment

Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Peter Eisentraut
Date:
On 5/2/15 11:05 AM, Andrew Dunstan wrote:
> Note that the override is EXACTLY what is done unconditionally in the
> plperl GNUmakefile:
>
>    override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
>    -I$(perl_archlibexp)/CORE

The override is a red herring.  It's done this way because all CPPFLAGS
settings have to be done with override, for unrelated reasons.  In a
pgxs build, however, you can set PG_CPPFLAGS, which is later put into
CPPFLAGS with override.

Also, requiring that -I$(perl_archlibexp)/CORE is last seems bizarre.
That would mean that an earlier include directory contains a file that
is also in .../CORE, and you don't want to the one in .../CORE.  In that
case, why add .../CORE at all?  We get Perl headers from .../CORE, and
that would then mean that other include directories contain some Perl
headers that you want to use in preference to the ones in .../CORE, but
at the same time you want the ones in .../CORE as a fallback.

That might possibly be the actual case, but it would be quite odd and
should be properly explained for posterity.



Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Peter Eisentraut
Date:
On 5/2/15 11:05 AM, Andrew Dunstan wrote:
> What failures on other platforms? There's not a single buildfarm failure
> I can see attributable to this.

http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=binturong&dt=2015-05-01%2020%3A13%3A12&stg=make-contrib



Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Andrew Dunstan
Date:
On 05/02/2015 01:03 PM, Peter Eisentraut wrote:
> On 5/2/15 11:05 AM, Andrew Dunstan wrote:
>> What failures on other platforms? There's not a single buildfarm failure
>> I can see attributable to this.
> http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=binturong&dt=2015-05-01%2020%3A13%3A12&stg=make-contrib
>
>

OK, that has nothing to do with the include order. I'm fine with making
the no-comment flag windows only, that was an error on my part. But you
changed more than that.

I don't have an explanation of why the include order matters. But it
does. If you don't think it does, try it, like I have.

cheers

andrew


Re: pgsql: Make hstore_plperl's build even more like plperl's

From
Andrew Dunstan
Date:
On 05/02/2015 01:02 PM, Peter Eisentraut wrote:
> On 5/2/15 11:05 AM, Andrew Dunstan wrote:
>> Note that the override is EXACTLY what is done unconditionally in the
>> plperl GNUmakefile:
>>
>>     override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
>>     -I$(perl_archlibexp)/CORE
> The override is a red herring.  It's done this way because all CPPFLAGS
> settings have to be done with override, for unrelated reasons.  In a
> pgxs build, however, you can set PG_CPPFLAGS, which is later put into
> CPPFLAGS with override.
>
> Also, requiring that -I$(perl_archlibexp)/CORE is last seems bizarre.
> That would mean that an earlier include directory contains a file that
> is also in .../CORE, and you don't want to the one in .../CORE.  In that
> case, why add .../CORE at all?  We get Perl headers from .../CORE, and
> that would then mean that other include directories contain some Perl
> headers that you want to use in preference to the ones in .../CORE, but
> at the same time you want the ones in .../CORE as a fallback.
>
> That might possibly be the actual case, but it would be quite odd and
> should be properly explained for posterity.
>
>


here's a list of the .h files in perl's CORE library on jacana.


I can see plenty of opportunities for conflict there.

cheers

andrew

arps/inet.h
av.h
bitcount.h
BuildInfo.h
charclass_invlists.h
config.h
cop.h
cv.h
dirent.h
dosish.h
embed.h
embedvar.h
EXTERN.h
fakesdio.h
fakethr.h
feature.h
form.h
git_version.h
gv.h
handy.h
hv.h
INTERN.h
intrpvar.h
iperlsys.h
keywords.h
l1_char_class_tab.h
malloc_ctl.h
metaconfig.h
mg.h
mg_data.h
mg_raw.h
mg_vtable.h
mydtrace.h
netdb.h
nostdio.h
op.h
opcode.h
opnames.h
op_reg_common.h
overload.h
pad.h
parser.h
patchlevel.h
perl.h
perlapi.h
perlhost.h
perlio.h
perliol.h
perlsdio.h
perlsfio.h
perlvars.h
perly.h
pp.h
pp_proto.h
proto.h
reentr.h
regcharclass.h
regcomp.h
regexp.h
regnodes.h
scope.h
sv.h
sys/socket.h
thread.h
time64.h
time64_config.h
uconfig.h
unixish.h
utf8.h
utfebcdic.h
util.h
uudmap.h
vdir.h
vmem.h
warnings.h
win32.h
win32iop-o.h
win32iop.h
win32thread.h
wince.h
XSUB.h