Subject: Re: the ScriptingMojo
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <5F368380.9070203@anastigmatix.net>
 <CAASLQ4NgH+tXbdUSA8xDh2ct3gAS3xa8V7=E0gcYd+d=m8kujw@mail.gmail.com>
 <5F368A99.6070709@anastigmatix.net> <5F36965C.60707@anastigmatix.net>
 <5F3A7DC6.8090905@anastigmatix.net>
 <CAASLQ4PEgo5Nj0EVsOfGoB96dQFXfFYxrrB0rS1wdad7x_E5fA@mail.gmail.com>
 <CAASLQ4OowXOmnbbbT0Wh1ZkB4h=6JQfC61ojmN07EYnO0QCcow@mail.gmail.com>
 <5F3D32E9.80504@anastigmatix.net> <5F3D36BE.70703@anastigmatix.net>
 <5F3D38F8.7050907@anastigmatix.net> <5F3D3A75.5010304@anastigmatix.net>
 <5F3D3D2E.4070109@anastigmatix.net> <5F3D43BD.4020900@anastigmatix.net>
 <5F3DA48D.7090408@anastigmatix.net>
 <CAASLQ4MBWQeFLdAPNcsgcpLyekU5X=ExQUuGgE49AUFdEYJr8g@mail.gmail.com>
 <5F41943D.9080707@anastigmatix.net>
 <CAASLQ4P5oEx0DLoDLEOAkZSwffU2eS2+NKbevgYqpknHGucaTw@mail.gmail.com>
 <5F427604.5010103@anastigmatix.net>
 <CAASLQ4Nr3MPQuZziVU+89ULZ=BgQjaPPnCLsyPR3mhW-eWoOBw@mail.gmail.com>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5F42A71F.8000002@anastigmatix.net>
Date: Sun, 23 Aug 2020 13:27:59 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
MIME-Version: 1.0
In-Reply-To: 
 <CAASLQ4Nr3MPQuZziVU+89ULZ=BgQjaPPnCLsyPR3mhW-eWoOBw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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