Subject: Re: PL/Java new build plugin
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <5F1F88E0.2050305@anastigmatix.net>
 <CAASLQ4PyG-YvKUTYdqfuQW8yGL+t_93b1i2CMTusoYHbZ0ZTMg@mail.gmail.com>
 <CAASLQ4MDUNP_4BAqK7ZtxtqG7ErH6skMKbNRvL0vE58wdRVxiQ@mail.gmail.com>
 <5F20E585.6010703@anastigmatix.net>
 <CAASLQ4PEcB-Rf1-vFtKCLNxVC6YZsSrzRUd_5TXZsPJp74vvcg@mail.gmail.com>
 <CAASLQ4M1dg+z+J44g77cvzSe3y8q01kWbvCcgoyNzA0j=apfAA@mail.gmail.com>
 <5F22DF79.2090701@anastigmatix.net>
 <CAASLQ4POay=ZLsdOb02n6NqwTn7MGyyWQprcCEo2Wm6Y6rStEw@mail.gmail.com>
 <5F230337.3040406@anastigmatix.net>
 <CAASLQ4Ny+VWWH=_AR+=7gjXinUScybraOJxBUaG5AmLZmx+NPQ@mail.gmail.com>
 <5F232EBB.9030208@anastigmatix.net>
 <CAASLQ4OknEQTpSeoBfyAZR1na-EyL+AtN9t0fPGyv5DfWJOSWA@mail.gmail.com>
 <CAASLQ4NHROvOq-rUz6Jn0dUd7oAq3_P655WNeqKksNBqFtAGgw@mail.gmail.com>
 <CAASLQ4OCLUYXpkk0tTioz8=zc4UEbAm-Whgp_5K7uy-5hZdjKA@mail.gmail.com>
 <5F278CCE.1090006@anastigmatix.net>
 <CAASLQ4M8cN20L7cyVEgks9qdnfsXH4ci7iQbmyi4v_y0W85eLA@mail.gmail.com>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5F2897AF.9040909@anastigmatix.net>
Date: Mon, 3 Aug 2020 19:03:11 -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: 
 <CAASLQ4M8cN20L7cyVEgks9qdnfsXH4ci7iQbmyi4v_y0W85eLA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08/03/20 15:25, Kartik Ohri wrote:
> I have fixed this. I checked maven javadoc code but there was nothing much
> obvious from it. I then compared the project-reports.html file for the
> plugins and found what the issue was. Generating the file using sink was
> actually wrong and wouldn't have worked anyways. (Not in the current setup
> at least.) Further details are in the commit message.

Ah, so that's what the isExternalReport() method is good for!

So this looks good, only now the output name and isExternal bit are
hardcoded in Java. This is probably a good time to look again at
defining a small Java interface similar to MavenReport (at least with
methods resembling canGenerateReport, getCategoryName, getDescription,
getName, getOutputName, and isExternalReport, plus the AbstractMavenReport-
specified executeReport method), of which the script engine can create an
implementation.

I remember you were concerned about the order in which Maven might
call those. I think it might be workable if done like this: the mojo
overrides each of those methods, with a wrapper that first checks whether
the script interface implementation has been created yet; if not, evaluate
the script block and create it. Then call the corresponding method on the
script interface object, and return the result.

The only way I can think of that not working would be if some of those
methods can be called before Maven has interpolated the ${property} values
into the script block. Otherwise, it sounds workable to me.

I think the methods on the script interface may need to be declared with
the 'report' mojo instance as a parameter. That way, the script interface
could have, for example,

default boolean isExternalReport(ReportScriptingMojo report)
{
  return report.isExternalReport(); // ok not quite that simple ;)
}

so the behavior is only changed if the script block defines an overriding
method.

It has to be a little more complicated to avoid infinite recursion,
because report.isExternalReport() would just call the interface method
again. Maybe each of those methods should have a corresponding, e.g.,

boolean isExternalReportDefault() { return super.isExternalReport(); }

which would be what the default method on the script interface calls.

> ----------------
> I have been checking out nar maven plugin's codebase to see how to invoke
> the C compiler and so on. It seems that the plugin delegates at least some
> parts to ant. It creates a custom task and hooks its adapter for different
> os, architecture and linker configurations into it. Do we want to use ant
> for that part or follow a direct approach ?

I lean toward a direct approach; I think part of the appeal of ant may
have been that it was born when Java was very young, and handled things
that were harder to do in straight Java then (before ProcessBuilder,
before java.nio.file, etc.), that now might be simple enough to do directly.

But that wouldn't make it a bad idea to write up a brief page or so of
what particular operations you see it delegating to ant, and which of those
look like they might be nontrivial enough to keep.

----------------
When you have a moment for doing something different, could you review
the methods forWindowsCRuntime and asPgCtlInvocation found here:

https://github.com/jcflack/pljava/blob/2fc8513/pljava-packaging/src/main/java/Node.java#L1646

The first one especially, forWindowsCRuntime, is a part of what I had
originally proposed for your plugin, but I turned out to also need this
simplified subset of it here. So, it should provide some code that you
will be able to reuse in the plugin. (I am not concerned about avoiding
code duplication; the Node.java where I am using it here is intended to
work standalone, just inserted in the installer jar, so it's kind of
special. The plugin can just end up with a similar method included
in PGXSUtils, and perhaps it will need to implement a few more of the
edge cases I was able to reject.)

Getting these transformations just right is kind of critical to avoid
nutty failures dependent on values of passed arguments. I was looking
at that David Deley document on Windows quoting rules, and the Java
ProcessImpl sources, so long I was going crosseyed. Despite writing
detailed comments for each step and referring back to lines in Java's
source, I still ended up fixing some details in debugging (a344af6).

So it would be very helpful if you could look those over in some detail
and let me know of parts that do or don't convince you.

Regards,
-Chap