Re: PL/Java new build plugin

On Wed, Aug 12, 2020 at 3:45 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> 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.
>
>
Yes, I agree with you. I used String.join because the ProcessBuilder
documentation said that "here are operating systems where programs are
expected to tokenize command line strings themselves - on such a system a
Java implementation might require commands to contain exactly two elements".
I was not sure if this was relevant but tried to incorporate it.

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).
>
>
Right, this is certainly desirable. I'll look into this and fix this.

> 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.
>
> Yes, makes sense. I'll revert this change.

Regards,
> -Chap
>

Thanks.
Regards,
Kartik