Re: PL/Java new build plugin

On 08/11/20 14:49, Kartik Ohri wrote:
> I have currently added this
> https://github.com/amCap1712/pljava/commit/174dd71365986094a500d6dda4c832de78f90783.
> Is this fine? I had to change the way error handling worked to avoid
> creating a new functional interface.

That looks like some of the right ideas shaping up.

You definitely do NOT want "arguments = String.join(" ", argumentsList)".
That's exactly how to create a mess that cannot be correctly reparsed
into the original arguments, depending on what characters they contain.

The proper model to think in always a list of individual arguments. That
is the model the Java ProcessBuilder uses, and it is also the true native
model in POSIX (and so in Linux and Mac OS).

The native model in Windows is a single joined command line that the
receiving program then has to reparse to get the original individual
arguments correctly back, and the Java runtime itself will create the
joined string for you (you still pass individual arguments to
ProcessBuilder), but it needs help to do that correctly; I hope you are
reviewing the code in Node.java that I mentioned earlier. The point is to
do this transparently and correctly, so you start with a list of arguments,
and the invoked program ultimately has a list of arguments, and that's
the same as the starting list.

Regarding the subprocess stdout and stderr, as a way of doing things in
stages, I would suggest starting as simply as possible: just set them
both to INHERIT, so they go straight to our own stdout and stderr, and
we don't have to do anything with them, and can concentrate on getting
the build working.

As a later step, we could revisit that to capture output and funnel it
into Maven's logging, probably applying some filtering first, to decide
what should be logged as info, warn, or error (and ideally do a better
job of that than nar-maven-plugin does).

Capturing all of the output in a readAllBytes() is not the usual way
to do that, because it's preferable to see the messages as they happen
rather than all at the end (and if a process happens to generate a whole
lot of output, readAllBytes uses a lot of memory).

In that way, that makes this task different from getPgConfigProperty,
where the point really was to just read all of the output from pg_config
and return it. I see now that you have refactored that method in terms
of this one, but that may just complicate things. It is ok to have a
couple of different methods using ProcessBuilder but in different ways,
as "run pg_config and return its output" and "run this arbitrary compiler
or linker with its output going to the log" are sufficiently different
tasks.

Regards,
-Chap