Re: PL/Java new build plugin

On Tue, Aug 4, 2020 at 4:33 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> 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 did some digging around how maven generates reports. I found that the
maven plugin manager delegates the task to invoke various report plugins to
maven-site-plugin. Looking through its source, I found that it calls the
executeReport method before anything else. The codebase was a bit large so
I may have missed a few subtle details but I tested out my implementation
as explained below and it worked.

> 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 tried to comprehend how this would work but was not able to comprehend
how it would work. I did try to go this route but I felt that it was
getting a bit difficult. So, after some thinking I moved the hard coded
values to the default values of the interface and utilised it in the mojo.
This approach worked well and I have pushed the changes. I have also pushed
a working example to test it. Please review it and let me know if changes
are required.

> ----------------
> > 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.
>

I'll try to figure this out.

> ----------------
> 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.
>

Yeah, sure. I'll take a look at it soon.

Regards,
Kartik