Subject: Re: PL/Java new build plugin
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <5EF7B265.3060907@anastigmatix.net>
 <CAASLQ4P6vTfRNVnreNp7Su8VmyhA+wKOYDY8iUfa7eSoogFkXA@mail.gmail.com>
 <5EFA6C22.8060006@anastigmatix.net> <5EFC18D9.3090108@anastigmatix.net>
 <CAASLQ4N0KeUx8PaJMKbHWiVtLvBo3QNACdHgOyk72q30976d+g@mail.gmail.com>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5EFD41B1.30009@anastigmatix.net>
Date: Wed, 1 Jul 2020 22:08:49 -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: 
 <CAASLQ4N0KeUx8PaJMKbHWiVtLvBo3QNACdHgOyk72q30976d+g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 07/01/20 15:31, Kartik Ohri wrote:

> The branch I am using for developing can be viewed at
> https://github.com/amCap1712/pljava/tree/plugin. As you can see I have
> modified the execute method to set a property and print it later. It works
> for me. How should we proceed ?

It seems like there are multiple sensible places to begin. If a nice safe
incremental one appeals to you, that could be to keep going with properties,
and make sure you can set one in this plugin and use it for something
in pljava-so and pljava-packaging.

I think (but it's worth checking this) that if you just do
project.getProperties().setProperty(...), it will only be set in
the current project, presumably pljava-so where the plugin is being
invoked.

It might be necessary to insert getParent() and set the property on
the parent project, with the hope that both pljava-so and pljava-packaging
can see it there.

I am not certain in advance that that's sufficient (does a subproject
see properties that you only set on its parent?). If not, it might be
necessary to do something hybrid, like have this plugin set the property
both on its own project and the parent, and have some code in
pljava-packaging /get/ the property from the parent.

But I would /like/ the answer to be that setting a property on the parent
just works, so I would try that first.

A next step could be to keep everything else unchanged, but see if you
can eliminate the code in pljava-so/build.xml that writes the
pgsql.properties file, and the places in pljava-{so,packaging}/pom.xml
that read that file, and see if everything works with the new plugin
doing the work of finding the right values and setting those properties.
That could quickly make pljava-so/build.xml just go away.

As for the work that build.xml currently does, I think setting the
properties based on pg_config can easily be hardcoded in the plugin:
we know we will always want to do that, and won't really change our
minds about how to do it.

The more platform-specific stuff, like finding the MSVC version on
Windows, I don't think should be hardcoded. I could see hardcoding the
basic checks of "what OS and architecture am I on", which don't have to
look any farther than the Java system properties. Then, as part of the
configurable per-platform 'recipes' we should eventually have, maybe
one optional element can be a JavaScript function that probes the
environment for further details. So a platform:windows recipe would
have a probe function that would try to figure out if this is MSVC
or MinGW, and what version and so on, and that would be visible in
the POM as a configurable little JavaScript snippet.

It would be ok to hardcode it at first, work on the embedded-JavaScript
details later and factor it out then.

It might be safe to just stop computing an MSVC.RINT property. That one
seems kind of goofy. I think it mattered for Visual Studio 2012 and
earlier combined with PostgreSQL 9.3 and earlier. I think it can go away.

- - -

The formatting of the files is better now, but still not exactly right:
the XML files are now indented on four-column multiples, which is right,
but they are still indented using space characters instead of tabs set
at 4 columns.

A quick pipe through 'unexpand -t 4 --first-only' would fix that right
up for the existing content, so then it's just a matter of tweaking
editor settings so it stays right.

There are still some occurrences of pljava.pgxs rather than pljava-pgxs,
including the name of the directory itself.

Regards,
-Chap