From: | Dave Page <dpage(at)pgadmin(dot)org> |
---|---|
To: | Vladimir Kokovic <vladimir(dot)kokovic(at)gmail(dot)com> |
Cc: | pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_scanner - patch no.1 |
Date: | 2012-11-13 12:35:05 |
Message-ID: | CA+OCxow_Jj6fEyaJMLPsCvJEk-eM-Bn0w0XEmTmFCJrw6_qr-g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
Hi
On Sat, Nov 10, 2012 at 2:51 PM, Vladimir Kokovic
<vladimir(dot)kokovic(at)gmail(dot)com> wrote:
> Hi,
>
> On 11/9/12, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> I've spotted what I think are a few issues with the build system
>> changes, which will contribute to this, and possibly other problems we
>> may see in the future:
>>
>> - I believe the above issue is caused by not adding $OSX_ARCH to the
>> LDADD variable for pg_scanners. See /acinclude.m4, which sets this
>> sort of thing up and ensures we get the right flags passed to every
>> individual build in the code.
>
>
> Missing CFLAGS="$CFLAGS $OSX_ARCH" added.
OK.
>> - You shouldn't use ../xtra/pg_scanners in Makefiles, but
>> $(top_srcdir)/xtra/pg_scanners. You can use the png2c as a roughly
>> equivalent guide.
>
>
> pgAdmin make system is VPATH enabled.
> My build script 'build-debug.sh' works as expected:
Maybe, but that doesn't make needless inconsistency acceptable.
>> - Do you need to define the rules for each source file explicitly in
>> the Makefile? At minimum this should be a generic rule automatically
>> picking up all source files in the SOURCES list - ideally there should
>> be no explicit rules there at all.
>
>
> xtra/pg_scanners/Makefile.am is now simplified.
Hmm, that's definitely wrong - any parts of the pgadmin3 build target
should be in $SRC/pgadmin. Having it in xtra/ is acceptable if it's
being built as a library that then gets linked into other projects,
but if we're adding to pgadmin3_LDADD, then the code should definitely
not be in xtra/ but in pgadmin/. I'd prefer it not be a library
personally, so can you move it please?
>> Some other questions:
>>
>> - At some point we're going to need to go through all this on Windows
>> as well :-/. Do you have a Windows system to work on that when we get
>> to it?
>
>
> I am linux only man !
:-)
>> - Currently the code builds scanners with "92" in the name. Is this
>> intended to allow us to have multiple versions of the same scanner in
>> the future? If so, then it will also need to allow for Postgres Plus
>> Advanced Server and Greenplum DB scanners, which may have the same
>> versions but do support different syntax. If not, we should probably
>> just remove the "92" and make it as generic as possible.
>
>
> Name is optional, but folder name is xtra/pg_scanners/ which means the
> place for more than one.
> I like "92".
92 is fine - my point is that we support 3 different types of
PostgreSQL server, that each may have a 9.2 version with different
syntax, so if we're including the version number to allow multiple
scanners in the future, then we should also include something to
differentiate the forks - eg. pg92 for PostgreSQL, as92 for Postgres
Plus Advanced Server and gp92 for Greenplum Database.
>> - Can you please add a README file to xtra/pg_scanners that describes
>> the various files in there, and explains what needs to be updated to
>> move us to a new scanner from a different version of PostgreSQL?
>
>
> xtra/pg_scanners/README added.
> Please Dave, take look for some English error or bad terms. Thanks.
Sure, I'll do that during commit.
Thanks.
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Vladimir Kokovic | 2012-11-14 18:07:00 | Re: pg_scanner - patch no.1 |
Previous Message | Vladimir Kokovic | 2012-11-10 14:51:33 | Re: pg_scanner - patch no.1 |