Thread: Broken handling of lwlocknames.h
Bjorn Munch reported off-list that this sequence:
unpack tarball, cd into it
./configure ...
cd src/test/regress
make
no longer works in 9.6beta2, where it did work in previous releases.
I have confirmed both statements. The failure looks like
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security-fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o
regress.c
In file included from ../../../src/include/storage/lock.h:23, from
../../../src/include/access/heapam.h:22, from ../../../src/include/nodes/execnodes.h:18,
from../../../src/include/commands/trigger.h:17, from regress.c:29:
../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory
make: *** [regress.o] Error 1
So this is some sort of fallout from commit aa65de042f582896, which
invented that as a generated file.
Perhaps the solution is to extend src/test/regress/GNUmakefile to know
about this file explicitly (as it already does know about
src/port/pg_config_paths.h). But that seems rather brute-force; in
particular it seems like that does nothing to keep us from getting burnt
again the same way in future. I wonder if we should modify
src/backend/Makefile so that it exposes a phony target for "update all the
generated include files", and then have src/test/regress/GNUmakefile call
that. Or maybe there are other ways.
regards, tom lane
Re: Tom Lane 2016-06-27 <31398.1467036827@sss.pgh.pa.us> > Bjorn Munch reported off-list that this sequence: > > unpack tarball, cd into it > ./configure ... > cd src/test/regress > make > > no longer works in 9.6beta2, where it did work in previous releases. > I have confirmed both statements. The failure looks like > > gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o regress.c > In file included from ../../../src/include/storage/lock.h:23, > from ../../../src/include/access/heapam.h:22, > from ../../../src/include/nodes/execnodes.h:18, > from ../../../src/include/commands/trigger.h:17, > from regress.c:29: > ../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory > make: *** [regress.o] Error 1 > > So this is some sort of fallout from commit aa65de042f582896, which > invented that as a generated file. > > Perhaps the solution is to extend src/test/regress/GNUmakefile to know > about this file explicitly (as it already does know about > src/port/pg_config_paths.h). But that seems rather brute-force; in > particular it seems like that does nothing to keep us from getting burnt > again the same way in future. I wonder if we should modify > src/backend/Makefile so that it exposes a phony target for "update all the > generated include files", and then have src/test/regress/GNUmakefile call > that. Or maybe there are other ways. That would also fix the "build plpython3 only" problem I was aiming at in https://www.postgresql.org/message-id/20150916200959.GB32090@msg.df7cb.de So another +1 from a packagers perspective. Christoph
On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg <myon@debian.org> wrote: > Re: Tom Lane 2016-06-27 <31398.1467036827@sss.pgh.pa.us> >> Bjorn Munch reported off-list that this sequence: >> >> unpack tarball, cd into it >> ./configure ... >> cd src/test/regress >> make >> >> no longer works in 9.6beta2, where it did work in previous releases. >> I have confirmed both statements. The failure looks like >> >> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o regress.c >> In file included from ../../../src/include/storage/lock.h:23, >> from ../../../src/include/access/heapam.h:22, >> from ../../../src/include/nodes/execnodes.h:18, >> from ../../../src/include/commands/trigger.h:17, >> from regress.c:29: >> ../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory >> make: *** [regress.o] Error 1 >> >> So this is some sort of fallout from commit aa65de042f582896, which >> invented that as a generated file. >> >> Perhaps the solution is to extend src/test/regress/GNUmakefile to know >> about this file explicitly (as it already does know about >> src/port/pg_config_paths.h). But that seems rather brute-force; in >> particular it seems like that does nothing to keep us from getting burnt >> again the same way in future. I wonder if we should modify >> src/backend/Makefile so that it exposes a phony target for "update all the >> generated include files", and then have src/test/regress/GNUmakefile call >> that. Or maybe there are other ways. > > That would also fix the "build plpython3 only" problem I was aiming at > in https://www.postgresql.org/message-id/20150916200959.GB32090@msg.df7cb.de > > So another +1 from a packagers perspective. Yes that would be indeed cleaner this way. I have poked a bit at that and finished with the attached that defines some rules to generate all the files needed. But actually it does not seem to be enough, for example on OSX this would fail to compile because it cannot find the postgres binary in src/backend/postgres. Does somebody have an idea what this is about? It seems that we have two problems here. -- Michael
Attachment
On Tue, Jun 28, 2016 at 12:26:00PM +0900, Michael Paquier wrote: > On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg <myon@debian.org> wrote: > > Re: Tom Lane 2016-06-27 <31398.1467036827@sss.pgh.pa.us> > >> Bjorn Munch reported off-list that this sequence: > >> > >> unpack tarball, cd into it > >> ./configure ... > >> cd src/test/regress > >> make > >> > >> no longer works in 9.6beta2, where it did work in previous releases. > >> I have confirmed both statements. The failure looks like > >> > >> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic -I../../../src/include -D_GNU_SOURCE -c -o regress.o regress.c > >> In file included from ../../../src/include/storage/lock.h:23, > >> from ../../../src/include/access/heapam.h:22, > >> from ../../../src/include/nodes/execnodes.h:18, > >> from ../../../src/include/commands/trigger.h:17, > >> from regress.c:29: > >> ../../../src/include/storage/lwlock.h:129:33: error: storage/lwlocknames.h: No such file or directory > >> make: *** [regress.o] Error 1 > >> > >> So this is some sort of fallout from commit aa65de042f582896, which > >> invented that as a generated file. > >> > >> Perhaps the solution is to extend src/test/regress/GNUmakefile to know > >> about this file explicitly (as it already does know about > >> src/port/pg_config_paths.h). But that seems rather brute-force; in > >> particular it seems like that does nothing to keep us from getting burnt > >> again the same way in future. I wonder if we should modify > >> src/backend/Makefile so that it exposes a phony target for "update all the > >> generated include files", and then have src/test/regress/GNUmakefile call > >> that. Or maybe there are other ways. > > > > That would also fix the "build plpython3 only" problem I was aiming at > > in https://www.postgresql.org/message-id/20150916200959.GB32090@msg.df7cb.de > > > > So another +1 from a packagers perspective. > > Yes that would be indeed cleaner this way. I have poked a bit at that > and finished with the attached that defines some rules to generate all > the files needed. But actually it does not seem to be enough, for > example on OSX this would fail to compile because it cannot find the > postgres binary in src/backend/postgres. Does somebody have an idea > what this is about? It seems that we have two problems here. [Action required within 72 hours. This is a generic notification.] The above-described topic is currently a PostgreSQL 9.6 open item. Robert, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a 9.6 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within 72 hours of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping 9.6rc1. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com
On Fri, Jul 1, 2016 at 12:09 AM, Noah Misch <noah@leadboat.com> wrote: > [Action required within 72 hours. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 9.6 open item. Robert, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > 9.6 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within 72 hours of this > message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping 9.6rc1. Consequently, I will appreciate your > efforts toward speedy resolution. Thanks. > > [1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com Since Tom proposed the approach which Michael's patch takes, I'm hoping he will review and commit this. If it is left to me to fix it, I may just adopt a minimal fix. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Since Tom proposed the approach which Michael's patch takes, I'm
> hoping he will review and commit this. If it is left to me to fix it,
> I may just adopt a minimal fix.
I'll take a look at it.
regards, tom lane
On Fri, Jul 1, 2016 at 10:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Since Tom proposed the approach which Michael's patch takes, I'm >> hoping he will review and commit this. If it is left to me to fix it, >> I may just adopt a minimal fix. > > I'll take a look at it. Note: the patch is not complete yet. I found some bugs in it. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> Yes that would be indeed cleaner this way. I have poked a bit at that
> and finished with the attached that defines some rules to generate all
> the files needed. But actually it does not seem to be enough, for
> example on OSX this would fail to compile because it cannot find the
> postgres binary in src/backend/postgres. Does somebody have an idea
> what this is about? It seems that we have two problems here.
Yeah, on OS X, building regress.so (or any other loadable extension)
will fail unless you built the backend first:
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security-fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -g -O2 -bundle -multiply_defined
suppress-Wl,-undefined,dynamic_lookup -o regress.so regress.o -L../../../src/port -L../../../src/common
-Wl,-dead_strip_dylibs -bundle_loader ../../../src/backend/postgres
ld: file not found: ../../../src/backend/postgres
The reason is you need the "-bundle_loader postgres" option, because
OS X's linker is pickier about unresolved symbols than Linux's.
However, that is not a 9.6 regression: that's never worked on OS X, or at
least not since we started using -bundle_loader.
I do not think that we want the makefiles to enforce this build
dependency, as that would completely destroy any speed advantage of trying
to build just the particular .so. Maybe we could enforce the dependency
just on OS X, but since we haven't gotten complaints from people trying
to build like this on OS X, I doubt it's worth the trouble.
So my inclination is to fix the include-file issue and call it good.
In any case, if someone did want to deal with making the -bundle_loader
calls safer, it would be material for a separate patch IMO.
regards, tom lane
Michael Paquier <michael.paquier@gmail.com> writes:
> Yes that would be indeed cleaner this way. I have poked a bit at that
> and finished with the attached that defines some rules to generate all
> the files needed.
I made some mostly-cosmetic changes to this and pushed it. One thing
to note is that it seemed to me you'd broken the rule for schemapg.h:
by removing the phony target, I think you removed the behavior that
we'd always go and recheck schemapg.h's dependencies. To do it correctly
without that target, we'd need src/backend/Makefile to know all of those
dependencies, duplicating the rather long list in catalog/Makefile.
regards, tom lane
On Sat, Jul 2, 2016 at 4:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> Yes that would be indeed cleaner this way. I have poked a bit at that >> and finished with the attached that defines some rules to generate all >> the files needed. > > I made some mostly-cosmetic changes to this and pushed it. One thing > to note is that it seemed to me you'd broken the rule for schemapg.h: > by removing the phony target, I think you removed the behavior that > we'd always go and recheck schemapg.h's dependencies. To do it correctly > without that target, we'd need src/backend/Makefile to know all of those > dependencies, duplicating the rather long list in catalog/Makefile. Thanks. Yes the bug that I was mentioning yesterday was that I missed one place where the target of schemapg.h was referenced, and I didn't take the time to fully cover this set of dependencies.. -- Michael
Re: Tom Lane 2016-07-01 <26357.1467400438@sss.pgh.pa.us> > I made some mostly-cosmetic changes to this and pushed it. I confirm that Debian's out-of-tree python3 build works now when invoked directly in the relevant plpython/hstore_plpython subdirectories. Thanks! Christoph