pgsql: Properly install gram.h on MSVC builds
Properly install gram.h on MSVC builds
This file is now needed by pgAdmin builds, which started
failing since it was missing in the installer builds.
Branch
------
master
Details
-------
http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=3457514c2d9bac552d4caeb1d3ac5a8d03d3a439
Modified Files
--------------
src/tools/msvc/Install.pm | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
Magnus Hagander <magnus@hagander.net> writes:
Properly install gram.h on MSVC builds
This file is now needed by pgAdmin builds, which started
failing since it was missing in the installer builds.
I'd like to protest this patch as misguided. AFAICS it is a *seriously*
bad idea for pgAdmin to be including gram.h --- we don't even want most
of the backend to include that, let alone external pieces of code. It's
not stable enough. See the note in gramparse.h.
To my way of thinking, not installing that file is a fine idea.
What we really need to be asking is why the pgAdmin folks think
they should be including it.
regards, tom lane
On Sun, Jan 9, 2011 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
Properly install gram.h on MSVC builds
This file is now needed by pgAdmin builds, which started
failing since it was missing in the installer builds.I'd like to protest this patch as misguided. AFAICS it is a *seriously*
Uh, we install the file on Unix, so we should do the same on Windows.
bad idea for pgAdmin to be including gram.h --- we don't even want most
of the backend to include that, let alone external pieces of code. It's
not stable enough. See the note in gramparse.h.
Whether that's misguided is another thing :-)
To my way of thinking, not installing that file is a fine idea.
What we really need to be asking is why the pgAdmin folks think
they should be including it.
It is required in order to pull kwlist.h, which is used to determine
which keywords to highlight in the SQL editor (and other places that
do syntax highlighting). It never actually uses the values, but it
needs the file for kwlist.h to compile.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
On Sun, Jan 9, 2011 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'd like to protest this patch as misguided. �AFAICS it is a *seriously*
Uh, we install the file on Unix, so we should do the same on Windows.
Well, my idea of how to fix that would be the other way 'round.
What we really need to be asking is why the pgAdmin folks think
they should be including it.
It is required in order to pull kwlist.h,
No, it is not required. What they should be doing is #define'ing
PG_KEYWORD() in a way that ignores its second argument. See pg_dump's
keywords.c for an example of safe usage.
If we allow this to stand then we are going to find people complaining
that we've broken ABI anytime we make a minor change in the grammar.
I repeat: it's a SERIOUSLY bad idea.
regards, tom lane
On Sun, Jan 9, 2011 at 17:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Sun, Jan 9, 2011 at 17:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'd like to protest this patch as misguided. AFAICS it is a *seriously*
Uh, we install the file on Unix, so we should do the same on Windows.
Well, my idea of how to fix that would be the other way 'round.
Sure, then it's at least consistent...
What we really need to be asking is why the pgAdmin folks think
they should be including it.It is required in order to pull kwlist.h,
No, it is not required. What they should be doing is #define'ing
PG_KEYWORD() in a way that ignores its second argument. See pg_dump's
keywords.c for an example of safe usage.
Ahh, good point.
And yes, that seems to work for pgadmin. I'll commit a patch there as
soon as I've finished testing, at which point it won't be required
anymore.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/