Re: pg_scanner - patch no.1 - Mailing list pgadmin-hackers

Hi

On Sat, Nov 10, 2012 at 2:51 PM, Vladimir Kokovic
<vladimir.kokovic@gmail.com> wrote:
> Hi,
>
> On 11/9/12, Dave Page <dpage@pgadmin.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