Nils Goroll napsal(a):
> Hi Paul, Mayuresh and all,
>
> what started as a little change to fix a specific requirement has now evolved
> into a general overhaul of the SFW postgres Make- and install files.
>
> I did this because Paul suggested to use a METADATA file, and looking at
> other
> Makefiles in the sfw consolidation, I realized that the postgres build
> environment would profit from a general overhaul to improve clarity and to
> reduce the amount of duplicate information.
>
> The webrev is at
> http://cr.opensolaris.org/~nigoroll/postgres_overhaul_and_defect3298_missing_pgxs_files-rd3/index.html
>
>
> I suggest to do the following:
>
> * I will comment on all changes I made below, please let me know what you
> think
> and review them.
>
> * If necessary, additional bugs could be opened for the various changes (e.g.
> "postgres makefiles need overhaul" or "64bit bin and lib should be ${ARCH64}")
>
> * So far I have neither tested the changes on sparcv9, nor have I built
> packages
> and installed them (there should be an easy way to build packages which I
> don't
> know - how?). Would anyone help with these?
>
> * After we have agreed on what should be integrated and after changes due to
> reviews have been made, we should have a final review phase.
>
> I will first reply to Pauls and Mayureshs comments and then give a general
> overview of all changes.
>
> Paul Cunningham wrote:
>
> > Normally the developer would checkin their changes into their local ws
> > before generating the webrev - thus they get changed. But I'm not sure
> > how you are working outside the SWAN so you may not be able to do that.
>
> Sun internal teamware repositories are not accessible from outside SWAN and I
> don't see any sense in using another SCM just for myself, so I use zfs
> snapshots
> as a rudimentary replacement for an SCM.
>
> #ident tags will get updated when a sponsor checks in the changes, so I don't
> understand why I should change them just for the webrev.
>
> > Roland Mainz wrote:
> > > - use "env - ..." and not "env ..." in the Makefiles
> > > to make sure "configure" & "make" only see the env
> > > variables they should really get (and not pick-up
> > > any random env variable)
>
> done
>
> > > - use either $(SHELL) or /usr/bin/bash for "configure"
> > > calls (so we know which one is used and "configure"
> > > doesn't pick one itself)
>
> done
>
> > > Use the method define in Makefile.master
> > > since you have a standard METADATA file.
>
> done, see below for more. And I still think we should have Metadata files in
> Makefile format.
>
> Mayuresh Nirhali wrote:
>
>> 1. SFW consolidation also includes version 8.2 of postgres and the bug
>> 3298 also applies to that version.
>> I think it makes sense to fix 8.2 as well with the fix for this bug. Let
>> me know what you think.
>
> Absolutely. I would very much like to get this done and integrated for 8.3
> fist
> and would then implement the same or similar changes for 8.2 as well. At this
> point, maintaining two sets of almost identical changes would just mean more
> work.
>
>> 2. I see following files in the 32 bit pgxs directory, please check if
>> they should be present in 64 bit directories as well.
>> d none usr/postgres/8.3/lib/pgxs/src/test 755 root bin
>> d none usr/postgres/8.3/lib/pgxs/src/test/regress 755 root bin
>> f none usr/postgres/8.3/lib/pgxs/src/test/regress/pg_regress 555 root bin
>
> done
>
>> 3. Another very minor comment, you should include following line in both
>> platform specific proto files.
>> d none usr/postgres/8.3/lib/amd64/pgxs 755 root bin
>
> oops, done
>
> Comments on the other changes / overhaul:
>
> === SYSCONFDIR ===
>
> (diff from Makefile.global)
>
> -sysconfdir := /usr/postgres/8.3/etc
> +sysconfdir := /etc/pgsql/8.3
>
> I don't think any config directory should be under /usr (will break with
> diskless clients for instance, see filesystem(5)), so I changed the path and
> introduced the new package SUNWpostgr-83-client-root, containing only the
> sample
> pgsql rc-file.
>
> I could come up with arguments for calling the directory both /etc/postgres
> and
> /etc/pgsql. pgsql is what postgres.org calls it, postgres is the name of the
> package. IMHO, we should stick to the names the original authors have chosen,
> so
> I think pgsql is better, but please let me know what you think.
No, please. /usr/postgres/8.3/etc contains only example configuration and it is
read only. I don't see any benefit to move the directory into /etc. And it is
not so easy introduce new package and new directory... (see ARC processes)
> === CFLAGS ===
>
> (diff from Makefile.global)
>
> CC = /opt/SunStudioExpress/bin/cc -Xa
> GCC =
> -CFLAGS = -xc99=none -xCC
> +CFLAGS = -xO3 -xarch=386 -xchip=pentium -xspace -Xa -xildoff -xc99=all
> -xc99=none -xCC
>
> In the oritinal Makefile.sfw, the default cflags were used everywhere
> (also for the 64bit version), BUT for the target $(VER)/config.status
>
> Thus it does look like an error that the (sensible) default CFLAGS are
> not used, but I might not be aware of a good reason for choosing the
> minimal CFLAGS.
It look likes a bug since 8.1 integration. Fortunately, compiler setting seems
ok. Check pg_config before and after your changes. They should not be different.
> === bindir, libdir ===
>
> Originally, bindir and libdir were chosen as .../64. My understanding is that
> those directories should be ../$(MACH64), either amd64 or sparcv9, which is
> what
> I've made them to be now. These directories existed already and .../64 was
> only
> a symlink, so the change is more making the build environment more consistent
> with the path name scheme.
No problem.
> === Comments on changed files ===
>
> * Makefile.master
> - added some path definitions which seemed to be missing
> - cleaned up CFGSFW definitions
> - added mussing definitions to ROOTSFW* (plus one fix)
Please, create new CR for these changes I think it is not directly related
postgreSQL and they should be discussed separately.
> * usr/src/cmd/postgres/postgresql-8.3/Makefile.sfw
> - general cleanup, use macros for all environment settings and configure
> args
>
> * usr/src/cmd/postgres/postgresql-8.3/install-sfw
> * usr/src/cmd/postgres/postgresql-8.3/install-sfw-64
> * usr/src/cmd/postgres/postgresql-8.3/libpqxx/install-sfw
>
> IMHO, all of the install scripts should really fail upon error, so I have
> changed their invocation to $(SH) -e. Made script ignore all uncritical
> errors.
> Fixed bogus error output for recreating symlinks by using resymlink function
>
> * usr/src/cmd/postgres/postgresql-8.3/libpqxx/Makefile.sfw
>
> - general cleanup
My comments:
METADATA
- version 8.3.4 is already in SFWNV (build 100)
- PROGRAM name is PostgreSQL not postgres
Makefile.sfw:
- 35 # these need to be synced manually with VERSION in METADATA
36 PGMAJVER= 8.3
37 PGMINVER= $(PGMAJVER).3
I think. It should be possible to derive it from $(COMPONENT_VERSION:sh)
At least PGMINVER = $(COMPONENT_VERSION:sh)
- @find . -name core -exec rm -f {} \;
please, remove these lines, they are rest of old workaround
- mv tmp/post*$(PGMINVER) $(VER64); rm -rf tmp
I prefer mv tmp/$(VER) $(VER64); rm -rf tmp
(same for 32bit version)
With regards Zdenek
--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql
