Subject: Re: the ScriptingMojo
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <5F3D3D2E.4070109@anastigmatix.net> <5F3D43BD.4020900@anastigmatix.net>
 <5F3DA48D.7090408@anastigmatix.net>
 <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>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5F484E53.1010209@anastigmatix.net>
Date: Thu, 27 Aug 2020 20:22:43 -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: 
 <CAASLQ4NcDozHC8J8RAAosc-X1k7JSTxUdaz-1vFLhpdE1nuXqg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 08/27/20 09:17, Kartik Ohri wrote:
> I was able to solve the issue. The dll file should was prefixed with lib
> and it seems that is wrong. Removing lib from the  dll name and it works
> fine.

I am looking over the code now.

I think the remaining profiles in -so/pom.xml can also be removed
(linkpglibs, needsrunpath, needsdefaultpgconfig). The first two were
always mysteries to me anyway; I had not seen a platform that needed
them, and nobody stepped forward to say "hey, mine does".

We now have CI covering all of the platforms that we have JavaScript
config entries for, and it's telling us we don't need those. So I would
say take them out (and those lengthy comments that go with them). The
story will still be told in the git history, and if anybody ever does
have a platform that needs that, a few lines of JavaScript and it's handled.
Sort of the way that should be.

(As for needsrunpath, if anything, I think we should be considering
a method to /remove/ a runpath option if we see it in LDFLAGS.)

The needsdefaultpgconfig was only there to say the property defaults
to 'pg_config' if not specified (12 lines of XML to say that??), and
that's what PGXSUtils.getPgConfigProperty already does, so that profile
can go away too.

You left the comment for the haslibjvmdefault profile when you removed
the profile. :)

The *FLAGS_SL and *FLAGS_EX properties are intended for shared libraries
and PostgreSQL extensions, respectively, and PL/Java is a shared library
and a PostgreSQL extension, so even if you typically see those properties
empty in pg_config, we should still be including them both at compile
and link time, just in case.

I would be using Paths.get and .resolve when setting things like
java_include and the base_includes, just to let the API do all the
thinking about separator characters. I notice that Windows doesn't
seem to complain with the forward slashes, but using the API
still seems to be preferable for staying as portable as possible.

I think it would be good for each configuration entry to start with a
name property, like name : 'Linux', even if it is never used for anything,
just to help reading the code. The selecting code could certainly also
do info('Using compile/link rules for ' + implementation.name) too.

It seems to me the behavior of formatIncludes should be like the
new behavior of formatDefines: not checking for whether the incoming
entry already starts with -I or /I, just assuming it is supplied the
plain value, and always adding the -I or /I.

I think it makes things clearer to handle options that take arguments
on a single line, like

  l.addAll(of("-bundle_loader", bindir.resolve("postgres")));
  l.addAll(of("-o",             "lib" + library_name + ".bundle"));

It's also fair to group a slew of fixed unrelated single options that way
too, "/MANIFEST", "/NOLOGO", "/DLL", "/SUBSYSTEM:CONSOLE" ...
just so the code isn't so vertically stretched.

The capital 'I' used for the list seems a little unconventional for
an ordinary local variable. I think my earlier example used a
lowercase l for list ... did that look like an I in your email font?

I am not convinced the common code at the bottom should always add -c
to flags. The Windows implementation is already adding /c (but not
deleting the -c). I think to be fair, if it's up to the Windows rule
to add its own /c, then the other rules should have to add their -c.

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.

I would add a blank line between implementations, and between methods
in an implementation, just to set them apart a little bit.

I think this <execution> isn't setting any properties anymore, so
its <id> can be changed to something like build-shared-object, and
there is no longer any reason it should run in initialize phase.
I think it should just omit the <phase>, and ScriptingMojo should
be changed to have defaultPhase = COMPILE.


I think that's all I've got for now.

Regards,
-Chap