Re: PL/Java new build plugin

Hi! I built a Hello, World! plugin and it seems that the build process will
need to be a two step one. I am not completely sure about this though. I
needed to run mvn clean install the first time I added the plugin module
before I could execute the Mojo. After that, I ran mvn clean and ctried to
execute my Mojo again and it did all the necessary steps itself.

I added some basic functionality to log a simple message in the plugin and
I am unable to execute it until running mvn install on the project. I will
try to modify the properties as you suggested in an earlier email. I am
working on https://github.com/amCap1712/pljava/tree/plugin.
Thanks. Regards.

Hi!

On 06/26/20 16:33, Kartik Ohri wrote:
> Hi! I built a Hello, World! plugin and it seems that the build process will
> need to be a two step one. I am not completely sure about this though. I
> needed to run mvn clean install the first time I added the plugin module
> before I could execute the Mojo.

Oddly, that doesn't seem to be a problem when I try it here. Just for
grins, I added these lines in pljava-so/pom.xml:

<plugin>
<groupId>org.postgresql</groupId>
<artifactId>pljava.pgxs</artifactId>
<version>0.0.1-SNAPSHOT</version>
<executions>
<execution>
<goals>
<goal>count</goal>
</goals>
</execution>
</executions>
</plugin>

and ran an ordinary mvn clean install, and Maven (3.5.2) correctly
figured out it had better build pljava.pgxs before pljava-so, and it
all went smoothly in one run:

[INFO] Reactor Build Order:
[INFO]
[INFO] PostgreSQL PL/Java
[INFO] PL/Java API
[INFO] PL/Java backend Java code
[INFO] pljava.pgxs Maven Plugin
[INFO] PL/Java backend native code
[INFO] PL/Java Ant tasks
[INFO] PL/Java examples
[INFO] PL/Java packaging
...
[INFO] Building pljava.pgxs Maven Plugin 0.0.1-SNAPSHOT
...
[INFO] Building PL/Java backend native code 1.6.0-SNAPSHOT
...
[INFO] --- pljava.pgxs:0.0.1-SNAPSHOT:count (default) @ pljava-so ---
[INFO] Number of modules0
...
[INFO] Reactor Summary:
[INFO]
[INFO] PostgreSQL PL/Java ................................. SUCCESS
[INFO] PL/Java API ........................................ SUCCESS
[INFO] PL/Java backend Java code .......................... SUCCESS
[INFO] pljava.pgxs Maven Plugin ........................... SUCCESS
[INFO] PL/Java backend native code ........................ SUCCESS
[INFO] PL/Java Ant tasks .................................. SUCCESS
[INFO] PL/Java examples ................................... SUCCESS
[INFO] PL/Java packaging .................................. SUCCESS
[INFO]
[INFO] BUILD SUCCESS

I am glad I tried it, because I was feeling pretty sour about the idea
of a two-step build. :)

Were you using an earlier Maven version? I wonder if that might be
part of the story if it wouldn't build for you in one step. (I noticed
a maven.version 3.3.9 in your new pom).

It's ok to assume Maven 3.5.2 as the minimum; that already became
necessary in PR #268 [1].

I notice it gives the count of modules as zero, so I guess something
about the parameter injection didn't go right, but everything else
seemed to.

It's possible it gets *its own MavenProject* injected, which doesn't
have any modules, and it might be interesting to follow the project's
parent link (assuming there is one) and count those modules instead.

A few comments:

- the commit diff makes it look as if the entire top pom.xml has been
deleted and replaced [2].

I suspect that means you edited the file with some tool that both
changed the indenting, and made the lines double-spaced.

A commit like that is hard to review, and it interferes with git
tools for exploring the origins and history of changes. It's important
to check that a git diff is able to show just the specific changes
you intended to make in a given commit.

- PL/Java uses the same source code format conventions as PostgreSQL,
described in [3]. That will call for changing the indenting in the
added pom.xml and HelloWorldMojo.java file, and maybe a few other
details like placement of { and }.

- Java source files all (should) have the project's current comment
header at the top (with your name as contributor, of course).
Some of the older files have an older version that hasn't been
updated yet, but any of the recently touched ones should have the
right one (the one that refers to BSD 3-Clause).

- All the other subproject IDs are pljava-something, not pljava.something.

Congratulations, it seems to be a working plugin!

Regards,
-Chap

[1] https://github.com/tada/pljava/commit/bfbe4c2

[2]
https://github.com/amCap1712/pljava/commit/39596cf#diff-600376dffeb79835ede4a0b285078036

[3] /docs/12/source-format.html

Hi!, I have worked on your comments and fixed the issues. It builds for me
in a single step as well. (It required two only the first time I added the
module.)

I am a bit clueless on how to proceed on working on the plugin. Can you
give me some pointers ? I was thinking of going through nar maven plugin
source code but didn't know where to start since our plugin is targeted at
a specific project and there may be many things we need not to work on.

Hi,

On 06/29/20 16:46, Kartik Ohri wrote:
> Hi!, I have worked on your comments and fixed the issues.

Thanks! Does that exist as a branch on github?

There are some choices how to do that; ideally, one would want to
rebase/squash the fixes into your original commit on branch 'plugin',
so there would only appear one commit that made the intended changes
in, say, pom.xml (as opposed to a commit that completely changes it
and one that changes it back).

Naturally, that changes the commit history, and requires a 'force' push
to publish. It is usually discouraged in a published branch, because
it means any other repositories cloned from it will require a manual step
of also forcing the local tracking branch to match.

So you could go for a very by-the-book approach and just start a branch
with a new name for the new commit, and never change the first one. For
the main repository I follow that policy, but it is probably overkill
here. There might not be anybody tracking your 'plugin' branch but me,
and I won't mind having to resync with it at this early stage. :)

> It builds for me
> in a single step as well. (It required two only the first time I added the
> module.)

I am curious; have you tried it again with org/postgresql/* deleted from
~/.m2/repository ? That should be the same as building it the first time,
and I was able to do that and get a successful one-step build.

My test was with a use of the plugin added to pljava-so; I was not
trying to run the plugin directly from the mvn command line. Maybe that's
the difference.

> give me some pointers ? I was thinking of going through nar maven plugin
> source code but didn't know where to start since our plugin is targeted at
> a specific project and there may be many things we need not to work on.

Exactly ... I have been developing gradually clearer ideas about it
over the last weeks and now I need to write them down. There is a lot
the nar-maven-plugin does that we don't need, and I agree, we will have
a simpler plugin and an easier-to-understand build by not trying to
replicate all that.

Now I just have some writing to do. I'll see how far I get tonight.

Is the plugin able to set some project properties now? That will be
one necessary piece.

Regards,
-Chap

On 06/29/20 18:33, Chapman Flack wrote:

> Exactly ... I have been developing gradually clearer ideas about it
> over the last weeks and now I need to write them down. There is a lot
> the nar-maven-plugin does that we don't need, and I agree, we will have
> a simpler plugin and an easier-to-understand build by not trying to
> replicate all that.
>
> Now I just have some writing to do. I'll see how far I get tonight.

I have added a document for discussion on the PL/Java wiki:

https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin

Much of it repeats what was in the GSoC idea proposal or discussed before,
but it also reflects more thinking I have had time to do since then.

"Document for discussion" would make more sense if you could edit the wiki.
I still need to make that happen. :) Github is weird about wiki permissions.

Regards,
-Chap

>
> I have added a document for discussion on the PL/Java wiki:
>
> https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin
>
> Much of it repeats what was in the GSoC idea proposal or discussed before,
> but it also reflects more thinking I have had time to do since then.
>
I have gone through the document and have a broad overview of the things to
be implemented.

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 builds for me
> > in a single step as well. (It required two only the first time I added
> the
> > module.)
>
> I am curious; have you tried it again with org/postgresql/* deleted from
> ~/.m2/repository ? That should be the same as building it the first time,
> and I was able to do that and get a successful one-step build.
>
> My test was with a use of the plugin added to pljava-so; I was not
> trying to run the plugin directly from the mvn command line. Maybe that's
> the difference.
>
Yes, this is probably the difference. Using the plugin with pljava-so works
well in a single step. Running the goal separately is a two step process.

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

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

I was reading about setting properties in MOJOs and found this detailed
answer <https://stackoverflow.com/a/14727072/5557184>. I think this impacts
how we utilise the plugin. Can you take a look?

On 07/02/20 00:15, Kartik Ohri wrote:
> I was reading about setting properties in MOJOs and found this detailed
> answer <https://stackoverflow.com/a/14727072/5557184>. I think this impacts
> how we utilise the plugin. Can you take a look?

That's a good find, and probably deserves a link on the wiki page, just
because I haven't ever found a better document for just laying out piece
by piece what will work and what won't.

It is clear why we can't use mojo-set properties in the activation of
profiles (profile activation happens too early), and I remember that
disappointed me greatly the first time I tried.

At present, the only ways we are using mojo-set properties are ways
that work, and provided we only change what mojo is setting them
(our own, vs. properties-maven-plugin reading them from a file), that
should not change.

We do, at present, have several Maven profiles in that POM that all
have to be activated different ways (command line, or system properties),
which has always been kind of inconvenient. In essence, we have been
using multiple profiles as a way to get nar-maven-plugin to apply the
right recipes for each platform.

Our own plugin should not need to be tweaked by profiles to do that;
it should natively have the notion of different recipes for different
platforms, and selecting the right one should be part of what it does.
(Of course nar-maven-plugin does have that notion natively also, it's
just that its built-in recipes aren't configurable enough for that to do
what we need.)

That should have a nice effect of shortening the POM too. The way the
Maven profile XML notation requires twenty-four lines to say "if it's
Mac OS, add --bundle_loader postgres" has never been a boon to readability.

That StackOverflow write-up does mention "properties injected into the
MavenSession"—rather than into any particular MavenProject—so that might
be a solid hint where we should set them.

Regards,
-Chap

Hi! I have pushed a commit to branch modifying the mojo to create a
pgxs.properties file. It works fine on Linux except the version
classifier regex. I copied it from the build.xml file. I do not know what
the pattern of a version is so I am unable to debug this. Can you explain
this a bit ?

On 07/03/20 06:20, Kartik Ohri wrote:
> Hi! I have pushed a commit to branch modifying the mojo to create a
> pgxs.properties file. It works fine on Linux except the version
> classifier regex. I copied it from the build.xml file. I do not know what
> the pattern of a version is so I am unable to debug this. Can you explain
> this a bit ?

The stuff in the filterchain/tokenfilter elements, right?

Hmm, well well, it looks like I wrote those, back in 2015, and then
tweaked them slightly in 2016 because some builds of PostgreSQL can have
extra custom information appended to the version. Then tweaked them again
in 2017 for PG 10 where the numbering scheme changed.

The commit where I tweaked for the custom suffixes was bf59577, and
the one for the PG10 numbering change was c434748. The commit comments
and diffs give some insight into what was changed and why. (Have you
played with 'blame' in git before?)

There are two steps there. The first will just replace any suffix
like BETA1 or RC2 with a .99 numeric component, so the next step can
just: skip some possible non-digits at the front, then look for
some groups of digits separated by real dots, then skip anything else
at the end. Replace all that with pg\1.\2 where the references are to
the first two groups of digits.

Now that I think of it, that wasn't quite the right tweak for the PG10
version-scheme change. Prior to 10, a PG version had three numeric
components, and the first two together made a major version (9.4, 9.5,
etc.) and the third component was a minor update.

In 10 and later, there are two numeric components, and just the first one
is the major version, and the second is the minor.

So, if the way the script previously worked was to make, say, a
...-pg9.5-amd64-Linux-gpp.jar (where the classifier mentioned just
the PG major version 9.5, and not the minor version), then in 10 and
later it should just produce ...-pg10-amd64-Linux-gpp.jar. Right now
it produces -pg10.12 (that is, it still includes two components, so
now the jar name reflects the minor version updates, which it previously
did not).

I also see I was careless writing the earlier, replaceregex pattern.
I am prone to forgetting that pretty much everything binds more tightly
than | does, so the way I wrote that pattern, the $ at the end only applies
to the last alternative. It turns out not to make much difference because
every alternative ends with .*, but it could be a subtle trap if there is
ever a reason to tweak that pattern. It would be clearer to enclose all the
alternatives in a non-capturing group and have the $ follow that.

It looks as if you copied the expressions faithfully, except perhaps
for the flags that are set for the first one. That probably doesn't
explain any problem you're having (though it should be fixed anyway),
as that first pattern isn't even in play unless you are building against
a devel, alpha, beta, or rc version of PostgreSQL.

Can you supply an example of what is going wrong?

One last thing, not regex related:

You can see the original build.xml code invokes pg_config multiple
times, reading one configuration value each time, and the same is
done by PGXS in Makefile.global [1].

It does seem more efficient to invoke pg_config only once and parse
its output for the various values, as you are doing here.

What could be the reason existing code avoided doing that?

Regards,
-Chap

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/Makefile.global.in;h=20d7a1d;hb=4fc935a#l119

>
> The commit where I tweaked for the custom suffixes was bf59577, and
> the one for the PG10 numbering change was c434748. The commit comments
> and diffs give some insight into what was changed and why. (Have you
> played with 'blame' in git before?)
>
No, I do not know about git blame but I will take a look and learn about
that.

It looks as if you copied the expressions faithfully, except perhaps
> for the flags that are set for the first one. That probably doesn't
> explain any problem you're having (though it should be fixed anyway),
> as that first pattern isn't even in play unless you are building against
> a devel, alpha, beta, or rc version of PostgreSQL.
>
I had checked about the si flag and think it is probably the same function
as the DOTALL flag I set for that pattern.

Can you supply an example of what is going wrong?
>
After the replacement, the string when written to file displays some empty
boxes instead of the correct version. I'll share an example soon.

One last thing, not regex related:
>
> You can see the original build.xml code invokes pg_config multiple
> times, reading one configuration value each time, and the same is
> done by PGXS in Makefile.global [1].
>
> It does seem more efficient to invoke pg_config only once and parse
> its output for the various values, as you are doing here.
>
> What could be the reason existing code avoided doing that?
>
I saw this as well and though that the plugin in POM executed multiple
times with the specific argument to assign the entire output as the
property. I did not go with that approach because I thought it was simply
not possible for that plugin to read the output and process it to store
different parts in multiple properties. I believe it to be the case with
PGXS as well since both of them are generic and need to cater various use
cases.

On 07/05/20 00:35, Kartik Ohri wrote:
> I had checked about the si flag and think it is probably the same function
> as the DOTALL flag I set for that pattern.

DOTALL is the long name of one of those flags (s); the other one (i) is
CASE_INSENSITIVE. Am I missing where they are being set?

> After the replacement, the string when written to file displays some empty
> boxes instead of the correct version. I'll share an example soon.

Oh, I think I see where that probably is. For some reason, while the
'replacement' string in the underlying java.util.regex methods uses
$1, $2, etc., to refer to captured groups, just to keep things interesting
the 'replace' attribute in an ant containsregex element uses \1, \2
instead. I bet if you ran the output through od or the like, you would see
that the two empty boxes are the byte values 001 and 002, which are what \1
and \2 in a Java string literal represent.

> I saw this as well and though that the plugin in POM executed multiple
> times with the specific argument to assign the entire output as the
> property. I did not go with that approach because I thought it was simply
> not possible for that plugin to read the output and process it to store
> different parts in multiple properties.

Have you written out the assumptions that must be true of pg_config's
output, for parsing it into the separate properties to be reliable?

Regards,
-Chap

>
> DOTALL is the long name of one of those flags (s); the other one (i) is
> CASE_INSENSITIVE. Am I missing where they are being set?

I saw I had not committed the change. I'll fix it in the next commit.

> Oh, I think I see where that probably is. For some reason, while the
> 'replacement' string in the underlying java.util.regex methods uses
> $1, $2, etc., to refer to captured groups, just to keep things interesting
> the 'replace' attribute in an ant containsregex element uses \1, \2
> instead. I bet if you ran the output through od or the like, you would see
> that the two empty boxes are the byte values 001 and 002, which are what \1
> and \2 in a Java string literal represent.
>
Thanks. This fixed the issue.

> Have you written out the assumptions that must be true of pg_config's
> output, for parsing it into the separate properties to be reliable?
>
The only assumption I make is that pg_config prints each variable
separated by the system's file separator.

I have deleted the ant-run code generating the pgsql.properties file and
read the pgxs.properties file generated by PL/Java PGXS plugin. I have set
up the branch with CI and have observed that this is working for Linux and
macOS but failing for Windows. I will try to debug why this fails for
Windows.

On 07/05/20 06:44, Kartik Ohri wrote:

>> Have you written out the assumptions that must be true of pg_config's
>> output, for parsing it into the separate properties to be reliable?
>>
> The only assumption I make is that pg_config prints each variable
> separated by the system's file separator.

Anything about the system's line separator?

Anything about what characters are or are not allowed to be present
inside any of the variables being printed?

I am emphasizing this now because it involves the same kind of thinking
that the Windows ProcessBuilder workaround will require a lot of.

Regards,
-Chap

>
> Anything about the system's line separator?
>
If I am right, readLine in Java should take of system's line separator.

Anything about what characters are or are not allowed to be present
> inside any of the variables being printed?
>
None, as of now but I am still exploring it.

I am emphasizing this now because it involves the same kind of thinking
> that the Windows ProcessBuilder workaround will require a lot of.
>
Yes, I understand. I'll try to debug and get this working on Windows and
that should clarify things.

>

On 07/05/20 14:05, Kartik Ohri wrote:
>> Anything about the system's line separator?
>>
> If I am right, readLine in Java should take of system's line separator.
>
> Anything about what characters are or are not allowed to be present
>> inside any of the variables being printed?
>>
> None, as of now but I am still exploring it.

Assumptions are what you have /before/ exploring to find out if they are
true. So I think that means you do have some assumptions. :)

On any OS that we build for, are line separator characters forbidden
in path names?

Regards,
-Chap

On Sun, Jul 5, 2020, 11:40 PM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/05/20 14:05, Kartik Ohri wrote:
> >> Anything about the system's line separator?
> >>
> > If I am right, readLine in Java should take of system's line separator.
> >
> > Anything about what characters are or are not allowed to be present
> >> inside any of the variables being printed?
> >>
> > None, as of now but I am still exploring it.
>
> Assumptions are what you have /before/ exploring to find out if they are
> true. So I think that means you do have some assumptions. :)
>
> On any OS that we build for, are line separator characters forbidden
> in path names?
>
Yes, that is the case.

>

On 07/05/20 14:17, Kartik Ohri wrote:

>> On any OS that we build for, are line separator characters forbidden
>> in path names?
>>
> Yes, that is the case.

For which ones?

$ cd 'Hi!
I am a directory.
You can ls me.'

$ pwd
/var/tmp/Hi!
I am a directory.
You can ls me.

$ ls
foo

Regards,
-Chap

>
> For which ones?
>
> $ cd 'Hi!
> I am a directory.
> You can ls me.'
>
> $ pwd
> /var/tmp/Hi!
> I am a directory.
> You can ls me.
>
> $ ls
> foo
>
Only '\r' and '\n' are not allowed in the path. Everything else should
work. Also, this behaviour can be modified as well by Scanner instead of
BufferedReader.

On 07/05/20 14:35, Kartik Ohri wrote:
> Only '\r' and '\n' are not allowed in the path. Everything else should
> work.

Um, "not allowed" by what? Seems more like you are describing a case that
is possible, but not handled by your code.

As the example showed, real paths on the system are allowed to contain
those characters.

The limitation here is that the pg_config output will be incorrectly parsed
if they do, leading probably to a mysterious PL/Java build failure, quite
similar our existing issue #261, which was caused by other code assuming
a directory name would never contain whitespace.

Solving issue #261 will be one of the benefits of your work this summer,
by ending our reliance on that other faulty code.

... which is a good reason to think hard about whether the new code should
be bringing in an analogous new limitation.

/In cases where there is no simple alternative/, it's acceptable to address
such limits in documentation. The release notes can say something like:

We fixed a problem where PL/Java wouldn't build in a directory whose
name contains a space, because that turned out to be not so unusual
really. At the same time, we have made things break if PostgreSQL
is in a directory with newlines, because we think that's /really/
unusual and you deserve to have our build break if you do it.

You especially deserve whatever happens if you name a directory
something like:

Hi!
BINDIR=I am a sneaky
directory.

because we are sure that would never happen except in adversarial
usage, and we don't regard building PL/Java as any kind of
privileged operation that would be of interest to an adversary,
so we are not using a threat model where that would matter.

That can be a reasonable way of explaining implementation decisions that
get made when there is no alternative that avoids the limitation, or
when the only alternatives would be too much work. You can definitely
browse through PL/Java's git history and find comments very much like that,
explaining decisions that got made when there wasn't an easy better
alternative available.

But it is a lot of explaining to have to do in a situation where
there is an alternative that doesn't have the problem, and not only
is it easy, it is the way the code did work before you changed it.

Does that make sense?

---

I hope this doesn't sound unkind; it seems like you had a belief
"the OS doesn't allow line separators in pathnames" that turned out
not to be correct. Happens all the time. When something like that happens
to me, I go through about five stages of processing it:

1. Hmm, I believed X was true and it isn't. Bummer.

2. Can I remember roughly how long I have believed X was true?

3. Have I written anything else in that time where I might have used
the belief that X was true? Any of it worth going back to review
or add test cases?

4. Can I remember from what source I first got the idea X was true?

5. Can I remember any other things Y, Z, ... that I learned from
the same source? Would this be a good time to check Y, Z, ... also?

---

The case of line separators in pathnames makes a prime example of something
that is no problem as far as the core OS is concerned, but is very widely
cautioned against /because/ it may often trigger failures in, ahem, userland
code that makes too many assumptions.

And that is a very good reason for people who are using a system, and
choosing how to name things, to be cautious in that way.

At the same time, it is not a good reason for people who write code
to write the kind of code that makes such caution necessary.

This reduces to Jon Postel's robustness principle from RFC 760, "be
conservative in what you do, be liberal in what you accept from others."

Regards,
-Chap

>
> But it is a lot of explaining to have to do in a situation where
> there is an alternative that doesn't have the problem, and not only
> is it easy, it is the way the code did work before you changed it.
>
> Does that make sense?
>
Yes, it does. I'll fix this immediately.

> ---
>
> I hope this doesn't sound unkind; it seems like you had a belief
> "the OS doesn't allow line separators in pathnames" that turned out
> not to be correct.

Certainly, I did not know that line separators were allowed in path names
and hence made this error. I will keep this in mind from now on.

> Happens all the time. When something like that happens
> to me, I go through about five stages of processing it:

>
1. Hmm, I believed X was true and it isn't. Bummer.
>
> 2. Can I remember roughly how long I have believed X was true?
>
> 3. Have I written anything else in that time where I might have used
> the belief that X was true? Any of it worth going back to review
> or add test cases?
>
> 4. Can I remember from what source I first got the idea X was true?
>
> 5. Can I remember any other things Y, Z, ... that I learned from
> the same source? Would this be a good time to check Y, Z, ... also?
>
> ---
>
> The case of line separators in pathnames makes a prime example of something
> that is no problem as far as the core OS is concerned, but is very widely
> cautioned against /because/ it may often trigger failures in, ahem,
> userland
> code that makes too many assumptions.
>
> And that is a very good reason for people who are using a system, and
> choosing how to name things, to be cautious in that way.
>
> At the same time, it is not a good reason for people who write code
> to write the kind of code that makes such caution necessary.
>
> This reduces to Jon Postel's robustness principle from RFC 760, "be
> conservative in what you do, be liberal in what you accept from others."
>
Thanks for the detailed advice. It will help me to write better code.

Hi!
I have changed the implementation to execute one command at a time. Here is
the new implementation,
https://github.com/amCap1712/pljava/commit/f3dd925049cce9bfdff2ae55a41b2005272ed162.
It
is still not working on Windows but I have diagnosed the issue.
pgsql.properties has paths of the type C\:/PROGRA~1/POSTGR~1/12/bin while
the our plugin has paths of type C:/PROGRA~1/POSTGR~1/12/bin. I can
probably fix this by detecting the OS and doing a string substitution but I
wanted to ask if this is the right way to proceed.

Regards,
Kartik.

On 07/07/20 09:58, Kartik Ohri wrote:

> pgsql.properties has paths of the type C\:/PROGRA~1/POSTGR~1/12/bin while
> the our plugin has paths of type C:/PROGRA~1/POSTGR~1/12/bin. I can
> probably fix this by detecting the OS and doing a string substitution

I'm not sure that's really a Windows-specific behavior, just an
overcautious \ added before the : when the property is written to the file.

If you were using java.util.Properties and the store() method to write
the file, you also would have the \ there:

jshell> var p = new Properties()
jshell> p.put("mypath", "C:/PROGRA~1/POSTGR~1/12/bin")
jshell> p.store(System.out, "I am a comment")
#I am a comment
#Tue Jul 07 18:32:12 EDT 2020
mypath=C\:/PROGRA~1/POSTGR~1/12/bin

The backslash isn't actually necessary before a colon as part of the value.
(If a colon were part of the /key/, the backslash would be needed.)

A Properties object can happily read in the same value with or without the
backslash there:

jshell> p.clear()
jshell> p.load(new StringReader("""
...> #Tue Jul 07 18:32:12 EDT 2020
...> mypath=C:/PROGRA~1/POSTGR~1/12/bin
...> """))
jshell> p.get("mypath")
$12 ==> "C:/PROGRA~1/POSTGR~1/12/bin"

So that backslash isn't really essential there, it is just something
Properties.store() happens to add when writing a file.

So I have to assume that whatever isn't working on Windows must have
some other cause really.

It might not be worth spending much time on backslash or no backslash
in the file, as I assume eventually you will write no file at all, and
just set the properties in Maven directly.

I added some additional code comments in github:
https://github.com/amCap1712/pljava/commit/f3dd925

Regards,
-Chap

I tried to debug the Windows crash yesterday but without any success. Here
is the log from the old build process, oldlog.txt
<https://usercontent.irccloud-cdn.com/file/A3ZmSPHz/oldlog.txt> and here is
the link to the new plugin build log.txt
<https://usercontent.irccloud-cdn.com/file/FnKQLvrI/log.txt>. Can you
take a look at it ?

On 07/09/20 06:48, Kartik Ohri wrote:
> <https://usercontent.irccloud-cdn.com/file/A3ZmSPHz/oldlog.txt> and here is
> the link to the new plugin build log.txt
> <https://usercontent.irccloud-cdn.com/file/FnKQLvrI/log.txt>. Can you

Odd ... were these two builds started with different command lines?

The first one looks like it builds all subprojects:

[INFO] Reactor Build Order:
[INFO]
[INFO] PostgreSQL PL/Java
[INFO] PL/Java API
[INFO] PL/Java backend Java code
[INFO] PL/Java backend native code
[INFO] PL/Java Ant tasks
[INFO] PL/Java examples
[INFO] PL/Java packaging

The second one seems to skip right to pljava-so:

[INFO] Reactor Build Order:
[INFO]
[INFO] PL/Java backend native code
[INFO] PL/Java Ant tasks
[INFO] PL/Java examples
[INFO] PL/Java packaging

In nar-maven-plugin 3.2.3, the line with the null pointer exception
seems to be this one:

https://github.com/maven-nar/nar-maven-plugin/blob/nar-maven-plugin-3.2.3/src/main/java/com/github/maven_nar/Compiler.java#L499

which makes me think that some entry of 'defines' is a null.

If you're running the build with Java 14 or later, you may be able
to squeeze a little more information out of a NullPointerException
by putting the right -XX:... option in the MAVEN_OPTS environment
variable (see [1]). In this case it might only say something like
"cannot call method split() because receiver is null" (I don't see
a lot of other possibilities on that line) but in general it might
be helpful.

Regards,
-Chap

[1] /message-id/5ED2B113.1000606%40anastigmatix.net

>
> Odd ... were these two builds started with different command lines?
>
> The first one looks like it builds all subprojects:
>
> [INFO] Reactor Build Order:
> [INFO]
> [INFO] PostgreSQL PL/Java
> [INFO] PL/Java API
> [INFO] PL/Java backend Java code
> [INFO] PL/Java backend native code
> [INFO] PL/Java Ant tasks
> [INFO] PL/Java examples
> [INFO] PL/Java packaging
>
> The second one seems to skip right to pljava-so:
>
> [INFO] Reactor Build Order:
> [INFO]
> [INFO] PL/Java backend native code
> [INFO] PL/Java Ant tasks
> [INFO] PL/Java examples
> [INFO] PL/Java packaging
>

Yes, when the second build failed. I ran mvn install -rf :pljava-so -X -e.

In nar-maven-plugin 3.2.3, the line with the null pointer exception
> seems to be this one:
>
>
> https://github.com/maven-nar/nar-maven-plugin/blob/nar-maven-plugin-3.2.3/src/main/java/com/github/maven_nar/Compiler.java#L499
>
> which makes me think that some entry of 'defines' is a null.
>

Any suggestions what could be the problem? As far as I see in the
build.xml, I can only think of MSVC.RINT but it should not be probably the
cause.

> If you're running the build with Java 14 or later, you may be able
> to squeeze a little more information out of a NullPointerException
> by putting the right -XX:... option in the MAVEN_OPTS environment
> variable (see [1]). In this case it might only say something like
> "cannot call method split() because receiver is null" (I don't see
> a lot of other possibilities on that line) but in general it might
> be helpful.
>

Yes, I understand that but as you say it isn't beneficial in this case.

On 07/09/20 09:51, Kartik Ohri wrote:

> Any suggestions what could be the problem? As far as I see in the
> build.xml, I can only think of MSVC.RINT but it should not be probably the
> cause.

You could be on to something. I remember thinking that MSVC_RINT is
probably no longer essential to the build, at least for MSVC > 2012
or PG > 9.3. But still, if there's an MSVC-only define that uses it:

https://github.com/amCap1712/pljava/blob/d6608ff/pljava-so/pom.xml#L120

and it isn't being set to something, that might produce a null in the
defines list.

>> to squeeze a little more information out of a NullPointerException
>> by putting the right -XX:... option in the MAVEN_OPTS environment
>
> Yes, I understand that but as you say it isn't beneficial in this case.

I wonder if it wouldn't be worthwhile to have that in the MAVEN_OPTS
for all the automated builds (with Java >= 14), just to save time if
something else happens to blow up some day,

Regards,
-Chap

On Thu, Jul 9, 2020, 7:44 PM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/09/20 09:51, Kartik Ohri wrote:
>
> > Any suggestions what could be the problem? As far as I see in the
> > build.xml, I can only think of MSVC.RINT but it should not be probably
> the
> > cause.
>
> You could be on to something. I remember thinking that MSVC_RINT is
> probably no longer essential to the build, at least for MSVC > 2012
> or PG > 9.3. But still, if there's an MSVC-only define that uses it:
>
> https://github.com/amCap1712/pljava/blob/d6608ff/pljava-so/pom.xml#L120

I'll look further into this.

and it isn't being set to something, that might produce a null in the
> defines list.
>
> >> to squeeze a little more information out of a NullPointerException
> >> by putting the right -XX:... option in the MAVEN_OPTS environment
> >
> > Yes, I understand that but as you say it isn't beneficial in this case.
>
> I wonder if it wouldn't be worthwhile to have that in the MAVEN_OPTS
> for all the automated builds (with Java >= 14), just to save time if
> something else happens to blow up some day,
>
The latest preview builds of Java 15 come with this feature automatically
enabled. Probably, a consensus will be reached on the default state if this
feature before the General Availability later in the year. I don't know
what MAVEN_OPTS is but I'll learn about it and enable it in Java 14.

> Regards,
> -Chap
>

>
> > Any suggestions what could be the problem? As far as I see in the
>> > build.xml, I can only think of MSVC.RINT but it should not be probably
>> the
>> > cause.
>>
>> You could be on to something. I remember thinking that MSVC_RINT is
>> probably no longer essential to the build, at least for MSVC > 2012
>> or PG > 9.3. But still, if there's an MSVC-only define that uses it:
>>
>> https://github.com/amCap1712/pljava/blob/d6608ff/pljava-so/pom.xml#L120
>
> I'll look further into this.
>

IIRC, PL/Java won't support PG 9.3 in any case so should I do away with the
define completely ?

On 07/09/20 11:11, Kartik Ohri wrote:

> IIRC, PL/Java won't support PG 9.3 in any case so should I do away with the
> define completely ?

I had not made a final decision about how many versions back 1.6.0 will
support, but 9.3 - 9.4 are certainly in the neighborhood where I was
thinking of drawing the line, so let's say yes.

Regards,
-Chap

Hi!
I am having some temporary internet connectivity issues due to which I am
unable to RDP into a Windows VM and debug the issue.

Meanwhile, I tried to eliminate the properties file and encountered the
include path not found issue in nar-maven plugin. It took some time to find
the issue but I was able to find the issue. It was that the pg_config
appended a newline character to its output which was being added to the
property value causing a path error. I have fixed it now and eliminated the
need of the pgxs.properties file.

Moving on, I tried to eliminate the CDATA regex in the pom.xml. I have come
up with a Java version. There are a few caveats like hex escaping is not
supported in Java so these need to be converted to Unicode escaping and so
on. But I do not know how to test the regex and what the purpose of
libjvmdefault is here. Can you explain it and tell how to test this regex
escaping code ?

Regards,
Kartik.

>
> Meanwhile, I tried to eliminate the properties file and encountered the
> include path not found issue in nar-maven plugin. It took some time to find
> the issue but I was able to find the issue. It was that the pg_config
> appended a newline character to its output which was being added to the
> property value causing a path error. I have fixed it now and eliminated the
> need of the pgxs.properties file.
>

While debugging this issue, I took a look at properties-maven-plugin and
found that it writes the properties to the project instead of session. So,
I am writing the properties to project as well instead of the session.
Also, I read about the difference between getUserProperties provided by the
session and the getProperties provided by the session. I grasped that
getUserProperties is used for the properties the user passes to the maven
command using -D. It is possible to obtain project properties using a
session but directly using a project is more straightforward. The current
implementation using project properties compiles and works for me.

On 07/09/20 14:45, Kartik Ohri wrote:
> Meanwhile, I tried to eliminate the properties file and encountered the
> include path not found issue in nar-maven plugin. It took some time to find
> the issue but I was able to find the issue. It was that the pg_config
> appended a newline character to its output which was being added to the

Ah! Good catch. Yes, in your earlier approach with readLine(), that
terminator would have been eaten already.

It turns out [1] pg_config always ends the line with a single \n (no
matter the platform) so that's always what to remove. (Java's readLine
could have caused a related sort of problem too; it would be unlikely
for the values we care about in this case to end with \r, but if one
did, readLine would see the \r followed by the line-ending \n and eat both.)

> Moving on, I tried to eliminate the CDATA regex in the pom.xml. I have come
> up with a Java version. There are a few caveats like hex escaping is not
> supported in Java so these need to be converted to Unicode escaping and so
> on. But I do not know how to test the regex and what the purpose of
> libjvmdefault is here. Can you explain it and tell how to test this regex
> escaping code ?

I assume what you mean by a Java version is a Java version of the JavaScript
quoteStringForC() function, as the regex it's using is already built on
java.util.regex, so should not need much/any revision. (Strictly, the string
literals being passed to compile() might have to be checked for any
differences between JS and Java string-literal syntax, but nothing is
jumping out at me.)

There are some regrettable subtleties in the pattern because of differences
between what escapes a string-literal understands and what escapes a
java,util.regex.Pattern understands: you can see in capture group 2 that
\a\f\n\r\t\x0B are all spelled with double-backslashes, which means they
survive the string literal (the double backslashes becoming single ones)
and the regex compiler literally sees \a\f\n\r\t\x0B and it understands all
those forms so that's fine, but the \b does not have its backslash doubled.
So the string literal turns it immediately into a real backspace character,
which is what the regex compiler sees. \b in the regex language does have
a meaning but it's different: it means a word boundary, not a backspace.

Of course the purpose of quoteStringForC is "you give me an arbitrary
string, I'll give you the exact C language string literal that you need
for building that string into a C program." So the output it produces
has to be correct according to the rules of C.

I wanted to make it as clear as I could by breaking the regex into three
lines (capture group 1, group 2, and groups 3,4 which go together) and
organizing the function logic the same way.

Group 1 takes care of the things that only need a \ in front to be escaped
for C: \ itself, ", and a ? if it could look like part of a C trigraph [2].
Really, C will turn \? to ? anywhere, but to avoid cluttering the string
with backslashes, it is only necessary to find sequences like ??[=(/)'<!>-]
and \ just one of the ? characters, to prevent the sequence being seen as
a trigraph.

Group 2 is for characters known by special escape sequences; most of those
are the same in C and Java, but C has \a and \v and Java doesn't (though
java.util.regex does know \a), so those have to be spelled differently
for Java (Unicode or octal would work).

Group 3 just catches any other control character and turns it into a C
\xnn escape. Group 4 is only to detect if the control character is
followed by a hexdigit. That's necessary because C's \x escape will keep
eating as many hexdigits as it sees, so you can't just turn a character
into a \xnn if what follows it is also a hexdigit. You can turn it into
\xnn"" which essentially ends one string literal and starts another one.
C always joins together adjacent string literals, so that solves the
problem.

While general-purpose, the first reason to have a quoteStringForC function
was so a person building PL/Java could add -Dpljava.libjvmdefault=some_path
on the Maven command line, and have "some_path" properly escaped be built
into the C code so it becomes the default for pljava.libjvm_location and
there's no need to fuss with that GUC in PostgreSQL to make PL/Java work,

... which ends up, on Windows, being a challenging test of how reliably
Java passes complicated argument values to the C compiler process ... a
test the existing ProcessBuilder implementation does not pass.

Regards,
-Chap

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/bin/pg_config/pg_config.c;h=5279e7f#l124

[2] https://en.wikibooks.org/wiki/C_Programming/C_trigraph

Thanks for the detailed explanation. Yes, indeed. I had to make minimal
changes in order to get the regex to compile in Java. I'll try to test it.

You review the commit without the regex
https://github.com/amCap1712/pljava/commit/e02a71a0184b70b03b14e54ff454f97dc9946759
here.
The pgxs properties file is still created because it is needed by build.xml
in pljava-packaging. I have not gone into depth of it but I'll take a look
at it soon and try to remove it from there as well.

Regards,
Kartik.

On 07/09/20 16:46, Kartik Ohri wrote:
> https://github.com/amCap1712/pljava/commit/e02a71a

I've made some comments in github at the link above.

Regards,
-Chap

Hi!
I have addressed your comments and pushed a commit fixing those and some
other changes. I have opened a PR at
https://github.com/amCap1712/pljava/pull/3 so that our discussion is
preserved even if I force push changes.

Can you test the libjvm regex to see if it works ? Also, if there are no
other issues to resolve in the current functionality of the plugin, how
should we proceed? Meanwhile, I'll try to debug the Windows issue again.

Regards,
Kartik

Hi!
I have incorporated your comments in the code. I read about the scripting
engine but the details were scarce. Do you have any resources to learn
about it ?
How should I proceed with my work ? As of now, I am able to set the
properties. The next step seems to be to begin replacing nar
plugin's functionality. Can you point me towards a starting point ?

On 07/13/20 12:05, Kartik Ohri wrote:
> I have incorporated your comments in the code. I read about the scripting
> engine but the details were scarce. Do you have any resources to learn
> about it ?

Hi,

The basics of getting a script engine to work are pretty straightforward;
you can look here [1] in the class that goes in the self-installer jar
and runs a script to resolve installation path names.

You can have a MIME type like application/javascript and ask the
ScriptEngineManager to get an engine that handles that MIME type,
and if there's one around, the manager gives you it.

You can put things into its symbol table by names so scripts can refer
to them, and you can have it eval a script, and you can get values back
from its symbol table that the script may have set.

There are additional possibilities I didn't use there. If the script
engine that the manager gives you is instanceof Invocable, then you don't
have to just feed it scripts to eval: you can feed it a script that
defines various functions, and after that you can use it to just invoke
the functions whenever needed. If you have some Java interface in mind,
and the script defined the right set of functions, you can ask the
engine to generate a Java class that implements the interface, which
you can use in Java any way you want. Calls to the interface methods
go to the functions in the script.

We might very well want to use that.

> How should I proceed with my work ? As of now, I am able to set the
> properties. The next step seems to be to begin replacing nar
> plugin's functionality.

I think it would probably be helpful to work on script engine integration
next. I wouldn't really want to tackle replacing nar functionality without
it.

There is also another way script engine functionality would be helpful:
I would like this plugin to also include a mojo that implements
MavenReport [2], maybe by extending AbstractMavenReport, and would
essentially have one configuration option, a script.

I would like that because I am really very tired of maven-javadoc-plugin.

This is timely because I have just made a PR that adds a bunch of new
code in pljava-packaging, enough that I would now like javadoc for that,
and I am fighting with the maven-javadoc-plugin again.

5ee9b6b was already at least one full day of working around bugs in
that plugin, and 395ac0d was another full day of the same, and I am not
interested in spending another. The thing is, running the javadoc tool
really isn't that hard! Running it and passing it the right arguments
ought to be about four lines of script, and they would be right there
in the POM for anybody to look at who wondered why it did or did not
do something.

...

By the way, if you have a moment for a break in between other things,
I would be happy if you could look at the pull request I just put in [3].

I don't have an easy way to test it on Windows, and I wouldn't be
surprised if some details in init() or start(), for example, might
need adjustment there.

Regards,
-Chap

[1] https://github.com/tada/pljava/blob/b0330f7/org/gjt/cuspy/JarX.java#L544
[2]
https://maven.apache.org/guides/plugin/guide-java-report-plugin-development.html
[3] https://github.com/tada/pljava/pull/287

>
> By the way, if you have a moment for a break in between other things,
> I would be happy if you could look at the pull request I just put in [3].
>
> I don't have an easy way to test it on Windows, and I wouldn't be
> surprised if some details in init() or start(), for example, might
> need adjustment there.
>

Sure. Running a CI check is as easy as opening a PR against the master
branch of my PL/Java fork. For this case,
https://github.com/amCap1712/pljava/pull/4 . At the bottom, you can click
details next to the CI provider to view detailed logs of each build. As of
now, the Windows build will fail for Java 9, 10 (probably PG 9 as well) in
any case so that can be ignored. I'll notify you once the build is done.

--------
I have begun implementing a basic script integration mojo but while trying
to test it, I get a NullPointerException because the ScriptManager is
unable to find any suitable engine. How can I fix this ?

ScriptEngineManager manager = new ScriptEngineManager();
engine = manager.getEngineByMimeType("application/javascript");

Regards,
Kartik.

>
> Sure. Running a CI check is as easy as opening a PR against the master
> branch of my PL/Java fork. For this case,
> https://github.com/amCap1712/pljava/pull/4 . At the bottom, you can click
> details next to the CI provider to view detailed logs of each build. As of
> now, the Windows build will fail for Java 9, 10 (probably PG 9 as well) in
> any case so that can be ignored. I'll notify you once the build is done.
>

I just checked Appveyor CI builds for Windows (even those which work
properly) are failing. You can view the logs at
https://ci.appveyor.com/project/amCap1712/pljava/builds/34092481 . Also,
you can push new commits to your branch and the CI will automatically start
a new against them. The link to the new build will be available on the PR
page.

On 07/14/20 13:36, Kartik Ohri wrote:
> I have begun implementing a basic script integration mojo but while trying
> to test it, I get a NullPointerException because the ScriptManager is
> unable to find any suitable engine. How can I fix this ?

Probably the same way as in 6cd539b. Now, that was just passing a
classpathref= to an existing plugin. Writing a new plugin, you may
have to figure out what the plugin needs to /do/ with the classpathref,
but the maven-antrun-plugin source must contain the answer somewhere.

Regards,
-Chap

>
> Probably the same way as in 6cd539b. Now, that was just passing a
> classpathref= to an existing plugin. Writing a new plugin, you may
> have to figure out what the plugin needs to /do/ with the classpathref,
> but the maven-antrun-plugin source must contain the answer somewhere.
>
I looked into the maven-antrun-plugin. The relevant part is here
<https://github.com/apache/maven-antrun-plugin/blob/8e5b3c613e96c20881347ec16813864a2d8ef0bb/src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java#L364>.
It adds the classpaths as a reference to an instance of an Ant Project. The
documentation
<https://ant.apache.org/manual/api/org/apache/tools/ant/Project.html#addReference(java.lang.String,java.lang.Object)>
for which is not very illuminating. After that, it just executes the tasks
on the Ant Project. I'll keep digging further to find something useful. I
also found https://issues.apache.org/jira/browse/MANTRUN-200 and
https://issues.apache.org/jira/browse/MNG-6275. I think these might be
relevant to the problem as well. I have also pushed the changes. I suspect
I might be doing something wrong in the integration itself which might be a
cause of this.
Regards,
Kartik.

On 07/14/20 16:40, Kartik Ohri wrote:
> I looked into the maven-antrun-plugin. The relevant part is here
> <https://github.com/apache/maven-antrun-plugin/blob/8e5b3c613e96c20881347ec16813864a2d8ef0bb/src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java#L364>.
> It adds the classpaths as a reference to an instance of an Ant Project.

Right, because we have the extra indirection of Maven plugin, running Ant
task, running script. You'll be able to shorten that to just Maven plugin
running script, so the key might be looking in the Ant source to see what
the <script> task does with a classpath ref.

> I might be doing something wrong in the integration itself which might be a
> cause of this.

It's worth mentioning that the difficulty only results from the weird stuff
Maven does with classpaths. If you just write a standalone hello-world-style
Java program that uses the ScriptEngineManager, you should find that it
Just Works. (As long as you're in Java 14 or earlier; for 15 of course you'd
need the two Graal jars on the classpath.)

So that might be an easy way to reassure yourself that you haven't missed
something in how you're calling it.

Regards,
-Chap

On 07/14/20 13:40, Kartik Ohri wrote:
> I just checked Appveyor CI builds for Windows (even those which work
> properly) are failing. You can view the logs at
> https://ci.appveyor.com/project/amCap1712/pljava/builds/34092481 . Also,
> you can push new commits to your branch and the CI will automatically start
> a new against them. The link to the new build will be available on the PR
> page.

Thanks! Looks like I have fixed that with a42c416. Turned out to be
Windows line endings, and it means I gave you bad advice with regard to
getPgConfigProperty [1] after all. Even though pg_config explicitly writes
a single \n character on every platform, Windows (probably the C library
really) still changes it to \r\n before it arrives on the pipe. So just
removing one character won't always work. It has to be line.separator
(that, or an explicit test for Windows). Sorry for leading you astray.

Now the only appveyor "errors" I'm seeing seem to be a result of appveyor
thinking anything written to stderr is a problem. So running the installer
jar to install the files produces a wall of red every time, because it
prints the installed file names to stderr. And then on Java 9 there is
still one JNI reference warning, but that's being treated as a failure.

The first of those problems can surely be fixed with a 2>&1 on the
command running the installer jar (or whatever the equivalent Windows
syntax is for "merge your stderr into stdout").

The second looks a little trickier, as you're probably checking for
warning output from install_jar(...examples...), which really does
indicate trouble, but the WARNING: JNI local refs: does not.

I don't understand what the PG=9 configuration even is. The PG version
numbering change happened in 10, so "9" could mean any of seven different
major versions, and I can't even see from the log which of those the
run is trying to use. I think that could be the reason it's failing too,
because it's trying to run "\Program Files\PostgreSQL\9\bin\pg_ctl" and
there is no such thing with just "9" as the version.

Regards,
-Chap

[1]
https://github.com/amCap1712/pljava/blob/fc2bd50/pljava-pgxs/src/main/java/org/postgresql/pljava/pgxs/PGXSUtils.java#L110

but for a very strange reason:

/Users/travis/build/amCap1712/pljava/pljava-so/build.xml:72:
/Users/travis/build/amCap1712/pljava/pljava-so/pg_config.h doesn't exist

The line in build.xml says this:

<filelist files="pg_config.h" dir="${PGSQL_INCLUDEDIR}"/>

so it is very strange for Ant to complain about pg_config.h not being
in its current directory. Of course it isn't, it's in $PGSQL_INCLUDEDIR!

It is as if the dir= is being ignored, or $PGSQL_INCLUDEDIR is unset,
or something. Just for this one build? Odd.

Regards,
-Chap

>
> Thanks! Looks like I have fixed that with a42c416. Turned out to be
> Windows line endings, and it means I gave you bad advice with regard to
> getPgConfigProperty [1] after all. Even though pg_config explicitly writes
> a single \n character on every platform, Windows (probably the C library
> really) still changes it to \r\n before it arrives on the pipe. So just
> removing one character won't always work. It has to be line.separator
> (that, or an explicit test for Windows). Sorry for leading you astray.
>
Thanks for informing this. I'll check the plugin with this fix to see if
this now works on Windows.

Now the only appveyor "errors" I'm seeing seem to be a result of appveyor
> thinking anything written to stderr is a problem. So running the installer
> jar to install the files produces a wall of red every time, because it
> prints the installed file names to stderr. And then on Java 9 there is
> still one JNI reference warning, but that's being treated as a failure.
>
> The first of those problems can surely be fixed with a 2>&1 on the
> command running the installer jar (or whatever the equivalent Windows
> syntax is for "merge your stderr into stdout").
>
> The second looks a little trickier, as you're probably checking for
> warning output from install_jar(...examples...), which really does
> indicate trouble, but the WARNING: JNI local refs: does not.

There is another error as well on PG 10
https://ci.appveyor.com/project/amCap1712/pljava/builds/34099990/job/bgpqdy5qjkik2ya9,
one of the tests fails. This has been the case since I first added this
configuration.

I don't understand what the PG=9 configuration even is. The PG version
> numbering change happened in 10, so "9" could mean any of seven different
> major versions, and I can't even see from the log which of those the
> run is trying to use. I think that could be the reason it's failing too,
> because it's trying to run "\Program Files\PostgreSQL\9\bin\pg_ctl" and
> there is no such thing with just "9" as the version.
>
I had suspected the same but didn't test it due to some reason I seem to
have forgotten but I will do that now.

On Wed, Jul 15, 2020, 4:41 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> but for a very strange reason:
>
> /Users/travis/build/amCap1712/pljava/pljava-so/build.xml:72:
> /Users/travis/build/amCap1712/pljava/pljava-so/pg_config.h doesn't exist
>
> The line in build.xml says this:
>
> <filelist files="pg_config.h" dir="${PGSQL_INCLUDEDIR}"/>
>
> so it is very strange for Ant to complain about pg_config.h not being
> in its current directory. Of course it isn't, it's in $PGSQL_INCLUDEDIR!
>
> It is as if the dir= is being ignored, or $PGSQL_INCLUDEDIR is unset,
> or something. Just for this one build? Odd.
>
This is a known issue. Sometimes, one or two macOS builds fail. The
underlying cause is that macports is unable to connect using rsync protocol
to the outside world. This happens once in a while and since we have 10
macOS VMs in a build, it happens in every or every second build.

I had opened a ticket with the Travis support regarding this but they
rubbished it as an issue with Github availability. I'll rerun the build and
it passed https://travis-ci.com/github/amCap1712/pljava/builds/175645465.
This is a bit annoying. I do not know how to solve this yet. Last time I
checked, the macports devs themselves face this issue. I'll recheck with
them to know if a solution has been found. For the time being, the solution
is to manually rerun the failing job from Travis dashboard.

>

>
> Even though pg_config explicitly writes
>> a single \n character on every platform, Windows (probably the C library
>> really) still changes it to \r\n before it arrives on the pipe.
>
> This was indeed the case. I also confirmed with a C dev in ##c and they
said that C changes \n to \r\n on Windows. I have fixed this issue. There
is no longer any include path error. There was another error related nar
maven which I was able to fix by setting PGSQL_PKGLIBDIR in addition to
other variables. The plugin now works on windows as well.

I had suspected the same but didn't test it due to some reason I seem to
> have forgotten but I will do that now.
>
This has been also fixed by replacing PG=9 with PG=9.6. I chose 9.6 because
it is the only 9.x PG available in appveyor. If we want to test against any
other 9.x PG, then we need to install it separately.

On 07/14/20 23:49, Kartik Ohri wrote:
> This is a known issue. Sometimes, one or two macOS builds fail. The
> underlying cause is that macports is unable to connect using rsync protocol
> to the outside world. This happens once in a while and since we have 10
> macOS VMs in a build, it happens in every or every second build.

Does Travis have any third status for a build, between 'succeeded' and
'failed', like 'skipped'?

Could we then first check whether the prerequisite bits have been
downloaded, and set the result to skipped? Just to keep the spurious
red off the dashboard ....

Regards,
-Chap

>
> Does Travis have any third status for a build, between 'succeeded' and
> 'failed', like 'skipped'?
>
It is not possible to mark skipped. It is possible to mark succeeded but I
think that will be more dangerous than the red. I had a thought that we
could have a serverless function in the cloud somewhere. We can try to
detect if the failure is due to download error in macports and send a
signal to the function. Then the job is terminated immediately. The
function then triggers this failed job by submitting an API call. Since,
the issue vanishes on retrying, doing such retries for at most a 2-3 times
would work. What do you think?

On 07/15/20 07:57, Kartik Ohri wrote:
>>
>> Does Travis have any third status for a build, between 'succeeded' and
>> 'failed', like 'skipped'?
>>
> It is not possible to mark skipped. It is possible to mark succeeded but I
> think that will be more dangerous than the red. I had a thought that we
> could have a serverless function in the cloud somewhere. We can try to
> detect if the failure is due to download error in macports and send a
> signal to the function. Then the job is terminated immediately. The
> function then triggers this failed job by submitting an API call. Since,
> the issue vanishes on retrying, doing such retries for at most a 2-3 times
> would work. What do you think?

That sounds rather elaborate. Maybe it would be enough to simply check
for the prerequisites early, and just fail the job with a very short
log file that says MACPORTS FAILED TO DOWNLOAD - RE-RUN JOB. There would
still be red on the dashboard but it would be easy to see why.

Regards,
-Chap

On 07/14/20 23:30, Kartik Ohri wrote:

> There is another error as well on PG 10
> https://ci.appveyor.com/project/amCap1712/pljava/builds/34099990/job/bgpqdy5qjkik2ya9,
> one of the tests fails. This has been the case since I first added this
> configuration.

Hmm, yes, that appears to be this one:

https://github.com/tada/pljava/blob/6a83db4/pljava-examples/src/main/java/org/postgresql/pljava/example/annotation/SetOfRecordTest.java#L34

It works for me in PG10 on Linux.

Can we just look at the output of

SELECT * FROM javatest.executeselecttorecords(
'select ''Foo'', 1, 1.5::float, 23.67, ''2005-06-01'',
''20:56''::time, ''192.168.0''')
AS r(t_varchar varchar, t_integer integer, t_float float,
t_decimal decimal(8,2), t_date date, t_time time, t_cidr cidr);

on PG10 in Windows?

Regards,
-Chap

On 07/14/20 19:02, Chapman Flack wrote:
> then on Java 9 there is
> still one JNI reference warning, but that's being treated as a failure.
> ...
> looks a little trickier, as you're probably checking for
> warning output from install_jar(...examples...), which really does
> indicate trouble, but the WARNING: JNI local refs: does not.

Looking a little more closely...

psql : INFO: WARNING: JNI local refs: 40, exceeds capacity: 39

It is coming out INFO: WARNING: ...

The WARNING: is part of the message from Xcheck:jni, but it's being
deliberately passed to PostgreSQL's ereport at level INFO [1].

So, probably, by being careful enough to use the first INFO/WARNING tag
to distinguish the cases, we could avoid having that look
like a failure.

Regards,
-Chap

[1]
https://github.com/tada/pljava/blob/6f63568/pljava-so/src/main/c/Backend.c#L1092

I probed the ant Script task code. It does nothing much with the
classpaths. It just loads those in the classloader but how it does so is
unclear. Ant has its own classpath utils and uses some from plexus. I also
read this https://maven.apache.org/guides/mini/guide-maven-classloading.html.
Overall, I think the need is to just load the classpaths in the
classloader. The maven docs say this but are not very descriptive of the
process. The docs do refer to Plexus Classworlds. I'll look into it. I'll
try to see if I can view what are the classpaths antrun used in the earlier
build process which may throw some light on what is missing in our
classloader.
Regards,
Kartik.

Another thing, I have discovered is that the <script> tag inside pom.xml is
processed by the antrun plugin to generate a build-main.xml file inside
target/antrun. I believe this might be another issue. It is also in sync
with my observation that I have been unable to find anything about writing
blocks and executing blocks of javascript from pom.xml. I think antrun is
using Plexus Utils to process the script tags in pom.xml before executing
the ant task on it.

Therefore, we might need to do something similar as well to extract the
script from the script tag.

PS: One thing struck me that the javascript blocks seem to unnecessary if
this is the case. The scripting plugin is wrapping the java functions in
javascript and then parsing a javascript block to execute it. If it is
possible to use the jshell as a scripting engine from the mojo, we could
eliminate the javascript thing and script in Java instead. However, there
are a couple of things that need to be verified to make sure this works.

>

On 07/18/20 09:04, Kartik Ohri wrote:
> Therefore, we might need to do something similar as well to extract the
> script from the script tag.

That would surprise me. I think a lot of the details of how the
maven-antrun-plugin works are matters of history: Ant was there first,
then maven-antrun-plugin wanted to run it from maven and ended up
creating a temporary build xml file to do it, and so on.

I think if you were starting from scratch writing a Maven plugin to
allow scripts, you'd just have its configuration section in the pom
accept one element that is a mimetype and another element that's a
string, and go to the ScriptEngineManager and say "please give me
an engine for this mimetype so I can eval this string".

And the only tricky bit for Maven is making sure the ScriptEngineManager
will be able to find the engine on its class path.

> PS: One thing struck me that the javascript blocks seem to unnecessary if
> this is the case. The scripting plugin is wrapping the java functions in
> javascript and then parsing a javascript block to execute it. If it is
> possible to use the jshell as a scripting engine from the mojo, we could
> eliminate the javascript thing and script in Java instead. However, there

I like the jshell very much, but it is quite a different beast. By default
it starts up a second entire JVM and sends it snippets to run and gets
results back. It can be started with "-execution local" to move that
evaluation environment inside the current JVM, but that brings its own
complications. Out of the box, it isn't visible as a javax.script engine;
it's launched a whole different way.

That being said, there is somebody who figured out how to write a
ScriptEngine wrapper for it [1], which I consider very deep magic.

Which still brings us back where we were; if we develop a mojo that can
accept a mimetype and a block of text, and get the script engine for that
mimetype and run the block of text, then if we want to script bits in JS
we can, and if we want to script bits in Java we can, just by declaring
dependencies on the right script engines and using the right mimetypes.

Regards,
-Chap

On 07/18/20 09:55, Chapman Flack wrote:
> That being said, there is somebody who figured out how to write a
> ScriptEngine wrapper for it [1], which I consider very deep magic.

[1] https://github.com/eobermuhlner/jshell-scriptengine

I found this resource
http://blog.chalda.cz/2018/02/17/Maven-plugin-and-fight-with-classloading.html.
Using it I modified my mojo code to update the class loader. It didn't
work. I also used the ClassLoaderUtils referred in to view what resources
the class loader was loading.

This is the output from a standalone program where I call the script engine
to print the name of all script engines it can find. It is able to find
Oracle Nashorn.
[jdk(dot)internal(dot)loader(dot)ClassLoaders$AppClassLoader(at)5bc2b487]
hashCode=1539486855
-> loader resource: null
-> loader resource element: file:/home/amcap1712/Desktop/
-> loader resource element:
jar:file:/usr/share/java/java-atk-wrapper.jar!/META-INF
[jdk(dot)internal(dot)loader(dot)ClassLoaders$PlatformClassLoader(at)78308db1]
hashCode=2016447921
-> loader resource: null
-> loader resource element:
jar:file:/usr/share/java/java-atk-wrapper.jar!/META-INF
context class
loader=[jdk(dot)internal(dot)loader(dot)ClassLoaders$AppClassLoader(at)5bc2b487]
hashCode=1539486855

This is the output from the mojo where it is not able to find any script
engine.
java(dot)net(dot)URLClassLoader(at)14840ffe] hashCode=344199166
-> loader resource: null
-> loader resource element:
jar:file:/usr/share/java/java-atk-wrapper.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/pljava/pljava-pgxs/target/pljava-pgxs-0.0.1-SNAPSHOT.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/.m2/repository/org/codehaus/plexus/plexus-utils/1.1/plexus-utils-1.1.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/Downloads/idea-IC-193.6911.18/plugins/maven/lib/maven3/boot/plexus-classworlds-2.6.0.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/Downloads/idea-IC-193.6911.18/lib/idea_rt.jar!/META-INF
[ClassRealm[plugin>org.postgresql:pljava-pgxs:0.0.1-SNAPSHOT, parent:
jdk(dot)internal(dot)loader(dot)ClassLoaders$AppClassLoader(at)42a57993]]
hashCode=402683266
-> loader resource: null
-> loader resource element:
jar:file:/usr/share/java/java-atk-wrapper.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/pljava/pljava-pgxs/target/pljava-pgxs-0.0.1-SNAPSHOT.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/.m2/repository/org/codehaus/plexus/plexus-utils/1.1/plexus-utils-1.1.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/Downloads/idea-IC-193.6911.18/plugins/maven/lib/maven3/boot/plexus-classworlds-2.6.0.jar!/META-INF
-> loader resource element:
jar:file:/home/amcap1712/Downloads/idea-IC-193.6911.18/lib/idea_rt.jar!/META-INF
context class loader=[java(dot)net(dot)URLClassLoader(at)14840ffe] hashCode=344199166

What surprised me is that the only entry in the first case is present in
the second case but still the second fails, Am I missing something here ?

On 07/18/20 13:24, Kartik Ohri wrote:
> What surprised me is that the only entry in the first case is present in
> the second case but still the second fails, Am I missing something here ?

I am guessing a little bit from the output your code is showing without
seeing the actual code, but as I refer back to the commit comments for
6cd539b, it seems that the key thing Java 9 changed was that Nashorn
went from the boot classpath (the one with classloader null) to the
application classpath (the one implemented by
jdk.internal.loader.ClassLoaders$AppClassLoader).

I don't think the Nashorn engine is found in any of the specific
URLs that you see when you enumerate the classpath; it's still
internal to the Java installation, somewhere the AppClassLoader just
knows about.

In the Maven case, I think the key thing is some change in the rule
for delegating to the AppClassLoader. In the output you've shown,
I assume that java.net.URLClassLoader is the one you created from
explicit URL list, and you made its parent the classloader you were
initially given, which would be that ClassRealm instance. I see that
it has the AppClassLoader as its parent, and the "normal" Java
classloading rule is you ask your parent first, then only look in
your own places if the parent drew a blank. But I think the ClassRealm
changes that rule in some way. There was something in that MANTRUN-200
issue suggesting they did that because they wanted Maven builds to be
predictable regardless of what was on the application class path.
(Which, ok, I guess, but ... seriously ... ?)

But I would really like to know what ends up happening when a Maven
classpathref is passed to Ant, and ends up solving the problem.
What is a Maven classpathref? Is it just a list of URLs, or some
other object?

I wonder if it ends up working sort of by accident, maybe Ant uses
the classpathref to make a more usual classloader structure that has
the same URLs as the ClassRealm loader, but delegates from there to
the AppClassLoader instead of the ClassRealm. That could be plausible,
as Ant came first and wouldn't know about a ClassRealm. It would be
good to see it in the code though.

Ultimately we want a classloader that will work whether the engine
is part of the application classpath (as Nashorn on Java <15) OR is
in one of the explicit classpath jars (as in Java 15 if we download
Graal's js-scriptengine, or if we play with jshell-scriptengine).

Regards,
-Chap

>
> But I would really like to know what ends up happening when a Maven
> classpathref is passed to Ant, and ends up solving the problem.
> What is a Maven classpathref? Is it just a list of URLs, or some
> other object?
>

I went through the relevant codebase again going through the files a
Project or a Script Task call. As the manual mentions here, [1]
<https://ant.apache.org/manual/Tasks/script.html> a classpathref is just a
reference to paths stored elsewhere, I also realized that I had it all
inside out. maven.plugin.classpath is not something that maven declares. It
is declared by the maven antrun plugin for use by ant tasks [2]
<https://github.com/apache/maven-antrun-plugin/blob/8e5b3c613e96c20881347ec16813864a2d8ef0bb/src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java#L383>.
I had completely understood the opposite of it. This maven-antrun docs
verify this [3]
<https://maven.apache.org/plugins/maven-antrun-plugin/examples/classpaths.html>.
This means that adding classpathref="maven.plugin.classpath" to our
scripts will not be useful. The maven plugin classpath is essentially the
reference to artifact paths. In our case, the maven.plugin.classpath would
be referring to,
/home/amcap1712/pljava/pljava-pgxs/target/pljava-pgxs-0.0.1-SNAPSHOT.jar
/home/amcap1712/.m2/repository/org/codehaus/plexus/plexus-utils/1.1/plexus-utils-1.1.jar
joined into using a file separator. I used the code from maven-antrun to
generate this [4]
<https://github.com/apache/maven-antrun-plugin/blob/8e5b3c613e96c20881347ec16813864a2d8ef0bb/src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java#L398>
.

I wonder if it ends up working sort of by accident, maybe Ant uses
> the classpathref to make a more usual classloader structure that has
> the same URLs as the ClassRealm loader, but delegates from there to
> the AppClassLoader instead of the ClassRealm. That could be plausible,
> as Ant came first and wouldn't know about a ClassRealm. It would be
> good to see it in the code though.
>

Ant uses a custom AntLoader which extends the ClassLoader [5]
<https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/AntClassLoader.java>.
It is heavily customized and over 1600 lines. I am still going through it
and trying to understand what it does but it is definitely more than a
URLClassLoader. But I am sure, we might be able to chomp down a lot from
this class loader even if we have to implement one of our own. To
supplement the AntLoaderClass, Ant also has a ClasspathUtils class [6]
<https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/util/ClasspathUtils.java>
which
also provides an instance of an AntLoader. I think this class only serves
as a convenience class and to implement good software design patterns.

------------------------

The actual script evaluation happens here [6]
<https://github.com/apache/ant/blob/1ce1cc2317cb2b2773ab6b765f1a79ccd4806926/src/main/org/apache/tools/ant/util/optional/JavaxScriptRunner.java#L94>,
assuming Apache BSF is not present. (Since, the scripts do not specify the
manager to use, BSF is preferred if present according to [1] above. If BSF
is absent, then javax.script is used.) Most of the code in the function is
uninteresting and generic (in my opinion) , however one interesting thing
does happen. Before executing the script, Ant changes the ClassLoader and
stores the ClassLoader which it replaces. After the script has been
evaluated, it again switches the ClassLoader to the one which was before it
was replaced in the first place. I do not understand why it is done.

Some other interesting or mysterious things I found in the code related to
the Script task. Here [7]
<https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/util/ScriptRunnerCreator.java#L110>,
Ant loads the javax.script (or BSF) manager class manually into the
classpath which I don't understand why (to be fair, I don't know what
Class.forName even does, my best guess is load the class into classpath).
Then Here [8]
<https://github.com/apache/ant/blob/1ce1cc2317cb2b2773ab6b765f1a79ccd4806926/src/main/org/apache/tools/ant/util/ScriptRunnerHelper.java#L225>,
Ant chooses between three methods to create a classloader. Again, I don't
have the slightest idea why so.

This is all I could gather from the source. I think our answer may lie
inside this most probably inside AntLoader. I'll try to study the AntLoader
class to grasp it and find out the cause.

Regards,
Kartik

On 07/18/20 15:49, Kartik Ohri wrote:
> inside out. maven.plugin.classpath is not something that maven declares. It
> is declared by the maven antrun plugin for use by ant tasks [2]
> <https://github.com/apache/maven-antrun-plugin/blob/8e5b3c613e96c20881347ec16813864a2d8ef0bb/src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java#L383>.

Aha! And it generates that using getPathFromArtifacts(), which just goes
through its own dependency list and collects the jar names (which are
maybe the exact same jar names its current ClassRealm loader is using).

I wonder what would happen just making a dead-simple ClassLoader with
two "parents" and otherwise no functionality of its own. Of course it
could only have one real parent and an instance field for the 'extra' one.

Create it so one 'parent' is the ClassRealm loader that loaded us, and the
other is from getSystemClassLoader() (they changed the official name to
"application class loader" but didn't rename the method). Have its findClass
method just say "either parent A has it or parent B has it or I haven't
heard of it, sorry."

(In fact, findClass doesn't get called until the 'real' parent has already
been checked, so it doesn't need to do anything besides check the 'extra'
parent and return what it finds, if anything.)

That would leave the question of which parent should be the real one and
which should be the extra one, since the real one is the one that gets
searched first. In Java tradition, the app loader would be the real one.
For Maven it might be better to make the ClassRealm the real one, so
everything Maven constructed gets used, except when it fails to find
something.

I think you can pass a chosen ClassLoader directly to the
ScriptEngineManager constructor, so this special loader wouldn't need
to be used anywhere but there.

What Could Possibly Go Wrong?™

Regards,
-Chap

>
> Aha! And it generates that using getPathFromArtifacts(), which just goes
> through its own dependency list and collects the jar names (which are
> maybe the exact same jar names its current ClassRealm loader is using).
>
> I wonder what would happen just making a dead-simple ClassLoader with
> two "parents" and otherwise no functionality of its own. Of course it
> could only have one real parent and an instance field for the 'extra' one.
>
> Create it so one 'parent' is the ClassRealm loader that loaded us, and the
> other is from getSystemClassLoader() (they changed the official name to
> "application class loader" but didn't rename the method). Have its
> findClass
> method just say "either parent A has it or parent B has it or I haven't
> heard of it, sorry."
>
> (In fact, findClass doesn't get called until the 'real' parent has already
> been checked, so it doesn't need to do anything besides check the 'extra'
> parent and return what it finds, if anything.)
>

There is an issue with calling findClass on the instance field because it
has protected visibility. loadClass on the other hand is public.

>
> There is an issue with calling findClass on the instance field because it
> has protected visibility. loadClass on the other hand is public.
>

I tried with both passing the current context loader and system class
loader. getSystemClassLoader works in finding the JavaScript engine. So, I
tried passing ClassLoader.getSystemClassLoader directly to
ScriptEngineManager's constructor and it works the same. It is able to find
the engine. This could come with the issue that graal vm may not be on the
system class loader path so we need to think how to work that out. But in
the meantime, we can try to progress on the loading script from the pom.

Regards,
Kartik

With using the System Class Loader as a temporary fix, I have been able to
get the build working. I have pushed the changes. How can I test with Graal
or other engines ?

>

On 07/19/20 04:42, Kartik Ohri wrote:
> With using the System Class Loader as a temporary fix, I have been able to
> get the build working. I have pushed the changes. How can I test with Graal
> or other engines ?

I have just made a pull request on amCap1712/plugin that uses the
two-parented class loader idea. It turns out the idea wasn't completely
broken, but there's only one choice for which parent goes first.

I learned a lot more about how the Java 9 module system changed how
ServiceLoader works. That turns out to be the issue, and not so much
that the application class loader was just being left out. (And it turns
out the application class loader isn't needed to find Nashorn anyway;
the platform class loader can find it.)

To test with Graal, you can (1) do a build with Java 15 and see that
it breaks because Nashorn is gone :), and then (2) merge the
chore/master/denashorn branch and edit the main (parent) pom.xml, and
change the nashorngone profile so the two graal jars are added as
dependencies of org.postgresql:pljava-pgxs instead of maven-antrun-plugin.

Something isn't quite right about the handling of -Dpljava.libjvmdefault;
the C compiler gives syntax errors if I use it (the build succeeds if
I don't). Running with -X shows this being passed to the compiler:

-DPLJAVA_LIBJVMDEFAULT=${pljava.qlibjvmdefault}

so maybe there's a simple problem like one last line in the POM that
got overlooked, or something.

Of course it's kind of guaranteed not to work right on Windows, for
reasons we already know, but it ought to work on Linux.

Regards,
-Chap

On 07/19/20 18:03, Chapman Flack wrote:
> To test with Graal, you can (1) do a build with Java 15 and see that
> it breaks because Nashorn is gone :), and then (2) merge the
> chore/master/denashorn branch and edit the main (parent) pom.xml, and
> change the nashorngone profile so the two graal jars are added as
> dependencies of org.postgresql:pljava-pgxs instead of maven-antrun-plugin.

I meant to ask also: is it possible for the plugin configuration's
<script> tag to allow attributes, such as

<script mimetype="application/javascript">
...
</script>

or

<script engine="nashorn">...</script>

I have noticed that Maven configurations tend not to use attributes
much, and instead make everything its own element, like

<scriptLanguage>nashorn</scriptLanguage>
<script>...</script>

but I have not checked whether Maven is actually incapable of using
attributes in configs, or that has just been a common style among
existing plugins.

I would find the style with an attribute to be more compact and
clear.

Regards,
-Chap

>
> I meant to ask also: is it possible for the plugin configuration's
> <script> tag to allow attributes, such as
>
> <script mimetype="application/javascript">
> ...
> </script>
>
> or
>
> <script engine="nashorn">...</script>
>
It is possible to have this syntax. We need to specify an executionID in
the executionBlock for that. In the plugin, we can get the entire
configuration XML as a Plexus Xpp3Dom object from which we can retrieve the
attribute from any element. I'll add this. What attribute should be used
though mimetype or engine or allow both and give engine precedence over
mimetype ?

>
> Something isn't quite right about the handling of -Dpljava.libjvmdefault;
> the C compiler gives syntax errors if I use it (the build succeeds if
> I don't). Running with -X shows this being passed to the compiler:
>
> -DPLJAVA_LIBJVMDEFAULT=${pljava.qlibjvmdefault}
>
It is because the plugin is no longer detecting PLJAVA_LIBJVMDEFAULT. I did
not retain the method to automatically detect the variable and use it and
set its value. Should we keep it in the configuration block in pom.xml
since we have exposed the C quoting as well to the script engine or make an
exception for this to be specified via command line ?

On Tue, Jul 21, 2020 at 8:29 AM Kartik Ohri <kartikohri13(at)gmail(dot)com> wrote:

> Something isn't quite right about the handling of -Dpljava.libjvmdefault;
>> the C compiler gives syntax errors if I use it (the build succeeds if
>> I don't). Running with -X shows this being passed to the compiler:
>>
>> -DPLJAVA_LIBJVMDEFAULT=${pljava.qlibjvmdefault}
>>
> It is because the plugin is no longer detecting PLJAVA_LIBJVMDEFAULT. I
> did not retain the method to automatically detect the variable and use it
> and set its value. Should we keep it in the configuration block in pom.xml
> since we have exposed the C quoting as well to the script engine or make an
> exception for this to be specified via command line ?
>
To be sure and also to confirm, I didn't misunderstand something, can you
share the configuration block and the maven command you executed ?

On 07/20/20 23:00, Kartik Ohri wrote:
> On Tue, Jul 21, 2020 at 8:29 AM Kartik Ohri <kartikohri13(at)gmail(dot)com> wrote:
>
>> Something isn't quite right about the handling of -Dpljava.libjvmdefault;
>>> the C compiler gives syntax errors if I use it (the build succeeds if
>>> I don't). Running with -X shows this being passed to the compiler:
>>>
>>> -DPLJAVA_LIBJVMDEFAULT=${pljava.qlibjvmdefault}
>>>
>> It is because the plugin is no longer detecting PLJAVA_LIBJVMDEFAULT. I
>> did not retain the method to automatically detect the variable and use it
>> and set its value. Should we keep it in the configuration block in pom.xml
>> since we have exposed the C quoting as well to the script engine or make an
>> exception for this to be specified via command line ?
>>
> To be sure and also to confirm, I didn't misunderstand something, can you
> share the configuration block and the maven command you executed ?

I didn't change the configuration block; my usual mvn command is like:

mvn -Duser.home=/var/tmp/nohome -Ppgjdbc-ng -Psaxon-examples -Pwnosign \
-Dnar.cores=1 -Dso.debug=true \
-Dpljava.libjvmdefault=/var/tmp/jdk-14+36/lib/server/libjvm.so \
-Dpgsql.pgconfig=/var/tmp/nohome/pg$v/bin/pg_config clean install

The purpose is for -Dpgsql.pgconfig to be specified via command line;
it is there so that package maintainers (e.g. for Debian) can have it
built to refer to the default libjvm location on a deployed system
according to their packaging conventions. That doesn't have to be the
same as the jvm being used to build, so autodetecting wouldn't help.
It should be up to the package maintainer to choose what
-Dpljava.libjvmdefault path to give.

At present, how it gets from there to the compiler is Rube Goldberg;
the command line sets pljava.libjvmdefault, an antrun script checks if
that is non-null and defines a C-quoted version pljava.qlibjvmdefault,
and the haslibjvmdefault profile in pom.xml is activated if the first
property (without the q) exists, and when activated, it augments the
maven-nar-plugin configuration with a C compiler define for
PLJAVA_LIBJVMDEFAULT=${pljava.qlibjvmdefault}.

That could be a lot less circuitous once the nar plugin is out of the
picture, but for right now that's the way it works.

If the block of script had a test for the non-null system property
and set the 'q' property from it (like the last 8 lines of the old
antrun script), that at least would preserve the current behavior.

Regards,
-Chap

On 07/20/20 22:51, Kartik Ohri wrote:
>> <script engine="nashorn">...</script>
>>
> It is possible to have this syntax. We need to specify an executionID in
> the executionBlock for that. In the plugin, we can get the entire
> configuration XML as a Plexus Xpp3Dom object from which we can retrieve the
> attribute from any element. I'll add this. What attribute should be used
> though mimetype or engine or allow both and give engine precedence over
> mimetype ?

I like allowing both; giving engine preference, or just complaining if
both are specified. (Or, if both are specified, look up by engine, then
complain if its mimetypes don't include mimetype.)

By the way, when allowing an attribute to specify the script language,
it then becomes trickier to make functions available to the script
by eval'ing function definitions for them, as the mojo now does; if
those are in JavaScript and somebody wants to write a script block
in Tcl, that'll be trouble. (Not that I have any actual plans to add
Tcl scripts to this build, but....)

The ScriptEngineFactory has useful methods getMethodCallSyntax and
getOutputStatement, but it doesn't really have a
"getWholeFunctionDeclaration" method to help out there.

What I have found does work with Nashorn (haven't tried it with Graal yet)
is just engine.put (or getContext().setAttribute()), passing a functional
interface instance, e.g.:

engine.put("quoteStringForC",
(Function<String,String>)PGXSUtils::quoteStringForC);

I'm not sure that works with every script engine out there (but then
again, neither would defining the functions as JavaScript). If it does
work with both Nashorn and Graal, it seems the cleanest way.

Regards,
-Chap

> > It is possible to have this syntax. We need to specify an executionID in
> > the executionBlock for that. In the plugin, we can get the entire
> > configuration XML as a Plexus Xpp3Dom object from which we can retrieve
> the
> > attribute from any element. I'll add this. What attribute should be used
> > though mimetype or engine or allow both and give engine precedence over
> > mimetype ?
>
There is one caveat though. This means we need to hard code the executionID
in the mojo. It might be problematic when we specify multiple execution
blocks for different phases. But I am not sure how that works, is a new
Mojo instance initiated for each execution if there are multiple ones since
each execution block has its own configuration block?

On 07/21/20 14:03, Kartik Ohri wrote:
>>> It is possible to have this syntax. We need to specify an executionID in
>>> the executionBlock for that. In the plugin, we can get the entire
>>> configuration XML as a Plexus Xpp3Dom object from which we can retrieve
>> the
>>> attribute from any element. I'll add this. What attribute should be used
>>> though mimetype or engine or allow both and give engine precedence over
>>> mimetype ?
>>
> There is one caveat though. This means we need to hard code the executionID
> in the mojo. It might be problematic when we specify multiple execution
> blocks for different phases. But I am not sure how that works, is a new
> Mojo instance initiated for each execution if there are multiple ones since
> each execution block has its own configuration block?

Hmm. That does sound worrisome. And I don't, at present, know the answer
on instantiation; I would assume different executions are distinct instances.

I have just had one of those déjà vu moments of thinking "wait, haven't
I wondered about this before?" and it seems I did, five years ago (which
I guess is long enough to forget).

https://stackoverflow.com/a/32800814/4062350

Is it possible to declare the script parameter as a PlexusConfiguration?
Does that lead anywhere useful?

Regards,
-Chap

>
> Hmm. That does sound worrisome. And I don't, at present, know the answer
> on instantiation; I would assume different executions are distinct
> instances.
>
> I have just had one of those déjà vu moments of thinking "wait, haven't
> I wondered about this before?" and it seems I did, five years ago (which
> I guess is long enough to forget).
>
> https://stackoverflow.com/a/32800814/4062350
>
> Is it possible to declare the script parameter as a PlexusConfiguration?
> Does that lead anywhere useful?
>

I had already come across your answer in my search and figured that you
might have forgotten about it :). I have seen antrun's source. I initially
wanted to have something like it but was unable to figure out how
antrun does that. I left on the speculation that it is another one of ant's
dark magic. JSR-330 seems interesting but I have not been able to find
enough documentation or examples around it. Even maven plugins seem to be
using Plexus configuration years after they announced the alternative.

We could use a hack. Specify the execution id in the configuration block
also in the pom and have maven give us the id to fetch the block but then
it is equivalent to having a scriptLanguage item in the configuration
block. More, actually since we need to specify the id twice in this case. I
say we continue to work this path and tackle the issue when we face it. We
could always revert to the scriptLanguage thing if need be.

I tested with Java 15 and it works. But surprisingly, it still finds
Nashorn and does not find Graal at all. Here is the build log,
https://travis-ci.com/github/amCap1712/pljava/jobs/363479322 and the branch
I tested with https://github.com/amCap1712/pljava/tree/test-graal.

>

On 07/21/20 16:28, Kartik Ohri wrote:
> I tested with Java 15 and it works. But surprisingly, it still finds
> Nashorn and does not find Graal at all. Here is the build log,
> https://travis-ci.com/github/amCap1712/pljava/jobs/363479322 and the branch
> I tested with https://github.com/amCap1712/pljava/tree/test-graal.

While I see in that log that an 'apt remove openjdk-11-jdk' happened,
and also an installation of 15-ea, it looks like openjdk-14 was also
present and is probably what mvn was using.

I usually don't bother removing jdk versions; I have several installed
at one time, and just use, e.g.,

JAVA_HOME=/var/tmp/jdk-15/ mvn clean install

which is enough to determine which installed Java is used to run Maven.

Regards,
-Chap

On Wed, Jul 22, 2020 at 2:15 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/21/20 16:28, Kartik Ohri wrote:
> > I tested with Java 15 and it works. But surprisingly, it still finds
> > Nashorn and does not find Graal at all. Here is the build log,
> > https://travis-ci.com/github/amCap1712/pljava/jobs/363479322 and the
> branch
> > I tested with https://github.com/amCap1712/pljava/tree/test-graal.
>
> While I see in that log that an 'apt remove openjdk-11-jdk' happened,
> and also an installation of 15-ea, it looks like openjdk-14 was also
> present and is probably what mvn was using.
>
> I usually don't bother removing jdk versions; I have several installed
> at one time, and just use, e.g.,
>
> JAVA_HOME=/var/tmp/jdk-15/ mvn clean install
>
> I had set JAVA_HOME and ran javac -version which pointed that Java 15 was
being used but I tested again to be sure.I specified the Java Home in the
mvn statement and ran with debug flags to see which version was running.
You can see around line 473 here
https://travis-ci.com/github/amCap1712/pljava/jobs/363479322. The result is
the same: the script engine finds nashorn but not graal.

> which is enough to determine which installed Java is used to run Maven.
>
> Regards,
> -Chap
>

On 07/21/20 17:00, Kartik Ohri wrote:
>> I had set JAVA_HOME and ran javac -version which pointed that Java 15 was
> being used but I tested again to be sure.I specified the Java Home in the
> mvn statement and ran with debug flags to see which version was running.
> You can see around line 473 here
> https://travis-ci.com/github/amCap1712/pljava/jobs/363479322. The result is
> the same: the script engine finds nashorn but not graal.

How odd. When I run the tip of test-graal here with 15-ea, I simply get
"No suitable engine found for specified engine name or mime type".

If I go to the parent-project POM and edit the nashorngone profile
so that the two graal jars are added as dependencies of pljava-pgxs,
then I run it again, it finds graal. The dependency-collector debug
output now shows the dependencies:

[DEBUG] org.postgresql:pljava-pgxs:jar:0.0.1-SNAPSHOT:
[DEBUG] org.graalvm.js:js:jar:20.1.0:runtime
[DEBUG] org.graalvm.regex:regex:jar:20.1.0:runtime
[DEBUG] org.graalvm.truffle:truffle-api:jar:20.1.0:runtime
[DEBUG] org.graalvm.sdk:graal-sdk:jar:20.1.0:runtime
[DEBUG] org.ow2.asm:asm:jar:7.1:runtime
[DEBUG] org.ow2.asm:asm-tree:jar:7.1:runtime
[DEBUG] org.ow2.asm:asm-analysis:jar:7.1:runtime
[DEBUG] org.ow2.asm:asm-commons:jar:7.1:runtime
[DEBUG] org.ow2.asm:asm-util:jar:7.1:runtime
[DEBUG] com.ibm.icu:icu4j:jar:66.1:runtime
[DEBUG] org.graalvm.js:js-scriptengine:jar:20.1.0:runtime
[DEBUG] org.codehaus.plexus:plexus-utils:jar:1.1:runtime
[DEBUG] Created new class realm plugin>org.postgresql:pljava-pgxs:0.0.1-SNAPSHOT
[DEBUG] Importing foreign packages into class realm plugin>org.postgresql:pljava
-pgxs:0.0.1-SNAPSHOT
[DEBUG] Imported: < project>org.postgresql:pljava-so:1.6.0-SNAPSHOT
[DEBUG] Populating class realm plugin>org.postgresql:pljava-pgxs:0.0.1-SNAPSHOT
[DEBUG] Included: org.postgresql:pljava-pgxs:jar:0.0.1-SNAPSHOT
[DEBUG] Included: org.graalvm.js:js:jar:20.1.0
[DEBUG] Included: org.graalvm.regex:regex:jar:20.1.0
[DEBUG] Included: org.graalvm.truffle:truffle-api:jar:20.1.0
[DEBUG] Included: org.graalvm.sdk:graal-sdk:jar:20.1.0
[DEBUG] Included: org.ow2.asm:asm:jar:7.1
[DEBUG] Included: org.ow2.asm:asm-tree:jar:7.1
[DEBUG] Included: org.ow2.asm:asm-analysis:jar:7.1
[DEBUG] Included: org.ow2.asm:asm-commons:jar:7.1
[DEBUG] Included: org.ow2.asm:asm-util:jar:7.1
[DEBUG] Included: com.ibm.icu:icu4j:jar:66.1
[DEBUG] Included: org.graalvm.js:js-scriptengine:jar:20.1.0
[DEBUG] Included: org.codehaus.plexus:plexus-utils:jar:1.1

and the logging you added:

[ERROR] Graal.js
[INFO] setProjectProperty("PGSQL_BINDIR",
...
[DEBUG] com(dot)oracle(dot)truffle(dot)js(dot)scriptengine(dot)GraalJSScriptEngine(at)66dfc523

So I think the dependency info explains why your run was not finding
graal. I remain baffled regarding how your run was managing to find
Nashorn instead. I wonder if you downloaded a more recent 15-ea than
I have, and there was a chorus of complaints and they put Nashorn back
in for now?

I would be interested to see what a similar Travis run will do after
making sure the graal dependencies are listed: will it then find nashorn,
or graal?

Regards,
-Chap

On 07/21/20 14:55, Kartik Ohri wrote:
> I had already come across your answer in my search and figured that you
> might have forgotten about it :). I have seen antrun's source. I initially
> wanted to have something like it but was unable to figure out how
> antrun does that.

That made me curious, so I took your plugin branch and just gave the
'script' parameter the type PlexusConfiguration, and it did pretty much
what I thought antrun was doing. Nothing up my sleeve. :)

https://github.com/jcflack/pljava/commits/PlexusConfig

I think that probably is preferable to the look-up-the-Xpp3Dom business,
as it doesn't require the hardcoding of the artifact IDs or execution ID,
etc. (I still wonder whether there might also be a way to get the
MojoExecution injected easily rather than having to look it up. But this
worked, so I stopped looking.)

Is it possible that the test condition for "your engine doesn't support
your mimetype" could be backwards?

Regards,
-Chap

On Wed, Jul 22, 2020 at 4:00 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/21/20 14:55, Kartik Ohri wrote:
> > I had already come across your answer in my search and figured that you
> > might have forgotten about it :). I have seen antrun's source. I
> initially
> > wanted to have something like it but was unable to figure out how
> > antrun does that.
>
> That made me curious, so I took your plugin branch and just gave the
> 'script' parameter the type PlexusConfiguration, and it did pretty much
> what I thought antrun was doing. Nothing up my sleeve. :)

>
https://github.com/jcflack/pljava/commits/PlexusConfig
>
> One less dark magical secret. So, this means that Maven looks
for @Parameter annotated variables and matches them to xml dom elements.
And the supertype for maven plugin parameter type matching is
PlexusConfiguration (or something it inherits).

Is it possible that the test condition for "your engine doesn't support
> your mimetype" could be backwards?

To be sure, you mean the mimetype should be preferred over engine ?

Regards,
Kartik.

>
> I think that probably is preferable to the look-up-the-Xpp3Dom business,
> as it doesn't require the hardcoding of the artifact IDs or execution ID,
> etc. (I still wonder whether there might also be a way to get the
> MojoExecution injected easily rather than having to look it up. But this
> worked, so I stopped looking.)
>
Since, the PlexusConfiguration thing worked, I tried
@Component
MojoExecution execution;
which worked. This seems to be what JSR-330 aimed at. If only the maven
plugin docs were more friendly.

On 07/21/20 22:25, Kartik Ohri wrote:
> If only the maven plugin docs were more friendly.

You're going to have to say that a lot before you'll catch up
to the number of times I've said it.

On 07/21/20 22:19, Kartik Ohri wrote:
> To be sure, you mean the mimetype should be preferred over engine ?

No, but in looking quickly at the code (I didn't actually try it)
I thought you might end up issuing a warning that the engine
/doesn't/ support the MIME type, in just the case where the engine
/does/ support it.

Regards,
-Chap

On Wed, Jul 22, 2020 at 3:03 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/21/20 17:00, Kartik Ohri wrote:
> >> I had set JAVA_HOME and ran javac -version which pointed that Java 15
> was
> > being used but I tested again to be sure.I specified the Java Home in the
> > mvn statement and ran with debug flags to see which version was running.
> > You can see around line 473 here
> > https://travis-ci.com/github/amCap1712/pljava/jobs/363479322. The
> result is
> > the same: the script engine finds nashorn but not graal.
>
> How odd. When I run the tip of test-graal here with 15-ea, I simply get
> "No suitable engine found for specified engine name or mime type".
>
> If I go to the parent-project POM and edit the nashorngone profile
> so that the two graal jars are added as dependencies of pljava-pgxs,
>

I had missed it during the merge. I had overlooked that the jars were added
as a dependency of maven-antrun. So, I added this fix but the CI failed (
https://travis-ci.com/github/amCap1712/pljava/jobs/363573395 commit SHA
8aa3e5c07108d944b66f164ad0b70082148a45b1 ) which made me realise that when
the profile is activated the plugin does not exist. The error message also
shows this. Maven looks in local and online repositories. Adding
dependencies directly to plava-pgxs's pom works. I think we could replicate
the profile in pgxs to ensure graal is only added as a dependency if needed.

then I run it again, it finds graal. The dependency-collector debug
> output now shows the dependencies:
>
> [DEBUG] org.postgresql:pljava-pgxs:jar:0.0.1-SNAPSHOT:
> [DEBUG] org.graalvm.js:js:jar:20.1.0:runtime
> [DEBUG] org.graalvm.regex:regex:jar:20.1.0:runtime
> [DEBUG] org.graalvm.truffle:truffle-api:jar:20.1.0:runtime
> [DEBUG] org.graalvm.sdk:graal-sdk:jar:20.1.0:runtime
> [DEBUG] org.ow2.asm:asm:jar:7.1:runtime
> [DEBUG] org.ow2.asm:asm-tree:jar:7.1:runtime
> [DEBUG] org.ow2.asm:asm-analysis:jar:7.1:runtime
> [DEBUG] org.ow2.asm:asm-commons:jar:7.1:runtime
> [DEBUG] org.ow2.asm:asm-util:jar:7.1:runtime
> [DEBUG] com.ibm.icu:icu4j:jar:66.1:runtime
> [DEBUG] org.graalvm.js:js-scriptengine:jar:20.1.0:runtime
> [DEBUG] org.codehaus.plexus:plexus-utils:jar:1.1:runtime
> [DEBUG] Created new class realm
> plugin>org.postgresql:pljava-pgxs:0.0.1-SNAPSHOT
> [DEBUG] Importing foreign packages into class realm
> plugin>org.postgresql:pljava
> -pgxs:0.0.1-SNAPSHOT
> [DEBUG] Imported: < project>org.postgresql:pljava-so:1.6.0-SNAPSHOT
> [DEBUG] Populating class realm
> plugin>org.postgresql:pljava-pgxs:0.0.1-SNAPSHOT
> [DEBUG] Included: org.postgresql:pljava-pgxs:jar:0.0.1-SNAPSHOT
> [DEBUG] Included: org.graalvm.js:js:jar:20.1.0
> [DEBUG] Included: org.graalvm.regex:regex:jar:20.1.0
> [DEBUG] Included: org.graalvm.truffle:truffle-api:jar:20.1.0
> [DEBUG] Included: org.graalvm.sdk:graal-sdk:jar:20.1.0
> [DEBUG] Included: org.ow2.asm:asm:jar:7.1
> [DEBUG] Included: org.ow2.asm:asm-tree:jar:7.1
> [DEBUG] Included: org.ow2.asm:asm-analysis:jar:7.1
> [DEBUG] Included: org.ow2.asm:asm-commons:jar:7.1
> [DEBUG] Included: org.ow2.asm:asm-util:jar:7.1
> [DEBUG] Included: com.ibm.icu:icu4j:jar:66.1
> [DEBUG] Included: org.graalvm.js:js-scriptengine:jar:20.1.0
> [DEBUG] Included: org.codehaus.plexus:plexus-utils:jar:1.1
>
> and the logging you added:
>
> [ERROR] Graal.js
> [INFO] setProjectProperty("PGSQL_BINDIR",
> ...
> [DEBUG] com(dot)oracle(dot)truffle(dot)js(dot)scriptengine(dot)GraalJSScriptEngine(at)66dfc523
>
>
> So I think the dependency info explains why your run was not finding
> graal. I remain baffled regarding how your run was managing to find
> Nashorn instead. I wonder if you downloaded a more recent 15-ea than
> I have, and there was a chorus of complaints and they put Nashorn back
> in for now?
>
I was using the latest JDK 15+32. I checked the release notes which mark
the nashorn removal as delivered. But they go only upto 15+29 till there
is no mention of nashorn again. So, I did a build again and checked again.
This time with automated travis build instead of debug and it detects only
Graal (no nashorn). Despite specifying JAVA_HOME with maven, it seems it
was using a different Java installation or I made an inadvertent error in
the JAVA_HOME due to which it used the wrong installation, :( but plausible.

I would be interested to see what a similar Travis run will do after
> making sure the graal dependencies are listed: will it then find nashorn,
> or graal?
>
I did a build with JDK 14 with the dependencies to find this out.
Interestingly, it fails when both coexist.
https://travis-ci.com/github/amCap1712/pljava/jobs/363575841 commit
SHA b5950dedc7018470e5331f20d9300344a00f40ce I think we will need to add
the profile in that case then.

(The Java 15 build failing in this case is probably because of some error I
made while merging the denashorn branch since the builds on the branch
itself were passing.)

Regards,
Kartik

>
> I would be interested to see what a similar Travis run will do after
>>
> making sure the graal dependencies are listed: will it then find nashorn,
>> or graal?
>>
> I did a build with JDK 14 with the dependencies to find this out.
> Interestingly, it fails when both coexist.
> https://travis-ci.com/github/amCap1712/pljava/jobs/363575841 commit
> SHA b5950dedc7018470e5331f20d9300344a00f40ce I think we will need to add
> the profile in that case then.
>
I duplicated the profile from the parent pom in the plugin and with the
plugin changed to pgxs but it didn't work. But placing it inside each
module did. So, we'll need to have the profile in both pljava-so and
pljava-packaging.

On 07/22/20 16:11, Kartik Ohri wrote:
> I duplicated the profile from the parent pom in the plugin and with the
> plugin changed to pgxs but it didn't work. But placing it inside each
> module did. So, we'll need to have the profile in both pljava-so and
> pljava-packaging.

Well ... that's slightly annoying ... but it could have been worse.

Regards,
-Chap

On Thu, Jul 23, 2020, 4:32 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/22/20 16:11, Kartik Ohri wrote:
> > I duplicated the profile from the parent pom in the plugin and with the
> > plugin changed to pgxs but it didn't work. But placing it inside each
> > module did. So, we'll need to have the profile in both pljava-so and
> > pljava-packaging.
>
> Well ... that's slightly annoying ... but it could have been worse.
>

That is indeed true. I think we can now move to towards replacing nar. Can
you provide some pointers as to where to begin.
Regards,
Kartik.

>

On 07/22/20 23:23, Kartik Ohri wrote:

> That is indeed true. I think we can now move to towards replacing nar. Can
> you provide some pointers as to where to begin.

That's a good place to be. This might be a good moment to take a little time
to review the recent commits and 'rebase -i' them into a tighter story.

It might be worth going back at least to 4301472 and doing the merge of
PR 7 as an actual merge, so the three commits in that PR aren't squashed
into one big one.

It's certainly ok to squash out commits that are just 'fix typo',
adding and removing debug printlns, and so on.

You'll see I'm not of the belief that every single tiny commit needs to
be preserved forever, but also not in the camp that would squash a
sequence of substantive commits all the way down to just one. My position
would be more that a sequence of commits should be thoughtfully edited,
just like any other writing, into a form that eliminates distractions
and tells an easy-to-follow story of how the code got to be the way it is.

The way I edited the commits in PR 7 down to three is one example. The
story is kind of "you would naturally want to delegate to Maven's loader
first" ... "but making it the parent doesn't work because X" ... "and by
the way, it still works using the platform loader and not the application
loader as earlier believed."

Besides making a clear presentation for someone later to understand
the code, that can have another benefit. Suppose in the future there
is a reason that swapping the real-parent/extra-parent in c857ce0
turns out not to be a great idea after all, and a more complicated
solution is needed. At that point, somebody wouldn't have to manually
repeat those changes backwards; they could just start another branch
from an appropriate spot and select and revert that commit.

Another example would be the seventeen commits ending at d9b07ca, edited
down to six commits ending at 9e2e50f. Ideally, for the commits that are
left after editing, it should be easy to write a good commit comment
for each one that explains what was learned or changed in that step.
Somebody later trying to understand the code needs to learn the same things.

Some of that information can be conveyed in regular comments in the code,
which are also necessary. I don't think code comments end the need for
a good git history, or vice versa. They both have their part to play.
Sometimes there will be some detail of a code comment that won't seem
completely clear, but the git history will show the exact code changes
that went with it, and that will clear things up.

When adding Javadoc comments to methods that were first added without
them, I do not object to using rebase -i to get the comments and the
new code to appear in the same commit.

Be careful when re-doing the merge that happened in 75bd8d5: look how
many lines of insignificant spacing changes are shown in the diffs:

- public static void qp(Connection c, String sql) throws Exception
++ public static void qp (Connection c, String sql) throws Exception
+ {
+ qp(q(c, sql));
+ }
+
+ /**
+ * Invoke some {(at)code execute} method on a {(at)code Statement} and pass
- * the {(at)link #q(Statement,Callable) result stream}
++ * the {(at)link #q(Statement, Callable) result stream}
+ * to {(at)link #qp(Stream<Object>)} for printing to standard output.
- *<p>
++ * <p>
+ * This is how, for example, to prepare, then print the results of, a
+ * {(at)code PreparedStatement}:
- *<pre>
++ * <pre>
+ * PreparedStatement ps = conn.prepareStatement("select foo(?,?)");
+ * ps.setInt(1, 42);
+ * ps.setString(2, "surprise!");
+ * qp(ps, ps::execute);
- *</pre>
++ * </pre>
+ * The {(at)code Statement} will be closed.
+ */
- public static void qp(Statement s, Callable<Boolean> work) throws Exception
++ public static void qp (Statement s, Callable<Boolean> work) throws Exception

(just a very small sample!)

Even in a project with specific formatting conventions (like the ones
from PostgreSQL used here), there is still some room for variation.
I don't necessarily think that one way of spacing the parenthesis after
a function, or the tags in a javadoc comment, is better or worse than
another.

But it is important not to make commits that needlessly change all of
the spacing choices made by a collaborator. The reason is it clutters up
the git record and makes it very hard to see what the actual intended
changes were in the commit.

If you are using some IDE that makes those changes without asking,
perhaps it has a setting to make it not do that.

If you are not using something like 'git gui' when making commits, it
would be worth taking a look at: it gives you a very usable way to see
exactly what changes are going to be in your commit, so you can make
sure they will be only the specific things you intended to change.

----

After taking a little break for editing, I think the start on the nar
functionality could follow the basic outline in
https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin#basic-operations

The "platform-specific recipes" mentioned there come from a couple of
places, generic information you can find in PostgreSQL's Makefile.shlib,
and specific values like CFLAGS, LDFLAGS, etc., that are nothing but
a few more calls on getPgConfigProperty().

It is probably good to spend a little design time before coding, thinking
of different ways the generic 'recipes' could be represented. (I assume
you're not going for the "write a GNU Makefile parser" approach.)

There are probably some reasonable ways you could decide to transcribe
different platform 'recipes' from Makefile.shlib into some JavaScript
snippets and put those in a special configuration block for the plugin.

The nar plugin chooses a recipe based on 'arch-OS-linker'. You can get
the arch and OS straight from Java system properties. I would suggest
the plugin do that, then allow JavaScript recipes per arch and OS to
have a method that determines the remaining piece if needed (on Windows
that could be a bit of JavaScript to decide if this in MSVC or MinGW).

----

As for
https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin#argument-passing-workaround-for-windows
I already have some of that now, because I needed it for something else.
So you shouldn't end up needing to conjure that up right from scratch,
though what I've got may need to be generalized further for use in this
plugin, and I would definitely appreciate your help reviewing the
approach for correctness; it's devilish tricky stuff.

----

Also, on the wiki page, I called this "One last thing", but that doesn't
mean it has to be last:

https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin#one-last-thing-a-simple-report-mojo

It would be helpful to me to move that part up, maybe even do it next;
I have a use for it, and in this case I can't use scripting in antrun
to get around it, because antrun doesn't implement MavenReport and so
that is only usable in build phases.

Regards,
-Chap

On Thu, Jul 23, 2020 at 10:35 PM Chapman Flack <chap(at)anastigmatix(dot)net>
wrote:

> On 07/22/20 23:23, Kartik Ohri wrote:
>
> > That is indeed true. I think we can now move to towards replacing nar.
> Can
> > you provide some pointers as to where to begin.
>
> That's a good place to be. This might be a good moment to take a little
> time
> to review the recent commits and 'rebase -i' them into a tighter story.
>
> It might be worth going back at least to 4301472 and doing the merge of
> PR 7 as an actual merge, so the three commits in that PR aren't squashed
> into one big one.
>
> It's certainly ok to squash out commits that are just 'fix typo',
> adding and removing debug printlns, and so on.
>
> You'll see I'm not of the belief that every single tiny commit needs to
> be preserved forever, but also not in the camp that would squash a
> sequence of substantive commits all the way down to just one. My position
> would be more that a sequence of commits should be thoughtfully edited,
> just like any other writing, into a form that eliminates distractions
> and tells an easy-to-follow story of how the code got to be the way it is.
>
> The way I edited the commits in PR 7 down to three is one example. The
> story is kind of "you would naturally want to delegate to Maven's loader
> first" ... "but making it the parent doesn't work because X" ... "and by
> the way, it still works using the platform loader and not the application
> loader as earlier believed."
>
> Besides making a clear presentation for someone later to understand
> the code, that can have another benefit. Suppose in the future there
> is a reason that swapping the real-parent/extra-parent in c857ce0
> turns out not to be a great idea after all, and a more complicated
> solution is needed. At that point, somebody wouldn't have to manually
> repeat those changes backwards; they could just start another branch
> from an appropriate spot and select and revert that commit.
>
> Another example would be the seventeen commits ending at d9b07ca, edited
> down to six commits ending at 9e2e50f. Ideally, for the commits that are
> left after editing, it should be easy to write a good commit comment
> for each one that explains what was learned or changed in that step.
> Somebody later trying to understand the code needs to learn the same
> things.
>
> Some of that information can be conveyed in regular comments in the code,
> which are also necessary. I don't think code comments end the need for
> a good git history, or vice versa. They both have their part to play.
> Sometimes there will be some detail of a code comment that won't seem
> completely clear, but the git history will show the exact code changes
> that went with it, and that will clear things up.
>
> When adding Javadoc comments to methods that were first added without
> them, I do not object to using rebase -i to get the comments and the
> new code to appear in the same commit.
>
> Be careful when re-doing the merge that happened in 75bd8d5: look how
> many lines of insignificant spacing changes are shown in the diffs:
>
> - public static void qp(Connection c, String sql) throws Exception
> ++ public static void qp (Connection c, String sql) throws Exception
> + {
> + qp(q(c, sql));
> + }
> +
> + /**
> + * Invoke some {(at)code execute} method on a {(at)code Statement} and
> pass
> - * the {(at)link #q(Statement,Callable) result stream}
> ++ * the {(at)link #q(Statement, Callable) result stream}
> + * to {(at)link #qp(Stream<Object>)} for printing to standard output.
> - *<p>
> ++ * <p>
> + * This is how, for example, to prepare, then print the results
> of, a
> + * {(at)code PreparedStatement}:
> - *<pre>
> ++ * <pre>
> + * PreparedStatement ps = conn.prepareStatement("select foo(?,?)");
> + * ps.setInt(1, 42);
> + * ps.setString(2, "surprise!");
> + * qp(ps, ps::execute);
> - *</pre>
> ++ * </pre>
> + * The {(at)code Statement} will be closed.
> + */
> - public static void qp(Statement s, Callable<Boolean> work) throws
> Exception
> ++ public static void qp (Statement s, Callable<Boolean> work) throws
> Exception
>
>
> (just a very small sample!)
>
> Even in a project with specific formatting conventions (like the ones
> from PostgreSQL used here), there is still some room for variation.
> I don't necessarily think that one way of spacing the parenthesis after
> a function, or the tags in a javadoc comment, is better or worse than
> another.
>
> But it is important not to make commits that needlessly change all of
> the spacing choices made by a collaborator. The reason is it clutters up
> the git record and makes it very hard to see what the actual intended
> changes were in the commit.
>
> If you are using some IDE that makes those changes without asking,
> perhaps it has a setting to make it not do that.
>
> If you are not using something like 'git gui' when making commits, it
> would be worth taking a look at: it gives you a very usable way to see
> exactly what changes are going to be in your commit, so you can make
> sure they will be only the specific things you intended to change.
>

I took your advice and rebased the branch and tried to write some
informative commit messages. I have pushed the changes to github. While
rebasing, instead of merging your branch directly I cherry picked the
relevant commits because I had made changes to the commits before it was
forked. You can check the commits and let me know if any more changes are
needed.

----
>
> After taking a little break for editing, I think the start on the nar
> functionality could follow the basic outline in
>
> https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin#basic-operations
>
> The "platform-specific recipes" mentioned there come from a couple of
> places, generic information you can find in PostgreSQL's Makefile.shlib,
> and specific values like CFLAGS, LDFLAGS, etc., that are nothing but
> a few more calls on getPgConfigProperty().
>
> It is probably good to spend a little design time before coding, thinking
> of different ways the generic 'recipes' could be represented. (I assume
> you're not going for the "write a GNU Makefile parser" approach.)
>
> There are probably some reasonable ways you could decide to transcribe
> different platform 'recipes' from Makefile.shlib into some JavaScript
> snippets and put those in a special configuration block for the plugin.
>
> The nar plugin chooses a recipe based on 'arch-OS-linker'. You can get
> the arch and OS straight from Java system properties. I would suggest
> the plugin do that, then allow JavaScript recipes per arch and OS to
> have a method that determines the remaining piece if needed (on Windows
> that could be a bit of JavaScript to decide if this in MSVC or MinGW).
>
> ----
>
> As for
>
> https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin#argument-passing-workaround-for-windows
> I already have some of that now, because I needed it for something else.
> So you shouldn't end up needing to conjure that up right from scratch,
> though what I've got may need to be generalized further for use in this
> plugin, and I would definitely appreciate your help reviewing the
> approach for correctness; it's devilish tricky stuff.
>
> ----
>
> Also, on the wiki page, I called this "One last thing", but that doesn't
> mean it has to be last:
>
>
> https://github.com/tada/pljava/wiki/Build-process-custom-Maven-plugin#one-last-thing-a-simple-report-mojo
>
> It would be helpful to me to move that part up, maybe even do it next;
> I have a use for it, and in this case I can't use scripting in antrun
> to get around it, because antrun doesn't implement MavenReport and so
> that is only usable in build phases.
>
> Sure, I'll work on this first then. I'll add a JavaDoc Mojo and will see
the Maven JavaDoc plugin's implementation to see how it invokes the
JavaDoc. From the commit messages, I can gather that the pain point is the
lack of ability to configure it. I'll expose a method through the mojo just
like the previous one to fix that.

> Regards,
> -Chap
>

Regard,
Kartik

I have been going through the maven javadoc plugin's source. It is huge and
is over 6000 lines. Thankfully, most of it is customization things we can
avoid. I saw that the current configuration for maven javadoc has
exclude parameters in the pom.xml file. Now, maven uses a DirectoryScanner
to help out with the excludes which is available as a package in maven
shared utils. I am unable to think of a way to handle excludes with the
inbuilt Java APIs. Should I utilise DirectoryScanner or do you have any
other suggestions?

Regards,
Kartik.

>

On 07/25/20 12:54, Kartik Ohri wrote:
> I have been going through the maven javadoc plugin's source. It is huge and
> is over 6000 lines. Thankfully, most of it is customization things we can
> avoid. I saw that the current configuration for maven javadoc has
> exclude parameters in the pom.xml file. Now, maven uses a DirectoryScanner
> to help out with the excludes which is available as a package in maven
> shared utils. I am unable to think of a way to handle excludes with the
> inbuilt Java APIs. Should I utilise DirectoryScanner or do you have any
> other suggestions?

I honestly want something without any of that jazz at all.

The bare javadoc tool does the right thing if run with defaults and
pointed at the module descriptor.

All the pain of 5ee9b6b and 395ac0d involved finding any possible way
to prevent the maven-javadoc-plugin from making the javadoc tool do
the wrong thing.

Running the tool the right way shouldn't take more than 5 lines of JS.

A MavenReport plugin will also need to send some information to the
Doxia Sink to get the basic report page made with a link to the javadoc;
that part might take more than 5 lines of script but I'd be surprised
if more than 15.

Literally, if a mojo implements MavenReport (or extends AbstractMavenReport)
and makes the right objects available to the script engine, I don't think
much else would be needed.

I did some playing around with javax.script.Invocable (which the JS
engines implement). You can define an interface with some instance
methods, and ask the engine for an instance of it, and if the script
defined the right functions, you get a usable-in-Java implementation
of that interface, which works by calling the script functions.

You can even define an interface that has some default methods, and
it can be instantiated as long as the script defined the functions
that are needed. If the script also defines a function corresponding
to a default method, that gets used too. It works very much as one
would guess.

(I did a big double-take when I noticed you could have an interface
with a default method, get an instance from the script engine, then
eval another script that defines the missing function, and after that
the Java instance you already had starts using the script function
instead of the default!)

That might be useful for this purpose, as a MavenReport has a bunch
of methods (getName, getCategoryName, canGenerateReport,
isExternalReport, etc.) that could be given reasonable defaults for
most cases, but maybe a script should be able to override.
MavenReport itself might not be the exact right interface to use
(the mojo itself should implement that, or extend AbstractMavenReport),
but maybe the mojo should declare a similar interface with a subset
of those methods, and have that be what the script block should
be able to supply).

Regards,
-Chap

On 07/25/20 00:59, Kartik Ohri wrote:

>> Sure, I'll work on this first then. I'll add a JavaDoc Mojo and will see
> the Maven JavaDoc plugin's implementation to see how it invokes the
> JavaDoc. From the commit messages, I can gather that the pain point is the
> lack of ability to configure it. I'll expose a method through the mojo just
> like the previous one to fix that.

I should have replied to this message first ... to clarify, I'm not
really hoping for a JavaDoc mojo, just a scripting mojo that's usable
in the site life-cycle.

There shouldn't be anything that makes it specifically a JavaDoc mojo,
other than what is written in the script block in the pom.

Regards,
-Chap

On Sat, Jul 25, 2020 at 11:30 PM Chapman Flack <chap(at)anastigmatix(dot)net>
wrote:

> On 07/25/20 00:59, Kartik Ohri wrote:
>
> >> Sure, I'll work on this first then. I'll add a JavaDoc Mojo and will see
> > the Maven JavaDoc plugin's implementation to see how it invokes the
> > JavaDoc. From the commit messages, I can gather that the pain point is
> the
> > lack of ability to configure it. I'll expose a method through the mojo
> just
> > like the previous one to fix that.
>
> I should have replied to this message first ... to clarify, I'm not
> really hoping for a JavaDoc mojo, just a scripting mojo that's usable
> in the site life-cycle.
>
> There shouldn't be anything that makes it specifically a JavaDoc mojo,
> other than what is written in the script block in the pom.
>
> Regards,
> -Chap
>

I understand that essentially clears up things. So, we want to be able to
do the same thing as pg_config with javadoc, right ?

Also, I am trying to run the javadoc tool directly to know how it works but
am getting some errors. Can you tell me the command I should execute to
build the docs successfully? I looked up online for tutorials but most
predate Java 9 and the module system so are not of much help.

Regards,
Kartik

On 07/25/20 14:05, Kartik Ohri wrote:
> Also, I am trying to run the javadoc tool directly to know how it works but
> am getting some errors. Can you tell me the command I should execute to
> build the docs successfully? I looked up online for tutorials but most
> predate Java 9 and the module system so are not of much help.

I think you can get a basic set of javadocs for the pljava-api subproject,
for example, if you run something like

javadoc \
--module org.postgresql.pljava \
--module-source-path org.postgresql.pljava=pljava-api/src/main/java \
-d /tmp/put-them-here

You can find out what options the maven-javadoc-plugin is using by
adding <debug>true</debug> in its configuration, then running mvn site.

With <debug>, it will leave behind files javadoc.sh, options, argfile
in the apidocs directory. Then you can just see how many options the
plugin put in can be thrown away and it still works.

Of course we'll want some of the options that control what access
levels to show, page headers and footers, that kind of stuff. It's
just that all the lengths the maven plugin goes to trying to micromanage
where javadoc finds the files to document are nothing but trouble.
If you tell it what module to document and where to find it, really,
javadoc can figure out the rest.

I wouldn't mind writing the scripty bits to run javadoc, once there is
a scriptable mojo I can use to do it. Or if you'd like to, that's cool.

Regards,
-Chap

On 07/25/20 13:42, Chapman Flack wrote:
> On 07/25/20 12:54, Kartik Ohri wrote:
>> I saw that the current configuration for maven javadoc has
>> exclude parameters in the pom.xml file. Now, maven uses a DirectoryScanner
>> to help out with the excludes which is available as a package in maven
>> shared utils. I am unable to think of a way to handle excludes with the

Just as perhaps the best illustration yet of why I want to move away from
the maven-javadoc-plugin: the reason those excludes are there is explained
in the commit log for 5ee9b6b.

The whole reason those excludes are there is to stop the wretched
maven-javadoc-plugin from passing stuff to javadoc that made it do
the wrong thing.

They wouldn't be needed at all otherwise.

Regards,
-Chap

Hi! The initial version of the javadoc mojo is ready. I have pushed the
changes. For now, I have just exposed a function using which arguments
could be added to the javadoc tool. I also wrote some script for making the
tool work. The javadoc tool works for pljava-api but not for pljava. I am
trying to debug this. It also works for PL/Java Examples. I have hardcoded
some things which I'll change later like the version once the mojo works
properly. The generate method is still to be added.

I did not try to go the interface route yet because I am not sure of the
order in which maven calls these methods. Because if maven calls these
methods before the executeReport method it would be of no use. Also, if
there are a small limited set of methods, directly exposing those could
work as well.

Regards,
Kartik Ohri

On 07/27/20 06:45, Kartik Ohri wrote:
> Hi! The initial version of the javadoc mojo is ready. I have pushed the
> changes. For now, I have just exposed a function using which arguments
> could be added to the javadoc tool. I also wrote some script for making the
> tool work. The javadoc tool works for pljava-api but not for pljava. I am
> trying to debug this. It also works for PL/Java Examples. I have hardcoded

That looks like a good start. I wonder if the explicit start of a subprocess
could be replaced with javax.tools.ToolProvider.getSystemDocumentationTool()
and the corresponding API.

I think that could be simple enough to just directly use from the
JavaScript, which would mean the mojo would not have to be called
JavadocMojo or have Javadoc-specific functionality built in. It could
concentrate on being a plugin that allows scripting of new reports in
general to run in the site lifecycle.

> I did not try to go the interface route yet because I am not sure of the
> order in which maven calls these methods. Because if maven calls these
> methods before the executeReport method it would be of no use.

It might (or, then again, might not) work to guard against that concern
by factoring some initialization code into a method of its own, to be
called from wrappers around the various interface methods.

Regards,
-Chap

On Tue, Jul 28, 2020 at 7:39 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/27/20 06:45, Kartik Ohri wrote:
> > Hi! The initial version of the javadoc mojo is ready. I have pushed the
> > changes. For now, I have just exposed a function using which arguments
> > could be added to the javadoc tool. I also wrote some script for making
> the
> > tool work. The javadoc tool works for pljava-api but not for pljava. I am
> > trying to debug this. It also works for PL/Java Examples. I have
> hardcoded
>
> That looks like a good start. I wonder if the explicit start of a
> subprocess
> could be replaced with
> javax.tools.ToolProvider.getSystemDocumentationTool()
> and the corresponding API.
>
I have made the switch from ProcessBuilder to ToolProvider and pushed the
changes. The usage is fairly simple as only a list of arguments is required
to be passed to the tool.

> I think that could be simple enough to just directly use from the
> JavaScript, which would mean the mojo would not have to be called
> JavadocMojo or have Javadoc-specific functionality built in. It could
> concentrate on being a plugin that allows scripting of new reports in
> general to run in the site lifecycle.
>
For now, I have kept the procedure the same but using the ToolProvider API
is indeed very simple and requires only a couple of lines of code. So, we
can move those entirely to the script and make the mojo completely agnostic
to what it does as you suggest. I'll try this and let you know how it goes.

> > I did not try to go the interface route yet because I am not sure of the
> > order in which maven calls these methods. Because if maven calls these
> > methods before the executeReport method it would be of no use.
>
> It might (or, then again, might not) work to guard against that concern
> by factoring some initialization code into a method of its own, to be
> called from wrappers around the various interface methods.
>
> Regards,
> -Chap
>

On Tue, Jul 28, 2020 at 11:53 PM Kartik Ohri <kartikohri13(at)gmail(dot)com> wrote:

> On Tue, Jul 28, 2020 at 7:39 AM Chapman Flack <chap(at)anastigmatix(dot)net>
> wrote:
>
>> On 07/27/20 06:45, Kartik Ohri wrote:
>> > Hi! The initial version of the javadoc mojo is ready. I have pushed the
>> > changes. For now, I have just exposed a function using which arguments
>> > could be added to the javadoc tool. I also wrote some script for making
>> the
>> > tool work. The javadoc tool works for pljava-api but not for pljava. I
>> am
>> > trying to debug this. It also works for PL/Java Examples. I have
>> hardcoded
>>
>> That looks like a good start. I wonder if the explicit start of a
>> subprocess
>> could be replaced with
>> javax.tools.ToolProvider.getSystemDocumentationTool()
>> and the corresponding API.
>>
> I have made the switch from ProcessBuilder to ToolProvider and pushed the
> changes. The usage is fairly simple as only a list of arguments is required
> to be passed to the tool.
>
>> I think that could be simple enough to just directly use from the
>> JavaScript, which would mean the mojo would not have to be called
>> JavadocMojo or have Javadoc-specific functionality built in. It could
>> concentrate on being a plugin that allows scripting of new reports in
>> general to run in the site lifecycle.
>>
> For now, I have kept the procedure the same but using the ToolProvider API
> is indeed very simple and requires only a couple of lines of code. So, we
> can move those entirely to the script and make the mojo completely agnostic
> to what it does as you suggest. I'll try this and let you know how it goes.
>
I tried this and it works well (except for the pljava module which was
anyways crashing in the previous way as well). Please take a look at the
latest changes and let me what other modifications are required.

> > I did not try to go the interface route yet because I am not sure of the
>> > order in which maven calls these methods. Because if maven calls these
>> > methods before the executeReport method it would be of no use.
>>
>> It might (or, then again, might not) work to guard against that concern
>> by factoring some initialization code into a method of its own, to be
>> called from wrappers around the various interface methods.
>>
>> Regards,
>> -Chap
>>
>

On 07/28/20 15:14, Kartik Ohri wrote:

> I tried this and it works well (except for the pljava module which was
> anyways crashing in the previous way as well). Please take a look at the
> latest changes and let me what other modifications are required.

I think I got a little farther there:

https://github.com/jcflack/pljava/commit/51bbe22

The commit comment talks about what I solved and what I didn't solve.
Something is still going on that puzzles me.

Regards,
-Chap

On Wed, Jul 29, 2020 at 8:27 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/28/20 15:14, Kartik Ohri wrote:
>
> > I tried this and it works well (except for the pljava module which was
> > anyways crashing in the previous way as well). Please take a look at the
> > latest changes and let me what other modifications are required.
>
> I think I got a little farther there:
>
> https://github.com/jcflack/pljava/commit/51bbe22
>
> The commit comment talks about what I solved and what I didn't solve.
> Something is still going on that puzzles me.
>
> Regards,
> -Chap

I read the commit comment. Maven has a method for the dependency path using
Artifact. I'll expose the path and remove the hardcoding. I'll expose a
reference to this so that the sink can be obtained. However, since every
maven report requires this, we could also add a configuration parameter
which is read by the mojo and then executed on the Sink.
Regards,
Kartik

>

I have added the pljava jar api path and exposed this instance in the
script. It completes our basic requirements. I think we could provide a
default implementation for the report generation by exposing an interface
as you suggested. I am a bit unsure of how that would work. Can you share
the snippet you tried in case you still have it ?

Regards,
Kartik

On 07/30/20 10:32, Kartik Ohri wrote:
> I have added the pljava jar api path and exposed this instance in the
> script. It completes our basic requirements. I think we could provide a
> default implementation for the report generation by exposing an interface
> as you suggested. I am a bit unsure of how that would work. Can you share
> the snippet you tried in case you still have it ?

I didn't try a snippet for that, but I'll be happy to take a crack at it
later today if the instance is exposed now.

Regards,
-Chap

>
> I didn't try a snippet for that, but I'll be happy to take a crack at it
> later today if the instance is exposed now.
>
To clarify, I have exposed the instance and written the snippet using
Doxia sink to generate the html file in the pom.xml. I meant that we could
factor out the sink part in a default method in an interface and expose it
to javascript for providing an alternate implementation if wanted. It is
this part I am unsure of how to do.

On 07/30/20 11:37, Kartik Ohri wrote:
> To clarify, I have exposed the instance and written the snippet using
> Doxia sink to generate the html file in the pom.xml. I meant that we could
> factor out the sink part in a default method in an interface and expose it
> to javascript for providing an alternate implementation if wanted. It is
> this part I am unsure of how to do.

Ah, got it.

Right now, nothing is jumping out at me as an obvious way to supply a
default implementation for a generic report. (Not even thinking of the
mechanics of doing it just now, only the wider question of what such a
default method would do.) I would say that's a bit of refactoring that
isn't needed now, but some future day if there has been some experience
using the mojo to script both Javadoc and some other sort of report,
what turns out to be common between those uses might supply ideas for
a refactoring then.

I am a little disappointed that the Sink methods return void instead of
the sink, so they can't be used fluently; if I were looking for one
simple way to make the script snippet end up prettier, I might consider
writing a wrapper class that's like Sink but with methods returning this,
so the script can be written in a fluent style.

However, writing such a wrapper for an interface with nearly 200 methods
sounds like a job for an extremely boring rainy day, definitely not a
priority for right now.

If I were writing such a wrapper, I might also consider wrapping each
foo()/foo_() pair of start/end methods as a single method that takes
a lambda generating the content inside, so misnesting would not be
possible. (If I'm not mistaken, there's a bit of misnesting in your
snippets, sectionTitle_ before link_ ... a nicer API would leave no
need to think about that.)

But probably there are better areas to polish right now. A generic
scripting mojo can't really know "you're going to want the jar path
for a dependency named pljava-api, I'll put that in a variable for you";
it just needs to expose some way to get the project object so the
script can do such lookups.

I am probably also passing the Javadoc -d option the wrong way
(hardcoding ${basedir}/target/site/apidocs). Probably should just
tack on "apidocs" to report.getReportOutputDirectory() (using something
like toPath().resolve("apidocs") so the tacking-on is platform-aware.

Likewise, what I was passing to -encoding and -docencoding
should probably come from report.getInputEncoding() and
report.getOutputEncoding(), respectively. All that should
be straightforward with the report instance exposed.

Hmm, but those encoding methods are protected; the script might not
have access. Accessible versions might have to be supplied.

Regards,
-Chap

>
> Likewise, what I was passing to -encoding and -docencoding
>
should probably come from report.getInputEncoding() and
> report.getOutputEncoding(), respectively. All that should
> be straightforward with the report instance exposed.
>
> Hmm, but those encoding methods are protected; the script might not
> have access. Accessible versions might have to be supplied.
>

I saw you had used project.build.sourceEncoding which is the default value
of inputEncoding [1]. Only if this value is null, the getInputEncoding
method returns ReaderFactory.FILE_ENCODING [2]. But since javadoc is not
complaining in the current implementation, the null case does not
apply. So, this will be probably redundant. A similar argument for output
encoding, it uses a different value which can also be interpolated directly
in the pom. Should I still expose public methods calling these ?

[1] :
https://maven.apache.org/shared/maven-reporting-impl/xref/org/apache/maven/reporting/AbstractMavenReport.html#L86
[2] :
https://maven.apache.org/shared/maven-reporting-impl/xref/org/apache/maven/reporting/AbstractMavenReport.html#L303

Regards,
Kartik

On 07/30/20 16:13, Kartik Ohri wrote:
> I saw you had used project.build.sourceEncoding which is the default value
> of inputEncoding [1]. Only if this value is null, the getInputEncoding
> method returns ReaderFactory.FILE_ENCODING [2]. But since javadoc is not
> complaining in the current implementation, the null case does not
> apply. So, this will be probably redundant. A similar argument for output
> encoding, it uses a different value which can also be interpolated directly
> in the pom. Should I still expose public methods calling these ?

As you say, the importance is kind of minor ... but if the effort required
is also minor, it is probably worthwhile, just because it allows a script
snippet to be written in a way that will look 'right' on its face to any
casual reader, who need not know as much of the implementation detail
as you do.

Might be simplest just to override them, loosening the access modifier
to public and calling the super method. As 'this' is already available
to the script, maybe nothing more is needed.

Regards,
-Chap

On Fri, Jul 31, 2020 at 2:04 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 07/30/20 16:13, Kartik Ohri wrote:
> > I saw you had used project.build.sourceEncoding which is the default
> value
> > of inputEncoding [1]. Only if this value is null, the getInputEncoding
> > method returns ReaderFactory.FILE_ENCODING [2]. But since javadoc is not
> > complaining in the current implementation, the null case does not
> > apply. So, this will be probably redundant. A similar argument for output
> > encoding, it uses a different value which can also be interpolated
> directly
> > in the pom. Should I still expose public methods calling these ?
>
> As you say, the importance is kind of minor ... but if the effort required
> is also minor, it is probably worthwhile, just because it allows a script
> snippet to be written in a way that will look 'right' on its face to any
> casual reader, who need not know as much of the implementation detail
> as you do.

Yes, that indeed makes sense :). I have made and pushed the
required changes.

>
>
Might be simplest just to override them, loosening the access modifier
> to public and calling the super method. As 'this' is already available
> to the script, maybe nothing more is needed.
>
> Regards,
> -Chap
>
Regards,
Kartik

Hi! Were you able to take a look at the current state of the plugin ?
Regards,
Kartik

Hi! I wanted to ask if you have been able to take a look at the latest
changes ?
Regards,
Kartik

(Sorry, for the multiple emails. It was some issue with my internet
connection.)

On Sun, Aug 2, 2020, 11:20 PM Kartik Ohri <kartikohri13(at)gmail(dot)com> wrote:

> Hi! I wanted to ask if you have been able to take a look at the latest
> changes ?
> Regards,
> Kartik
>

On 08/02/20 13:58, Kartik Ohri wrote:
> (Sorry, for the multiple emails. It was some issue with my internet
> connection.)
>
> On Sun, Aug 2, 2020, 11:20 PM Kartik Ohri <kartikohri13(at)gmail(dot)com> wrote:
>
>> Hi! I wanted to ask if you have been able to take a look at the latest
>> changes ?

Thanks for the reminder, I had not. I have just looked over the commit diff
and from there they look good. I had never used the Nashorn getter-method /
property equivalence before. That's kind of tidy.

I just noticed, comparing to how the current documentation site looks,
the maven-javadoc-plugin actually doesn't produce a vestigial Doxia
report containing a link to the javadocs. It somehow arranges for the
link to the JavaDocs report to go directly to apidocs/index.html.

I'm sorry I hadn't spotted that before. Have you seen how they did that?
It would be a nicer experience, and maybe would make the scripting shorter
also. (And eliminate the temptation to write a fluent wrapper around Sink.)

For git commit messages, there's a kind of widely followed but unofficial
convention to have a top-line summary of no more than 50 characters,
then a blank line and any further description as paragraphs below that.

I guess it's a little bit official; 'git help commit' mentions it:

Though not required, it's a good idea to begin the commit message with
a single short (less than 50 character) line summarizing the change,
followed by a blank line and then a more thorough description. The text
up to the first blank line in a commit message is treated as the commit
title, and that title is used throughout Git. For example, git-format-
patch(1) turns a commit into email, and it uses the title on the
Subject line and the rest of the commit in the body.

If you use commandline git commit and it starts up vim, you'll notice
the first line as you are typing will change color at character 50, just
as a kind of gentle reminder.

Regards,
-Chap

>
> I just noticed, comparing to how the current documentation site looks,
> the maven-javadoc-plugin actually doesn't produce a vestigial Doxia
> report containing a link to the javadocs. It somehow arranges for the
> link to the JavaDocs report to go directly to apidocs/index.html.
>
> I'm sorry I hadn't spotted that before. Have you seen how they did that?
> It would be a nicer experience, and maybe would make the scripting shorter
> also. (And eliminate the temptation to write a fluent wrapper around Sink.)
>
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.

For git commit messages, there's a kind of widely followed but unofficial
> convention to have a top-line summary of no more than 50 characters,
> then a blank line and any further description as paragraphs below that.
>
> I guess it's a little bit official; 'git help commit' mentions it:
>
> Though not required, it's a good idea to begin the commit message with
> a single short (less than 50 character) line summarizing the change,
> followed by a blank line and then a more thorough description. The text
> up to the first blank line in a commit message is treated as the commit
> title, and that title is used throughout Git. For example, git-format-
> patch(1) turns a commit into email, and it uses the title on the
> Subject line and the rest of the commit in the body.
>
> If you use commandline git commit and it starts up vim, you'll notice
> the first line as you are typing will change color at character 50, just
> as a kind of gentle reminder.
>

I have now configured my IDE so that such issues do not occur in future.

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

Regards,
Kartik

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

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

On 08/04/20 08:07, Kartik Ohri wrote:

> 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've created a branch
https://github.com/jcflack/pljava/commits/invocable
where you can see the way I had in mind for it to work.

It seems to, although javadoc still seems to be ignoring my
--show-packages all, as I had noticed before; I still don't know why,
but I don't think it's the mojo's fault.

Regards,
-Chap

On Wed, Aug 5, 2020 at 9:09 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 08/04/20 08:07, Kartik Ohri wrote:
>
> > 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've created a branch
> https://github.com/jcflack/pljava/commits/invocable
> where you can see the way I had in mind for it to work.
>
> I took a look and this way seems to be really better than how I had done
it. Now that I look at it I feel if I had tried harder, I could have made
it work myself. I'll try harder next time. Thanks for helping with this.
I'll merge this into my branch and begin work on the nar maven part.

> It seems to, although javadoc still seems to be ignoring my
> --show-packages all, as I had noticed before; I still don't know why,
> but I don't think it's the mojo's fault.
>
> Regards,
> -Chap
>
Regards,
Kartik

On 08/08/20 10:00, Kartik Ohri wrote:
> On Wed, Aug 5, 2020 at 9:09 AM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:
>
>> On 08/04/20 08:07, Kartik Ohri wrote:
>>
>>> 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've created a branch
>> https://github.com/jcflack/pljava/commits/invocable
>> where you can see the way I had in mind for it to work.
>>
>> I took a look and this way seems to be really better than how I had done
> it. Now that I look at it I feel if I had tried harder, I could have made
> it work myself. I'll try harder next time. Thanks for helping with this.
> I'll merge this into my branch and begin work on the nar maven part.

Works out better than either person working alone. It's nice to have
a collaborator. :)

After you merge, would you mind creating a new branch name from that
point, before continuing with the nar-maven part? I don't mind if it's
a new name for the nar-maven part, or you keep 'plugin' for that part
and use a new name to point to this reporting part.

I think the reporting part will be just about ready for a pull request
soon, so branching will allow us to do that while you continue with
the nar stuff.

I am still working on one more commit or so to get the relative links
between javadoc for different modules working right. (Javadoc seems to
have a misfeature that interferes; in the published docs on the site,
I only got it working by using absolute URLs instead, to the actual
tada.github.io site, but of course that makes them broken when testing
in a local staging directory, and makes it harder to one day support
publishing docs for > 1 PL/Java version. So I really want to solve
the relative-link problem.)

Regards,
-Chap

On Sat, Aug 8, 2020 at 7:43 PM Chapman Flack <chap(at)anastigmatix(dot)net> wrote:

> On 08/08/20 10:00, Kartik Ohri wrote:
> > On Wed, Aug 5, 2020 at 9:09 AM Chapman Flack <chap(at)anastigmatix(dot)net>
> wrote:
> >
> >> On 08/04/20 08:07, Kartik Ohri wrote:
> >>
> >>> 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've created a branch
> >> https://github.com/jcflack/pljava/commits/invocable
> >> where you can see the way I had in mind for it to work.
> >>
> >> I took a look and this way seems to be really better than how I had done
> > it. Now that I look at it I feel if I had tried harder, I could have made
> > it work myself. I'll try harder next time. Thanks for helping with this.
> > I'll merge this into my branch and begin work on the nar maven part.
>
> Works out better than either person working alone. It's nice to have
> a collaborator. :)
>
> After you merge, would you mind creating a new branch name from that
> point, before continuing with the nar-maven part? I don't mind if it's
> a new name for the nar-maven part, or you keep 'plugin' for that part
> and use a new name to point to this reporting part.
>
>
I'll create a new branch for the nar stuff and begin working on that.