Subject: Re: the ScriptingMojo
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <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>
 <5F42A71F.8000002@anastigmatix.net>
 <CAASLQ4PkP017rtyUswKO_TCiNmnica-nrQHBgggFw-U_9MstUg@mail.gmail.com>
 <5F459B6E.3020803@anastigmatix.net>
 <CAASLQ4P47TNiEsuj3ga-vuYeCSBJZmHrPM-49E-X=0T5RTmhKw@mail.gmail.com>
 <5F45EE7F.5020609@anastigmatix.net>
 <CAASLQ4MH5JaVbrG9O_S+9dnp=tj_8N3jArxxpf-dJD1ezd5SfQ@mail.gmail.com>
 <5F46B70E.6070601@anastigmatix.net> <5F46DC70.1080404@anastigmatix.net>
 <CAASLQ4Npudth33qcg8GHaKd0amJ8oSQro_sxgb72VkbkBj+oXQ@mail.gmail.com>
 <CAASLQ4NcDozHC8J8RAAosc-X1k7JSTxUdaz-1vFLhpdE1nuXqg@mail.gmail.com>
 <5F484E53.1010209@anastigmatix.net>
 <CAASLQ4Nqyt0DR=AhxS2zrwBgz1arWJ8aCkq81pBmf99aJ7y2RA@mail.gmail.com>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5F48F4E1.4020400@anastigmatix.net>
Date: Fri, 28 Aug 2020 08:13:21 -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: 
 <CAASLQ4Nqyt0DR=AhxS2zrwBgz1arWJ8aCkq81pBmf99aJ7y2RA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08/27/20 23:59, Kartik Ohri wrote:
>> How about compile() and link() returning int, and just returning the
>> exit status of the command, so the common code can compare to zero
>> or throw MojoFailureException. Just info()ing it isn't enough; yes,
>> the build would probably fail at a later step, but it wouldn't be
>> immediately clear where it had gone wrong.
>>
> Regarding this, I was thinking if we could return a ProcessBuilder rather
> and execute the command in the common code. The only issue I thought that
> may arise is when we try to PIPE the output of the ProcessBuilder because I
> am sure how that is done. What do you think ?

I think that when reading the piped output from the process, the different
platform implementations will want to process it differently, because the
messages from different compilers will match different patterns. That
might involve having runCommand take another parameter that is a filter
for messages, and each implementation would pass its own filter. That
means it is probably easier to continue invoking runCommand within the
implementation.

But returning the exit code so the common code can check it and throw
MojoFailureException would be easy.

It occurs to me that now that the pljava.libjvmdefault business is
implemented in the script, it would be worth testing to make sure
it works on all platforms. After #289 is merged, that would just
mean passing the -Dpljava.libjvmdefault="$libjvm" on the mvn command
line (after computing the right $libjvm), and removing the
pljava.libjvm_location settings from [1] and [2], and making sure
everything still passes.

Also, unit testing. Have you added jUnit tests in a Maven-built
project before? It is fairly straightforward; you can add some
Java files under src/test/java and if Maven finds any there, it
will build them and run them as tests.

There are, so far, very few examples in the PL/Java codebase, so there
is not a very high existing bar for you to meet, but it would be
worthwhile to start the ball rolling by having at least one file in
pljava-pgxs/src/test/java with a small handful of tests in it.

An existing example you can look at is in pljava-api/src/test/java.

One good candidate for testing would be getPgConfigPropertyAsList.
It should be tested on some systematically-constructed examples
as well as on a real-world example like [3].

Regards,
-Chap


[1] https://github.com/tada/pljava/blob/943152b/.travis.yml#L112
[2] https://github.com/tada/pljava/blob/943152b/appveyor.yml#L130
[3] https://travis-ci.com/github/amCap1712/pljava/jobs/378459461#L2217