Thread: pgsql: Make hstore_plperl's build even more like plperl's
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(-)
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
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.
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
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).
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
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
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.
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
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
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