Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k5d5v-0007WW-O1 for pljava-dev@arkaria.postgresql.org; Tue, 11 Aug 2020 22:50:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1k5d5u-0005BY-NZ for pljava-dev@arkaria.postgresql.org; Tue, 11 Aug 2020 22:50:10 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k5d5u-0005BR-FB for pljava-dev@lists.postgresql.org; Tue, 11 Aug 2020 22:50:10 +0000 Received: from mail-ej1-x635.google.com ([2a00:1450:4864:20::635]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1k5d5r-0002GQ-P1 for pljava-dev@lists.postgresql.org; Tue, 11 Aug 2020 22:50:10 +0000 Received: by mail-ej1-x635.google.com with SMTP id bo3so146721ejb.11 for ; Tue, 11 Aug 2020 15:50:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1ECZwzmUCgWyP6F2yMH7vz3MdBUdI+x917ds2wyxC+U=; b=CIAKzU39K49GhMVJBhge9Qj7AnLl9Z+b3p3CANZIDQyv4J0C9xQE9rm+hydWghxTT/ IryAHJO6dgTY/njiKRsTpYP6ELdenGeM+FQnrRMGd4PMbNV4vI9suKKOXHRBEXur+Cyd 16SwEjpHMiqRyr6e+GL/dcpG94sSBKho5aJM6oyKqGD5srvNcRriYq6TnrU12hDjX45W L2mjPQs/So/JLflNUmQPEHAf/cjHUzJoH4ohQSmU+BeymaAMGND+LNqGUQgRMKwOetDd TCFoVCHcpUTqQoTV0kwden5YVBMtqgzkOVueWeCLYfzW6zWi66nf6GqGDor9lYScrZvu on0Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1ECZwzmUCgWyP6F2yMH7vz3MdBUdI+x917ds2wyxC+U=; b=qIR6W5VD58KJgQL+9HWLbRNtJ2UDarTFvwJ7CzPEQ1NlD7oI+vIqfEV0UDvbLCCGXh 1YzIBmOvSgx6W1WJTE7cmXTvZS0sqXyEotcuqVClf6N1Uss1dy7hss+M33GpHWlyoshR KBXZn2oUw3juUto1yo8LDl5Fw7GQl5U+vkUTlHNdmI1NZdNE5wLlTRevlLuJtxp7OT6N rBheUQ31JhpDpSTOBHyUFXHZko8CmxBIV+3fcWScb3aFy4flKg8QP5CZN+tsi+rN7x2l c9OMAYNchh49ZgC15SMZDYu79MLnzjGHNAdAEGQWgfAb832bE6bMMPmaUSecMLTLtjbv 8hlg== X-Gm-Message-State: AOAM5305p9Rr9HsRugjiS6yAj4VyyFvyFGlkIUOLyLdYIl8Q6WPdkfIw ksPRJbwxOI1xMRFRS3evX78q1yvEqVBviODOdCVmdfmna7M= X-Google-Smtp-Source: ABdhPJwFaWGac/bYEDQDX1sfItfXtoKd6B0hKUFuZrvO9m0IPRPUpgrj017exFQISh5v91IprqQ7nn3heHOZLPtqY2A= X-Received: by 2002:a17:907:4303:: with SMTP id nh3mr29325124ejb.520.1597186205667; Tue, 11 Aug 2020 15:50:05 -0700 (PDT) MIME-Version: 1.0 References: <5F232EBB.9030208@anastigmatix.net> <5F278CCE.1090006@anastigmatix.net> <5F2897AF.9040909@anastigmatix.net> <5F2A29E4.2000803@anastigmatix.net> <5F2EB313.8090706@anastigmatix.net> <5F2EE97B.1030108@anastigmatix.net> <5F30B425.4020402@anastigmatix.net> <5F331877.50407@anastigmatix.net> In-Reply-To: <5F331877.50407@anastigmatix.net> From: Kartik Ohri Date: Wed, 12 Aug 2020 04:19:53 +0530 Message-ID: Subject: Re: PL/Java new build plugin To: Chapman Flack Cc: pljava-dev@lists.postgresql.org Content-Type: multipart/alternative; boundary="0000000000006dd55e05aca1e405" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000006dd55e05aca1e405 Content-Type: text/plain; charset="UTF-8" On Wed, Aug 12, 2020 at 3:45 AM Chapman Flack 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 --0000000000006dd55e05aca1e405 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Wed, Aug 12, 2020 at 3:45 AM Chapman F= lack <chap@anastigmatix.net= > wrote:
On 08/11/20 14:49, Kartik Ohri wrote:
> I have currently added this
> https://gith= ub.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 =3D String.join(" ", a= rgumentsList)".
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<= br> the same as the starting list.


Yes, I agree with you. I used String.j= oin because the ProcessBuilder documentation said that "here are operating systems where pr= ograms 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 in= corporate it.=C2=A0

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<= br> 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<= br> 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&= #39;ll look into this and fix this.
=C2=A0
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 arbit= rary compiler
or linker with its output going to the log" are sufficiently different=
tasks.

Yes, makes sense. I'll revert this change.=C2=A0<= br>

Regards,
-Chap

Thanks.
Regards,
<= div>Kartik=C2=A0
--0000000000006dd55e05aca1e405--