Subject: Re: the ScriptingMojo
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <5F2EB313.8090706@anastigmatix.net>
 <CAASLQ4M0uvNj0EyMWWNsizT1KtnWx0WFbk89ud9EVHZbu12LMg@mail.gmail.com>
 <5F2EC4F4.3080608@anastigmatix.net>
 <CAASLQ4M3pR+ZL+uPvcQ-dAEL2KdH_PGe-_pBhsjvOgN5qvf+aA@mail.gmail.com>
 <5F31EFEC.1080609@anastigmatix.net>
 <CAASLQ4NU9v=nuLDG-LneC68LuqKRXchFckU7mCZT9YQDNKqwvg@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>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5F3DA48D.7090408@anastigmatix.net>
Date: Wed, 19 Aug 2020 18:15:41 -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: <5F3D43BD.4020900@anastigmatix.net>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Kartik,

Taking an early look:

- I think the runCommand method has kind of a misleading name for what it
  does, as it only gives you a ProcessBuilder for you to set up for
  running your command. Maybe it would be better to just call that one
  ... processBuilder?


- All the same, I think a runCommand method would also be good to have;
  that would be what you call after getting your ProcessBuilder from
  processBuilder and configuring it for the command you want.

  That method would take care of ProcessBuilder.start() and waiting for
  completion and checking the exit status. I think there would be few
  times the script code would want to do those things separately (and if
  it ever did, it could anyway; no one is forcing it to use runCommand).

  Later on, it will be even better to have runCommand, as we will
  eventually want not to set the process stdout and stderr to INHERIT,
  but to have them as pipes and read them. At that point, runCommand should
  have an additional parameter, some JavaScript lambda that will be
  called back with messages read from the process, and will classify the
  messages as error/warning/info and pass them to the right Maven log
  method. That should be done in script because the patterns used to
  classify the messages will almost certainly need to be tailored
  for each compiler/linker.

  That will be a little tricky, because runCommand will need to use
  more than one thread, to be able to read both streams from the process
  and wait for the process to exit. But the script engine does not
  want to be shared between threads. So runCommand might create a
  concurrent queue, start threads that will read the process output and
  place messages on the queue, while runCommand just reads from the queue
  and passes messages to the script callback, until the process is done.
  But that can be later; just letting stdout and stderr be INHERIT is
  fine for now.


- Once there is processBuilder and runCommand, processBuilder can set a
  default for the working directory before it returns the ProcessBuilder,
  but I would move the existence test and directory creation into
  runCommand; no need to do the work up front when the caller might
  change the directory setting.


- I was thinking of simplifying the parameters to processBuilder, to not
  need the caller to separately pass the command string and a list of the
  rest of the arguments. An idea that just hit me was why not just
  processBuilder(Consumer<List<String>>)? A script could use it like:

  var pb = processBuilder(function(l) {
    l.add(command);
    l.addAll(stuff);
    l.add(thing);
    ...
  });

  so the script would avoid the messiness of allocating its own ArrayList
  and so on. On the Java side that's just

  pb = new ProcessBuilder();
  callback.accept(pb.command());
  ...
  return pb;


Once there is a platform/OS 'dispatcher object' structure, I think the
common methods compile(...), link(...), and so on, need their parameter
lists carefully thought out. One parameter to compile(), for example,
should just be a list of includes, without the -I; the method needs to
know the right syntax for its compiler. Another parameter should be a
simple map <name,value> of defines; again the method needs to know if
-Dname=value is right for its compiler. Similar considerations for link().

It might be worthwhile to define an abstract class in Java that has
the necessary methods; it could also supply some other useful methods
or default implementations. I think the javascript could then do something
like:

var platforms = {
  'Linux': { compile: function(...) { ... }, link: function... },
  'Windows' : { ... },
  ...
}

var impl = new AbstractPGXS(platforms['Linux']);

which would enforce that the needed methods were all present, and the
ones supplied in Java would be naturally available too. I have not
tried this out to make sure it does what I expect in both nashorn
and graal.

Regards,
-Chap