Обсуждение: pgsql: Move gramparse.h to src/backend/parser
Move gramparse.h to src/backend/parser This header is semi-private, being used only in files related to raw parsing, so move to the backend directory where those files live. This allows removal of Makefile rules that symlink gram.h to src/include/parser, since gramparse.h can now include gram.h from within the same directory. This has the side-effect of no longer installing gram.h and gramparse.h, but there doesn't seem to be a good reason to continue doing so. Per suggestion from Andres Freund and Peter Eisentraut Discussion: https://www.postgresql.org/message-id/20220904181759.px6uosll6zbxcum5%40awork3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ecaf7c5df54f7fa9df2fdc7225d2bb4e283f0081 Modified Files -------------- src/backend/Makefile | 7 +------ src/backend/parser/gram.y | 2 +- src/{include => backend}/parser/gramparse.h | 4 ++-- src/backend/parser/parser.c | 2 +- src/backend/parser/scan.l | 2 +- src/include/Makefile | 4 ++-- src/include/parser/.gitignore | 1 - src/tools/msvc/Install.pm | 4 ---- src/tools/pginclude/cpluspluscheck | 1 - src/tools/pginclude/headerscheck | 1 - 10 files changed, 8 insertions(+), 20 deletions(-)
On Wed, Sep 14, 2022 at 11:02 AM John Naylor <john.naylor@postgresql.org> wrote: > > Move gramparse.h to src/backend/parser Member crake doesn't like this. I thought I tried vpath for this patch, but I'll go confirm now. -- John Naylor EDB: http://www.enterprisedb.com
John Naylor <john.naylor@postgresql.org> writes:
> Move gramparse.h to src/backend/parser
The cfbot is unhappy since this commit; some but not all tests fail with
[09:33:13.793] In file included from scan.c:39:
[09:33:13.793] ./gramparse.h:29:10: fatal error: 'gram.h' file not found
[09:33:13.794] #include "gram.h"
[09:33:13.794] ^~~~~~~~
[09:33:13.839] In file included from parser.c:25:
[09:33:13.839] ./gramparse.h:29:10: fatal error: 'gram.h' file not found
[09:33:13.839] #include "gram.h"
[09:33:13.839] ^~~~~~~~
What I think is happening is that it was a mistake to remove
parser/gram.h from the dependencies of backend/Makefile's
generated-headers target: that allows builds to proceed before
gram.h has necessarily been created. The fact that it works
at all for anybody says that there's another dependency path
somewhere that causes bison to get run ... but, seemingly,
that doesn't always happen soon enough in a parallel build.
regards, tom lane
Hi, On 2022-09-14 15:37:06 -0400, Tom Lane wrote: > John Naylor <john.naylor@postgresql.org> writes: > > Move gramparse.h to src/backend/parser > > The cfbot is unhappy since this commit; some but not all tests fail with > > [09:33:13.793] In file included from scan.c:39: > [09:33:13.793] ./gramparse.h:29:10: fatal error: 'gram.h' file not found > [09:33:13.794] #include "gram.h" > [09:33:13.794] ^~~~~~~~ > [09:33:13.839] In file included from parser.c:25: > [09:33:13.839] ./gramparse.h:29:10: fatal error: 'gram.h' file not found > [09:33:13.839] #include "gram.h" > [09:33:13.839] ^~~~~~~~ > > What I think is happening is that it was a mistake to remove > parser/gram.h from the dependencies of backend/Makefile's > generated-headers target: that allows builds to proceed before > gram.h has necessarily been created. The fact that it works > at all for anybody says that there's another dependency path > somewhere that causes bison to get run ... but, seemingly, > that doesn't always happen soon enough in a parallel build. But why doesn't the below take care of it? > # Force these dependencies to be known even without dependency info built: > gram.o scan.o parser.o: gram.h The only file including gram.h is gramparse.h, which in turn is only included by parser.c, scan.l, gram.y. So this should suffice. Greetings, Andres Freund
Hi, On 2022-09-14 13:57:15 -0700, Andres Freund wrote: > On 2022-09-14 15:37:06 -0400, Tom Lane wrote: > > John Naylor <john.naylor@postgresql.org> writes: > > > Move gramparse.h to src/backend/parser > > > > The cfbot is unhappy since this commit; some but not all tests fail with > > > > [09:33:13.793] In file included from scan.c:39: > > [09:33:13.793] ./gramparse.h:29:10: fatal error: 'gram.h' file not found > > [09:33:13.794] #include "gram.h" > > [09:33:13.794] ^~~~~~~~ > > [09:33:13.839] In file included from parser.c:25: > > [09:33:13.839] ./gramparse.h:29:10: fatal error: 'gram.h' file not found > > [09:33:13.839] #include "gram.h" > > [09:33:13.839] ^~~~~~~~ > > > > What I think is happening is that it was a mistake to remove > > parser/gram.h from the dependencies of backend/Makefile's > > generated-headers target: that allows builds to proceed before > > gram.h has necessarily been created. The fact that it works > > at all for anybody says that there's another dependency path > > somewhere that causes bison to get run ... but, seemingly, > > that doesn't always happen soon enough in a parallel build. > > But why doesn't the below take care of it? > > > # Force these dependencies to be known even without dependency info built: > > gram.o scan.o parser.o: gram.h > > The only file including gram.h is gramparse.h, which in turn is only included > by parser.c, scan.l, gram.y. So this should suffice. Ah, I see: The problem is compilation of .c -> .bc files, not -> .o, so it only happens with llvm enabled. So far that was just taken care of by the generated-headers dependency, but it's more granular now... Since the bison aspect is quite slow, it'd probably be nicer to not include it in generated-headers? The most general solution I can see would be diff --git i/src/backend/common.mk w/src/backend/common.mk index fa96a82b1a0..61861f5c7eb 100644 --- i/src/backend/common.mk +++ w/src/backend/common.mk @@ -23,6 +23,7 @@ objfiles.txt: Makefile $(SUBDIROBJS) $(OBJS) ifeq ($(with_llvm), yes) objfiles.txt: $(patsubst %.o,%.bc, $(OBJS)) +$(patsubst %.o,%.bc, $(OBJS)): $(OBJS) endif # make function to expand objfiles.txt contents Greetings, Andres Freund
On Thu, Sep 15, 2022 at 4:04 AM Andres Freund <andres@anarazel.de> wrote: > > The most general solution I can see would be > > diff --git i/src/backend/common.mk w/src/backend/common.mk > index fa96a82b1a0..61861f5c7eb 100644 > --- i/src/backend/common.mk > +++ w/src/backend/common.mk > @@ -23,6 +23,7 @@ objfiles.txt: Makefile $(SUBDIROBJS) $(OBJS) > > ifeq ($(with_llvm), yes) > objfiles.txt: $(patsubst %.o,%.bc, $(OBJS)) > +$(patsubst %.o,%.bc, $(OBJS)): $(OBJS) > endif Since there have been no other ideas in the past few hours, I will push this but it will be a blind attempt since it seems sporadic and doesn't happen to reproduce for me. -- John Naylor EDB: http://www.enterprisedb.com
Hi, On 2022-09-15 10:52:31 +0700, John Naylor wrote: > On Thu, Sep 15, 2022 at 4:04 AM Andres Freund <andres@anarazel.de> wrote: > > > > The most general solution I can see would be > > > > diff --git i/src/backend/common.mk w/src/backend/common.mk > > index fa96a82b1a0..61861f5c7eb 100644 > > --- i/src/backend/common.mk > > +++ w/src/backend/common.mk > > @@ -23,6 +23,7 @@ objfiles.txt: Makefile $(SUBDIROBJS) $(OBJS) > > > > ifeq ($(with_llvm), yes) > > objfiles.txt: $(patsubst %.o,%.bc, $(OBJS)) > > +$(patsubst %.o,%.bc, $(OBJS)): $(OBJS) > > endif > > Since there have been no other ideas in the past few hours, I will > push this but it will be a blind attempt since it seems sporadic and > doesn't happen to reproduce for me. WFM. FWIW, you can reproduce it with rm -rf .deps/ gram.c scan.c *.o *.bc && make parser.bc in src/backend/parser. Greetings, Andres Freund