Re: the ScriptingMojo

On 08/23/20 10:27, Kartik Ohri wrote:
> Oh, no. I actually had forgotten to push the changes😅 . I have pushed the
> changes now.

That looks like it is shaping up nicely.

- A nice thing about the 'configuration' elements is that they are already
JavaScript objects, even before being made into magical AbstractPGXS
instances. They can have methods that aren't required by AbstractPGXS
and those can be called early in JS. I think probe() is a good candidate
for that. The calling code could get the "Windows" instance, for example,
and call its probe method, which could return a different object to be
used as the template:

Windows : { probe: function() {
return configuration[(...) ? 'Windows-MSVC' : 'Windows-MinGW-w64']; }
}

'Windows-MSVC' : { compile: ..., link: ... }

'Windows-MinGW-w64' : { compile: ..., link: ... }

Or, probing could work in the Java ServiceLoader kind of way, where the
dispatch code would simply get the OS name and call probe(osname) on every
configuration item until one returned true, and then instantiate that one
as an AbstractPGXS.

configuration = [
{ name: "Linux", probe: function(os) { return name == os; }
compile: ..., link: ... },
{ name: "Max OS X", probe: function(os) { return name == os; }
compile: ..., link: ... },
{ name: "Windows-MSVC",
probe: function(os) {return os.contains("Windows")&&looks_like_MSVC;},
compile: ..., link: ... },
{ name: "Windows-MinGW-w64",
probe: function(os) {return os.contains("Windows")&&looks_like_MGW;},
compile: ..., link: ... }
];

I am starting to like that idea better, as it simplifies the dispatcher.
For that approach, I think configuration should be a list rather than a
map, so that the probe tests happen in a predictable order.

- I don't think formatDefines and formatIncludes make good PGXSUtils
methods. The syntax could vary across compilers (might it use /D or /I
on a Windows compiler, for example?). So I think they make better
AbstractPGXS methods that can be overridden in the configuration.
There could be a built-in implementation in AbstractPGXS using -D and -I.
Defines should be provided as a map (key to value) and the option string
with = sign should be produced by formatting.

- compile() should probably just have another parameter for the list
of .c files, so that only has to be built in one place in the common
code. compile() for some platform could still add to the list, if
there were a special porting file for that platform, or something.
Similarly with link(), I think the list of object files should be a
parameter.

- The Maven-interpolated ${...} properties are convenient for getting
stuff working, but will eventually have to go, as in f413312.

- The hard-coded compile and link options should eventually be teased
apart into the ones that really are hard-coded in PostgreSQL's PGXS
makefiles and the ones that come from pg_config CC, CFLAGS, CFLAGS_SL,
CPPFLAGS, LDFLAGS, LDFLAGS_SL, and so on.

- Those pg_config values don't have to be parsed /completely/, that is,
I don't see any need to figure out which elements represent defines
or includes, and parse those into bare maps or lists to combine with
our own defines or includes. The ones coming from pg_config can be
assumed to be already in the right format for the compiler used, and
just kept in the form of command arguments, while we format and add
the ones we are adding.

- They do need to be parsed into lists though, which on Linux can be
shown by a little experimentation to require splitting on whitespace
but not between ' characters (and of course, after splitting, the '
characters that functioned as quoting should not remain in the values).
(To experiment, it is enough to make a PostgreSQL build and specify a
weird value for --prefix or something, and look at what pg_config
produces.) The universal next question, when talking about a format
that uses ' for quoting is "what is the rule for values that
contain ' ?" and from my experiments on Linux, the rule is "the
PostgreSQL build itself breaks" so I don't think we have to cover
that case. :)

- The same experimenting should probably be done on Windows to see what
rules are used there. So I think getPgConfigPropertyAsList needs to be
an AbstractPGXS method. It can call the existing getPgConfigProperty
to get the raw string, but might use platform-specific rules to break
it into a list. AbstractPGXS might supply a version that uses the spaces
and single-quotes rule.

- There may need to be a removeRunPath method though, to cover one bit
of weirdness. The PostgreSQL build seems to always leave a linker
option in LDFLAGS to set a runpath to LIBDIR, which should not be
necessary when building a PostgreSQL extension, as PL/Java is.
(An extension shared object gets loaded *into* an already running
PostgreSQL server, which already knows where its other libraries are.)

It has to be an AbstractPGXS method because the exact option syntax
to look for and remove from LDFLAGS will be platform/linker specific.
On Linux it looks like -Wl,-rpath,LIBDIR,--enable-new-dtags
where LIBDIR is the pg_config LIBDIR property. It might be quoted, so
of course the removal should happen after the splitting of LDFLAGS into
a list, which will have removed any ' characters used for quoting.

Regards,
-Chap