Subject: Re: PL/Java new build plugin

From: Kartik Ohri <[email-masked]>
Date: Sun, 2 Aug 2020 23:20:29 +0530

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

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 2 Aug 2020 23:28:05 +0530

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

---

From: Chapman Flack <[email-masked]>
Date: Mon, 3 Aug 2020 00:04:30 -0400

>> Hi! I wanted to ask if you have been able to take a look at the latest

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

---

From: Kartik Ohri <[email-masked]>
Date: Tue, 4 Aug 2020 00:55:51 +0530

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

> 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

---

From: Chapman Flack <[email-masked]>
Date: Mon, 3 Aug 2020 19:03:11 -0400

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

---

From: Kartik Ohri <[email-masked]>
Date: Tue, 4 Aug 2020 17:37:22 +0530

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

> 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'll try to figure this out.

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

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Tue, 4 Aug 2020 23:39:16 -0400

> a working example to test it. Please review it and let me know if changes

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

---

From: Kartik Ohri <[email-masked]>
Date: Sat, 8 Aug 2020 19:30:31 +0530

On Wed, Aug 5, 2020 at 9:09 AM Chapman Flack <[email-masked]> wrote:

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

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sat, 8 Aug 2020 10:13:39 -0400

> 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

---

From: Kartik Ohri <[email-masked]>
Date: Sat, 8 Aug 2020 19:59:49 +0530

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

---

From: Chapman Flack <[email-masked]>
Date: Sat, 8 Aug 2020 11:29:56 -0400

Hi Kartik,

There are a couple more methods it might be useful for getScriptEngine
to load into a script engine by default.

If you look in the javadoc script for pljava-examples, there's a test
for whether the saxon-examples profile is active.

It's an easy test, but I can foresee it showing up in compiling scripts
as well, so a simple boolean isProfileActive(String id) method is probably
worth supplying.

If supplying one from PGXSUtils, I would probably also have the method
check that the argument profile name is even a known profile in the POM,
and warn if it isn't. That would catch cases where somebody edits a POM
and deletes a profile, not realizing a script was checking for it.
I think all the known profiles are in project.getModel().getProfiles().

You'll also see the -examples script generates the -classpath argument
by stringing together project.compileClasspathElements. That works for
-examples because the -examples code isn't modular.

I did not change the scripts in pljava/ and pljava-api/ because those
are modular code, and need to generate a --module-path instead. In
general, Maven's compileClasspathElements might include both: elements
that belong on the module path, elements that belong on the class path
because they aren't modules.

So that suggests a useful method like

Map<String,String> buildPaths(List<String> elements)

where you could pass it a list of elements (from compileClasspathElements
or runtimeClasspathElements, etc.) and get back a map m such that
m['classpath'] has all the elements that aren't modules, and
m['modulepath'] has the elements that are (both with pathSeparator between
elements).

An element is a module if it is a directory and has a module-info.class,
or it is a jar and either contains a module-info.class or has an
Automatic-Module-Name entry in its manifest.

Such a method would also be a good place for checking that no element
contains pathSeparator as part of its name, as that would lead to a
malformed path,

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sat, 8 Aug 2020 22:24:27 +0530

> contains pathSeparator as part of its name, as that would lead to a

Yeah, it is a good idea to have these methods. I'll work on this soon.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sat, 8 Aug 2020 14:05:47 -0400

> and use a new name to point to this reporting part.

Funny I only noticed this now, but all the .java files in pgxs/ are
missing the copyright-banner comment at the top. PropertiesMojo.java
had it, but it disappeared in c5ae7af. The newer ones are all naked.

Could you do a git rebase -i 03a4508 after identifying the commits where
new Java files appeared, and (e)dit those commits so the files appeared
with the banner comments? I'd say it's ok to (s)quash the 2nd, 3rd, and
4th commits into the first one, rather than bothering to add comments
to those files that were only around that long.

*Then* we'll have a branch that's nearly ready to PR, and one that
can continue with the nar stuff.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 9 Aug 2020 01:43:23 +0530

I have added the copyrights, squashed the commits and pushed the changes,
Regards,
Kartik

---

From: Kartik Ohri <[email-masked]>
Date: Mon, 10 Aug 2020 02:13:51 +0530

Hi!

I went through nar's source and seems to be just a lot of abstraction and
generalization to support different use cases. I extracted the following
commands from Nar Maven's build process
https://gist.github.com/amCap1712/f6c643a25c9d8d1dcaa429e62a81c27a. It
basically boils down to invoking the compiler with the given flags,
includes and files. And then link the created object files to create the
shared library. The jar part is created by the maven-jar-plugin which I am
not yet sure about how to use.

For the initial iteration, I am planning to expose a CC object with
functions to add the includes and the flags manually. I think this could
benefit from the profile detecting function so I'll write it first then. I
assume that all files in the pljava-so are going to be compiled. Then, call
the linker on the generated files. Once the .so file is available, we can
generalize our implementation from there as needed.

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sun, 9 Aug 2020 22:42:45 -0400

> generalize our implementation from there as needed.

Those all seem like good initial things to be doing. Don't lose sight of
the goal being to make something that works more like PGXS and less like
the nar plugin, so it would be worthwhile to be looking there too,
in particular at the compiling and linking recipes in

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/Makefile.shlib;h=003aefb;hb=c217b36#l78

which, of course, depend on the CC, CFLAGS, CFLAGS_SL, and LDFLAGS values
that can also be obtained by getPgConfigProperty.

The properties we've retrieved from pg_config so far have just been a
single line that represents a single value. There should probably be another
function for retrieving a property like CFLAGS that represents a list
of values, and you may have to read some code and/or experiment to be
sure how to parse the unusual cases. For example, do a test build of
postgres with --prefix set to a directory with spaces in its name, then
look at what comes out of pg_config.

(The --configure output from pg_config is a special case; it follows
different lexical rules and would need a different function to parse it
if we needed it, but happily, I don't think we do.)

I would like to keep as much of the action in JavaScript as possible,
so it will be readily visible to anybody who wants to know what exactly
it's doing. I am sort of picturing a big JavaScript object for the recipes
that might look something like this:

{
  "Linux": {
    probe: function() { }
    compile: function(...) { ... }
    link: function(...) { ... }
  },

  "Windows": {
    probe: function() { am I MSVC or MinGW? }
    compile: ...
    link: ...
  },

  "Mac OS X": {
    ...
  }
}

where probe could have an inherited no-op version and only be supplied
where needed (as in Windows).

The recipe can be chosen according to os.name, but there probably needs
to be a mapping function; I recently ran into that myself. Java seems to
use "Linux" for Linux, and "Mac OS X" for that, but on Windows the value
will be "Windows something something". So maybe this configuration object
needs a top level

  dispatch: function(osname) { ... pick the right recipe ... }

Maybe the dispatcher pretty much does everything: pick the right recipe,
make a copy of it (so probe() can set instance variables in the copy),
call probe, and compile, and link.

What would need to be written in Java could be pretty minimal really,
just whatever turns out to make writing JavaScript recipes easy. It
should probably have already collected the CC/LD/CFLAGS/etc. values,
and pass them to the JavaScript as a nice map.

ProcessBuilder is already close to a nice-enough interface for constructing
a command to run, but there should probably be a convenience method for
actually running it; that method could take care of the last steps,
setting the I/O redirection, handling the output, checking the exit
status, and so on.

And of course for Windows there needs to be a function to transform a
ProcessBuilder before executing it, like the one you're reviewing
in Node.java.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Sun, 9 Aug 2020 23:51:15 -0400

> and pass them to the JavaScript as a nice map.

On second thought, I don't even think that's worth doing in Java.
We already have script calling getPgConfigProperty. If the script
block started with some lines like:

var CFLAGS = getPgConfigList("--cflags");
var LDFLAGS = ...
...

it would be right there, clear, easy to read, hard to beat any other way.

If there were more than one native submodule in PL/Java needing to be
compiled, it might be worth factoring that out to avoid duplication.
But there is only the one, so I think the advantage falls on the side
of having the logic visible and accessible.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Tue, 11 Aug 2020 02:25:34 +0530

Hi!
I have pushed the changes adding these two methods. Please take a look and
let me know if any changes are needed.
Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Mon, 10 Aug 2020 21:10:04 -0400

> let me know if any changes are needed.

Thanks! I added some comments on the github commitdiff.

I also think we should not forget to eventually turn various
catch ( ... e ) { e.printStackTrace(); } places into logged Maven errors.
It would not be nice to have failures get only printed and ignored.
I think the Maven log methods will accept a throwable, and will print
the stacktrace if mvn was run with -e.

Another option would be to wrap it in some unchecked exception, and
catch and unwrap it at some caller that is declared to throw
MojoExecutionException or MavenReportException, then wrap it in one
of those.

One thing I should have warned you about when you were git rebasing: the
commit that's now f96fc90 contains, in its comment, a reference to the
commit that used to be 1d9752b (but is now 037b518, because rebasing
changes commit ids). It still looks like a working link because the old
commit exists in your repository, but it might not get copied into clones.

So if you could do a git rebase -i edb4379 and then (r)eword the comment
for (Try to) link -api from -examples..., changing the reference 1d9752b
to be 037b518, that would be fixed. I don't see any other commit id
references in the subsequent commits.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Tue, 11 Aug 2020 23:08:46 +0530

Hi! I have addressed your comments and these suggestions in the latest
changes pushed to github. Thanks.
Regards,
Kartik

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 12 Aug 2020 00:19:23 +0530

> setting the I/O redirection, handling the output, checking the exit

I have currently added this
https://github.com/amCap1712/pljava/commit/174dd71365986094a500d6dda4c832de78f90783.
Is this fine? I had to change the way error handling worked to avoid
creating a new functional interface.

---

From: Chapman Flack <[email-masked]>
Date: Tue, 11 Aug 2020 18:15:19 -0400

> creating a new functional interface.

That looks like some of the right ideas shaping up.

You definitely do NOT want "arguments = String.join(" ", argumentsList)".
That's exactly how to create a mess that cannot be correctly reparsed
into the original arguments, depending on what characters they contain.

The proper model to think in always a list of individual arguments. That
is the model the Java ProcessBuilder uses, and it is also the true native
model in POSIX (and so in Linux and Mac OS).

The native model in Windows is a single joined command line that the
receiving program then has to reparse to get the original individual
arguments correctly back, and the Java runtime itself will create the
joined string for you (you still pass individual arguments to
ProcessBuilder), but it needs help to do that correctly; I hope you are
reviewing the code in Node.java that I mentioned earlier. The point is to
do this transparently and correctly, so you start with a list of arguments,
and the invoked program ultimately has a list of arguments, and that's
the same as the starting list.

Regarding the subprocess stdout and stderr, as a way of doing things in
stages, I would suggest starting as simply as possible: just set them
both to INHERIT, so they go straight to our own stdout and stderr, and
we don't have to do anything with them, and can concentrate on getting
the build working.

As a later step, we could revisit that to capture output and funnel it
into Maven's logging, probably applying some filtering first, to decide
what should be logged as info, warn, or error (and ideally do a better
job of that than nar-maven-plugin does).

Capturing all of the output in a readAllBytes() is not the usual way
to do that, because it's preferable to see the messages as they happen
rather than all at the end (and if a process happens to generate a whole
lot of output, readAllBytes uses a lot of memory).

In that way, that makes this task different from getPgConfigProperty,
where the point really was to just read all of the output from pg_config
and return it. I see now that you have refactored that method in terms
of this one, but that may just complicate things. It is ok to have a
couple of different methods using ProcessBuilder but in different ways,
as "run pg_config and return its output" and "run this arbitrary compiler
or linker with its output going to the log" are sufficiently different
tasks.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 12 Aug 2020 04:19:53 +0530

> the same as the starting list.
Yes, I agree with you. I used String.join because the ProcessBuilder
documentation said that "here are operating systems where programs are
expected to tokenize command line strings themselves - on such a system a
Java implementation might require commands to contain exactly two elements".
I was not sure if this was relevant but tried to incorporate it.

> lot of output, readAllBytes uses a lot of memory).
Right, this is certainly desirable. I'll look into this and fix this.

> Yes, makes sense. I'll revert this change.

Regards,

Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Tue, 11 Aug 2020 19:03:02 -0400

> I was not sure if this was relevant but tried to incorporate it.

Hmm, I suppose there could conceivably be some OS, and some implementation
of Java for that OS, where that limitation would exist. But the only OS
in common use that I know of that treats the command line that way is
Windows, and at least for the Oracle and OpenJDK implementation (again,
what is in common use), it does not require you to give ProcessBuilder
only two elements.

It will take the list of n arguments you give it, and try to construct
a correct Windows joined command line from them, only it gets the rules
somewhat wrong and so its transformation isn't correct, hence the need
for us to preprocess. The OpenJDK code where it does that is linked to
from several comments in the preprocessing transformation code that I hope
you are looking at.

If in the future we ever needed to support some other OS "A" and the
Java implementation for "A" required ProcessBuilder to take only two
elements, we could again handle that as a preprocessing transformation
of a ProcessBuilder with n args, transforming them to two.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Fri, 14 Aug 2020 02:54:24 +0530

Hi!
I have made the changes to the ProcessBuilder and am now trying to use it
build pljava-so using it. I wanted to ask should the working directory for
the process builder be hard coded as the project's target directory or left
for the user to specify as an argument.
Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Thu, 13 Aug 2020 17:32:19 -0400

> for the user to specify as an argument.

Hmm, that's a good question.

What if ... instead of script calling new ProcessBuilder(...) to obtain
an instance, we provide a method (with the same signature), that calls
the constructor and returns a ProcessBuilder with some common defaults
already configured?

Then the script can just change any bits it wants different from the
default.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Fri, 14 Aug 2020 08:14:55 +0530

> Then the script can just change any bits it wants different from the

Sounds good to me. Will do it this way.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Fri, 14 Aug 2020 08:28:48 -0400

Hi Kartik,

I've made a pull request onto your 'plugin' branch. The first three
commits are some small fixes; you'll probably want to review those to
see what the issues were. The rest deals with getting the javadoc links
from one submodule to another to be generated correctly and in relative
form, so the output tree can be relocated and still work. That has been
a sticking point for years the old way, so I am glad to be able to fix it
now.

I think that if you merge that PR, that branch will be ready for a PR
onto master in the main repo. As a matter of form, it might be best to
create a new branch name, at the same point, according to the conventions
in the contribution guide [1], something like feature/master/scriptreports,
and use that branch in the pull request.

Regards,
-Chap

[1] https://github.com/tada/pljava/wiki/Contribution-guide

---

From: Kartik Ohri <[email-masked]>
Date: Fri, 14 Aug 2020 18:09:10 +0530

>
> I'll do this immediately but I wanted to remind you that the current
master includes the CI changes as well. Should we merge that first or
include it inside this PR?

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Fri, 14 Aug 2020 08:59:05 -0400

> include it inside this PR?

Hmm, thanks for the reminder ... let me look at the graph again.
I may ask you for one more rebase ....

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Fri, 14 Aug 2020 09:49:16 -0400

> I may ask you for one more rebase ....

I think what I'd like to do is ask you to rebase plugin one more (sigh)
time,  git  rebase  --onto  857abf4  52ee373

I think it should go smoothly, again "(Try to) link -api from -examples"
is the only commit in that range that should need its message edited.

I'll then rebase intermodule-javadoc over that and redo the pull request.

*Then* you can merge that and do a PR onto tada/pljava:master.

With respect to the CI changes, I would like to merge
chore/master/denashorn first, and then develop the usenode-travis and
usenode-win-2 branches into a pull request for CI. But I'd prefer to
merge denashorn after you have had a chance to review the ProcessBuilder
transformation methods in it.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Mon, 17 Aug 2020 08:53:26 -0400

Hi Kartik,

I've opened a new pull request onto your plugin branch. After merging
that, I think it will be ready to PR onto tada/pljava:master (using
a branch name that follows the contribution guidelines).

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Mon, 17 Aug 2020 21:14:48 +0530

Hi!
I have merged your PR into the plugin branch and opened a new PR on
tada/pljava:master.
Regards,
Kartik

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 19 Aug 2020 17:34:23 +0530

Hi!
I saw the PR has been merged. I have accordingly updated my working
branches. I am working on getting the PGXS to build pljava-so on linux
first and will then port to use the dispatcher javascript object as you
recommended. The changes can be seen at
https://github.com/amCap1712/pljava/commit/c970e5dfd0df7d36bf5840ce01c1c065b02917ed
.

The only thing left here is building the jar. I am unable to figure out how
that is being done. I see that maven-jar-plugin is being invoked from the
debug logs (probably by nar maven plugin) but the source is not very
helpful on seeing how to do that and what files to put in the jar. I looked
in the jar to see the contents and was even more surprised to see that it
only contains pom.xml, pom.properties, nar.properties and the manifest
only. There are no real files there. However, in the same directory as the
jar there are two nar files. One with the pljava-so shared lib and other
with the header files.

Can you explain how to approach the jar building steps and how do we want
to implement it (jar + nars) or a single jar?

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 10:10:49 -0400

> to implement it (jar + nars) or a single jar?

That's easy! :) The nar-maven-plugin builds a jar because that's what
it does. We don't have any use for the jar it builds.

The only thing produced in that build that we use is the .so (or .dll
or .bundle, depending on your OS). pljava-packaging looks for that file
in pljava-so/target and puts it in the final package jar.

How's that for saving you some work?

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 19 Aug 2020 19:45:35 +0530

> How's that for saving you some work?

That's really nice. I think I probably have all the parts required then for
a linux build. I'll completely remove nar maven plugin from the project and
test the build. If it works, I'll proceed on removing the hardcoded values
and then to other OS support.

Thanks.

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 10:27:10 -0400

> it does. We don't have any use for the jar it builds.

I remembered it might be a little more complicated. The POM for
pljava-so declares it to have <packaging>nar</packaging>. Maven seems
to understand a certain arbitrary set of 'packaging' types, and 'nar'
is one of them, and perhaps it expects a jar, or a jar and a nar, to
be produced.

That would be for the usual Maven picture where that jar or nar would
be deployed to a central repository and declared as a dependency by
other projects that would be downloading and using it.

That's a picture that PL/Java just doesn't fit; you don't have any
use for a pljava.so file unless you are going to put it in a PostgreSQL
lib/ directory because you are installing PL/Java.

I would change the pljava-so POM to say <packaging>nevermind</packaging>
only I don't think that's one of the recognized types. It should possibly
be changed to something other than nar though.

I have never looked into how extensible the set of 'packaging' types
is, or what would be involved in defining a new one that essentially
means 'nevermind'.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 19 Aug 2020 20:03:39 +0530

I think this might be relevant https://stackoverflow.com/a/2538515/5557184.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 10:36:40 -0400

> is, or what would be involved in defining a new one that essentially

The default ones are listed here:

https://maven.apache.org/ref/3.6.3/maven-core/default-bindings.html

and I notice they do not include 'nar'. So quite possibly the
nar-maven-plugin does add 'nar' to the list somehow, which would
suggest there is a way to do that.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 10:43:01 -0400

> suggest there is a way to do that.

https://github.com/maven-nar/nar-maven-plugin/blob/master/src/main/resources/META-INF/plexus/components.xml

-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 10:54:38 -0400

On 08/19/20 10:43, Chapman Flack wrote:
> ...META-INF/plexus/components.xml

I think my inclination would be to supply such a file with the role-hint
of 'pgxs', and mapping every phase to empty. Then what the plugin does will
be completely determined by what script <execution>s you configure, and
what phases you put them in.

Then the packaging type of pljava-so could be pgxs, and you would simply
specify the <phase> for each <execution> in the pom (as is already done
for the set-properties execution). So, what the plugin does will be
completely determined by what script <execution>s you configure, and
what phases you put them in.

I think I would also change ScriptingMojo to declare COMPILE as its
default phase. The set-properties execution would still specify
initialize as its phase, as it does now, but I think the usual
behavior is if a phase isn't specified, it will be compile.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 11:22:37 -0400

changing the subject line so we don't keep reusing "the ReportScriptingMojo"
when that's not what we're talking about.

-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 19 Aug 2020 18:15:41 -0400

Hi Kartik,

Taking an early look:

- I think the runCommand method has kind of a misleading name for what it
  does, as it only gives you a ProcessBuilder for you to set up for
  running your command. Maybe it would be better to just call that one
  ... processBuilder?

- All the same, I think a runCommand method would also be good to have;
  that would be what you call after getting your ProcessBuilder from
  processBuilder and configuring it for the command you want.

  That method would take care of ProcessBuilder.start() and waiting for
  completion and checking the exit status. I think there would be few
  times the script code would want to do those things separately (and if
  it ever did, it could anyway; no one is forcing it to use runCommand).

  Later on, it will be even better to have runCommand, as we will
  eventually want not to set the process stdout and stderr to INHERIT,
  but to have them as pipes and read them. At that point, runCommand should
  have an additional parameter, some JavaScript lambda that will be
  called back with messages read from the process, and will classify the
  messages as error/warning/info and pass them to the right Maven log
  method. That should be done in script because the patterns used to
  classify the messages will almost certainly need to be tailored
  for each compiler/linker.

  That will be a little tricky, because runCommand will need to use
  more than one thread, to be able to read both streams from the process
  and wait for the process to exit. But the script engine does not
  want to be shared between threads. So runCommand might create a
  concurrent queue, start threads that will read the process output and
  place messages on the queue, while runCommand just reads from the queue
  and passes messages to the script callback, until the process is done.
  But that can be later; just letting stdout and stderr be INHERIT is
  fine for now.

- Once there is processBuilder and runCommand, processBuilder can set a
  default for the working directory before it returns the ProcessBuilder,
  but I would move the existence test and directory creation into
  runCommand; no need to do the work up front when the caller might
  change the directory setting.

- I was thinking of simplifying the parameters to processBuilder, to not
  need the caller to separately pass the command string and a list of the
  rest of the arguments. An idea that just hit me was why not just
  processBuilder(Consumer<List<String>>)? A script could use it like:

  var pb = processBuilder(function(l) {
    l.add(command);
    l.addAll(stuff);
    l.add(thing);
    ...
  });

  so the script would avoid the messiness of allocating its own ArrayList
  and so on. On the Java side that's just

  pb = new ProcessBuilder();
  callback.accept(pb.command());
  ...
  return pb;

Once there is a platform/OS 'dispatcher object' structure, I think the
common methods compile(...), link(...), and so on, need their parameter
lists carefully thought out. One parameter to compile(), for example,
should just be a list of includes, without the -I; the method needs to
know the right syntax for its compiler. Another parameter should be a
simple map <name,value> of defines; again the method needs to know if
-Dname=value is right for its compiler. Similar considerations for link().

It might be worthwhile to define an abstract class in Java that has
the necessary methods; it could also supply some other useful methods
or default implementations. I think the javascript could then do something
like:

var platforms = {
  'Linux': { compile: function(...) { ... }, link: function... },
  'Windows' : { ... },
  ...
}

var impl = new AbstractPGXS(platforms['Linux']);

which would enforce that the needed methods were all present, and the
ones supplied in Java would be naturally available too. I have not
tried this out to make sure it does what I expect in both nashorn
and graal.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Thu, 20 Aug 2020 22:30:04 +0530

> behavior is if a phase isn't specified, it will be compile.

I have made these changes
https://github.com/amCap1712/pljava/commit/14045b10ca1428fc2914440a2220871ac7c9921b
but the build process fails with this error
https://gist.github.com/amCap1712/ad9fe94fd9fb6f4ff4f26913a464c226. I
checked the jar files manually to make sure the file is present in it. The
pgxs jar is also listed in the lists of url classpaths.

---

From: Chapman Flack <[email-masked]>
Date: Thu, 20 Aug 2020 23:28:01 -0400

> pgxs jar is also listed in the lists of url classpaths.

Hmm ... for me it does not even get that far:

[ERROR] Unknown packaging: pgxs @
org.postgresql:pljava-so:[unknown-version],
/var/tmp/nohome/pljava/pljava-so/pom.xml, line 14, column 13

It seems to be an issue similar to what we had with the profiles before,
referring to a plugin that had not yet been built. Perhaps in your case
it got further because the plugin had been built in an earlier step?

This might make the custom-packaging idea unworkable without a multi-step
build, which I would really rather avoid. Maybe the packaging type
for pljava-so should just be changed from nar to whichever of the
predefined packaging types will impose the fewest extraneous expectations.
Maybe that's pom. It only binds two phases and doesn't assume there is
anything to produce besides the pom file, which is already there.
We'll produce the .so or .dll or .bundle also, but Maven doesn't need
to care.

The custom type seemed more satisfying to me, but not if it ties things
in knots.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sat, 22 Aug 2020 15:45:01 +0530

>   callback.accept(pb.command());

Hi!
I am working adding these changes but I am particularly stuck at this one.
It seems that Nashorn and Graal do not support SAM inside SAM or I am doing
this the wrong way. Please see https://www.ideone.com/cQVGuB and
https://www.ideone.com/1x7Ah7. The former one fails at runtime but the
latter completes execution. (Here I have done a Consumer<Function, ?>
whereas Function<Consumer, ?> will be required for our use case, but both
fail at runtime anyways). This is the commit (
https://github.com/amCap1712/pljava/commit/136a8e879951432ec47faeb74e2a86e848481e67)
with changes that fails. I have been able to work around the issue using
the approach here
https://github.com/amCap1712/pljava/commit/d5977c5cf386758fb2d9e56524b05f9549cefe31
.

Regards,
Kartik

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 23 Aug 2020 00:42:04 +0530

> tried this out to make sure it does what I expect in both nashorn

Hi!
I really like this idea. But I am not sure how to work this out. The main
point of contention is the class type of platforms["Linux"]. I tested with
Nashorn and Graal and printed the class of the object in the constructor.
For nashorn, it is jdk.nashorn.api.scripting.ScriptObjectMirror and for
graal it is com.oracle.truffle.polyglot.PolyglotMap. So, it will require
implementation based handling depending on the Script Engine. That would
probably require having both Nashorn and Graal present as dependencies or
dynamically instantiating implementations which could get messy (or we
could enforce using graal always, but I don't know if that's a good idea).

But I might be missing something or another easy way here. Also, we could
add a javascript function to examine the object runtime and throw an error
with a detailed message if something is incorrect.

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sat, 22 Aug 2020 15:33:13 -0400

> https://github.com/amCap1712/pljava/commit/d5977c5cf386758fb2d9e56524b05f9549cefe31

Just getting a look at this now. In the (failing) 136a commit, it appears
to me that nashorn fails but graal is happy. Is that what you saw?

Graal fails at a different spot, on the Files.walk().parallel()...
invocation in the script, but that's clearly a different issue;
neither script engine wants to be shared across threads. A sequential
walk is adequate here; this is just a build process that runs once,
so shaving milliseconds isn't essential.

Ok, I removed the parallel() and now graal also complains about the SAM
cast. This puzzles me because I thought both engines were intended to
support that, so I'll keep playing with it a bit.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Sat, 22 Aug 2020 16:13:28 -0400

> support that, so I'll keep playing with it a bit.

If I make the attached changes, both nashorn and graal seem to be happy.

It appears that, when invoking a method /of a Java object/, they
recognize when its parameter type is a SAM interface, and do the magic
to present a JavaScript function as an implementation of that.

We might just be asking for too many layers of magic when we start by
binding a Java lambda into the script engine (which probably wraps one
layer of magic over it to make it look like a JS function), and then try
to pass a JS function as a parameter needing its own magic wrapping to
appear as a Java SAM interface, and the engine may not see through the
first layer of magic to realize it needs to add the second.

"Thing" as a class name is just proof-of-concept of course. :) I had
been wondering earlier if it might be useful to make PGXSUtils instantiable
and bind an instance of it into the script; its constructor could take the
MavenProject and Log and those wouldn't have to be passed around as much.
It would still be possible to bind selected methods of it directly with
names of their own for convenience, but other methods could just be
accessible as utils.method() and I think that would solve this casting
issue.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 23 Aug 2020 01:49:08 +0530

> accessible as utils.method() and I think that would solve this casting

Sounds good to me. I'll make the changes to the PGXSUtils class.
Regards,
Kartik

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 23 Aug 2020 02:03:48 +0530

>> accessible as utils.method() and I think that would solve this casting
Regarding this, which methods should be bound by their own names ? Should I
keep the current list of methods and just add an instance of the PGXSUtils
to the script or should I remove the existing bindings ?

---

From: Chapman Flack <[email-masked]>
Date: Sat, 22 Aug 2020 16:45:13 -0400

> to the script or should I remove the existing bindings ?

I have no strong opinion. I guess an advantage of keeping the existing
bindings is it reduces busy-work just making adjustments to the script
snippets because names of things changed. There could come a future
day after everything is working when a polishing pass could be done in
its own right and rethink some bindings or names looking at the whole
finished picture.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Sat, 22 Aug 2020 17:55:09 -0400

On 08/22/20 15:12, Kartik Ohri wrote:

> graal it is com.oracle.truffle.polyglot.PolyglotMap.

If I do the attached, it does what I was hoping for. The constructor prints

PGXS impl class is
jdk.nashorn.javaadapters.org_postgresql_pljava_pgxs_PGXSUtils$PGXS

in nashorn, or

PGXS impl class is org.postgresql.pljava.pgxs.PGXSUtils$PGXS$$JSJavaAdapter

in graal. (Wow, pretty sneaky, Graal fakes up a class that is even in
the superclass's package! I bet it's using MethodHandles.Lookup.defineClass,
which appeared in Java 9 so Nashorn probably doesn't.)

The new Foo(jsobject) syntax only has magic if Foo is an interface or an
abstract class. If Foo is an instantiable class, that will be parsed as
simply calling a one-argument constructor to instantiate it, and passing
the jsobject as the argument. Maybe that is what you were seeing?

You sort of have to believe with this stuff; ask too many questions and
you drive the magic away.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 23 Aug 2020 16:56:00 +0530

Hi!
I have implemented the changes according to your patches and pushed them. I
have added a dispatcher object to the script to provide the correct linking
recipe at runtime. I also noticed a couple of things that are valid for
both Nashorn and Graal. While instantiating a java class using a javascript
object, no error is thrown if a method is missing or mismatches arguments
(number or type). Only when the function is invoked, an exception is thrown
if there are any issues.
Please take a look and let me know what changes are needed.
Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sun, 23 Aug 2020 09:58:28 -0400

> (number or type). Only when the function is invoked, an exception is thrown

Well, I would like it better if they noticed it earlier, but I can live with
that.

Commit d5977c5 is the latest one I am seeing ... ?

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 23 Aug 2020 19:57:42 +0530

> Commit d5977c5 is the latest one I am seeing ... ?

Oh, no. I actually had forgotten to push the changes😅 . I have pushed the
changes now.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sun, 23 Aug 2020 13:27:59 -0400

> Oh, no. I actually had forgotten to push the changes😅 . I have pushed the

That looks like it is shaping up nicely.

- A nice thing about the 'configuration' elements is that they are already
  JavaScript objects, even before being made into magical AbstractPGXS
  instances. They can have methods that aren't required by AbstractPGXS
  and those can be called early in JS. I think probe() is a good candidate
  for that. The calling code could get the "Windows" instance, for example,
  and call its probe method, which could return a different object to be
  used as the template:

  Windows : { probe: function() {
    return configuration[(...) ? 'Windows-MSVC' : 'Windows-MinGW-w64']; }
  }

  'Windows-MSVC' : { compile: ..., link: ... }

  'Windows-MinGW-w64' : { compile: ..., link: ... }

  Or, probing could work in the Java ServiceLoader kind of way, where the
  dispatch code would simply get the OS name and call probe(osname) on every
  configuration item until one returned true, and then instantiate that one
  as an AbstractPGXS.

  configuration = [
    { name: "Linux", probe: function(os) { return name == os; }
      compile: ..., link: ... },
    { name: "Max OS X", probe: function(os) { return name == os; }
      compile: ..., link: ... },
    { name: "Windows-MSVC",
      probe: function(os) {return os.contains("Windows")&&looks_like_MSVC;},
      compile: ..., link: ... },
    { name: "Windows-MinGW-w64",
      probe: function(os) {return os.contains("Windows")&&looks_like_MGW;},
      compile: ..., link: ... }
  ];

  I am starting to like that idea better, as it simplifies the dispatcher.
  For that approach, I think configuration should be a list rather than a
  map, so that the probe tests happen in a predictable order.

- I don't think formatDefines and formatIncludes make good PGXSUtils
  methods. The syntax could vary across compilers (might it use /D or /I
  on a Windows compiler, for example?). So I think they make better
  AbstractPGXS methods that can be overridden in the configuration.
  There could be a built-in implementation in AbstractPGXS using -D and -I.
  Defines should be provided as a map (key to value) and the option string
  with = sign should be produced by formatting.

- compile() should probably just have another parameter for the list
  of .c files, so that only has to be built in one place in the common
  code. compile() for some platform could still add to the list, if
  there were a special porting file for that platform, or something.
  Similarly with link(), I think the list of object files should be a
  parameter.

- The Maven-interpolated ${...} properties are convenient for getting
  stuff working, but will eventually have to go, as in f413312.

- The hard-coded compile and link options should eventually be teased
  apart into the ones that really are hard-coded in PostgreSQL's PGXS
  makefiles and the ones that come from pg_config CC, CFLAGS, CFLAGS_SL,
  CPPFLAGS, LDFLAGS, LDFLAGS_SL, and so on.

- Those pg_config values don't have to be parsed /completely/, that is,
  I don't see any need to figure out which elements represent defines
  or includes, and parse those into bare maps or lists to combine with
  our own defines or includes. The ones coming from pg_config can be
  assumed to be already in the right format for the compiler used, and
  just kept in the form of command arguments, while we format and add
  the ones we are adding.

- They do need to be parsed into lists though, which on Linux can be
  shown by a little experimentation to require splitting on whitespace
  but not between ' characters (and of course, after splitting, the '
  characters that functioned as quoting should not remain in the values).
  (To experiment, it is enough to make a PostgreSQL build and specify a
  weird value for --prefix or something, and look at what pg_config
  produces.) The universal next question, when talking about a format
  that uses ' for quoting is "what is the rule for values that
  contain ' ?" and from my experiments on Linux, the rule is "the
  PostgreSQL build itself breaks" so I don't think we have to cover
  that case. :)

- The same experimenting should probably be done on Windows to see what
  rules are used there. So I think getPgConfigPropertyAsList needs to be
  an AbstractPGXS method. It can call the existing getPgConfigProperty
  to get the raw string, but might use platform-specific rules to break
  it into a list. AbstractPGXS might supply a version that uses the spaces
  and single-quotes rule.

- There may need to be a removeRunPath method though, to cover one bit
  of weirdness. The PostgreSQL build seems to always leave a linker
  option in LDFLAGS to set a runpath to LIBDIR, which should not be
  necessary when building a PostgreSQL extension, as PL/Java is.
  (An extension shared object gets loaded *into* an already running
  PostgreSQL server, which already knows where its other libraries are.)

  It has to be an AbstractPGXS method because the exact option syntax
  to look for and remove from LDFLAGS will be platform/linker specific.
  On Linux it looks like -Wl,-rpath,LIBDIR,--enable-new-dtags
  where LIBDIR is the pg_config LIBDIR property. It might be quoted, so
  of course the removal should happen after the splitting of LDFLAGS into
  a list, which will have removed any ' characters used for quoting.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 26 Aug 2020 02:36:33 +0530

Hi!

> Regarding this, it seems that it is not possible to invoke a method whose
implementation
is provided in the Java class before an object is created. (Same for
default methods in an
interface) So, compile itself cannot call a formatDefines if a default
implementation is given using
AbstractPGXS. However, formatDefines can be called separately and the value
be passed to compile.
So, it would need creating a define method which also decides the defines
for the interface and
formats them. Another way is to pass the formatDefines function as a
parameter to compile. But
this only works for Graal and not nashorn. I am not sure whether nashorn
doesn't support it or there is
some way to do it I do not know of.

Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Tue, 25 Aug 2020 19:14:54 -0400

> AbstractPGXS. However, formatDefines can be called separately and the value

The way I imagined it working, there would be hardly any methods on the
JS object that would be expected to be called before using it to
instantiate an AbstractPGXS subclass. Only for the probe method did that
seem convenient.

What I had imagined was a dispatcher that would select one of the JS
objects, by calling their probe methods, and then use the selected one
to make an AbstractPGXS instance, and call other methods like compile
and link on that instance.

I've tried the basic idea in nashorn and jshell, like this:

jshell> import javax.script.ScriptEngineManager

jshell> var sem = new ScriptEngineManager()
sem ==> javax.script.ScriptEngineManager@5a5a729f

jshell> var se = sem.getEngineByName("javascript")
Warning: Nashorn engine is planned to be removed from a future JDK release
se ==> jdk.nashorn.api.scripting.NashornScriptEngine@6f6745d6

jshell> public abstract class Foo { public abstract int foo(); public
abstract void bar(); public void baz() { System.out.println("Whee!"); } }
|  created class Foo

/*
 * classes declared in jshell have weird names, so the only way I know to
 * make Foo known to JS is with these 2 steps a normal class wouldn't need.
 */

jshell> se.put("FooClass", Foo.class)

jshell> se.eval("var Foo = FooClass.static")
$6 ==> null

jshell> se.eval("var obj = {foo: function(){return 42;}, bar:
function(){this.baz();}}")
$7 ==> null

/*
 * obj is an ordinary JS object. Its foo method already works.
 */
jshell> se.eval("obj.foo()")
$8 ==> 42

/*
 * its bar method doesn't, because there's no baz method.
 */
jshell> se.eval("obj.bar()")
|  Exception javax.script.ScriptException: TypeError: this.baz is not a
function in <eval> at line number 1

/*
 * Now make bigobj be a subclass of Foo, based on obj.
 */

jshell> se.eval("var bigobj = new Foo(obj)")
$10 ==> null

jshell> se.eval("bigobj.foo()")
$11 ==> 42

jshell> se.eval("bigobj.baz()")
Whee!
$12 ==> null

So that looks like it works ok. One thing I noticed is that bigobj does
not keep any JS methods from obj, if they aren't declared in the Java
class. So, for example, if I declare the Java class Foo without its foo
method, I can still call obj.foo(), but not bigobj.foo(). The adapter
magic only exposes the exact API declared by the Java object.

But that seems ok. For example, we could leave the probe method undeclared
in AbstractPGXS. Every JS recipe would have a probe method, and then when
the selected one is used to make a PGXS object, that doesn't have an
accessible probe method anymore, which it doesn't need anymore anyway.

I have noticed a quirk (in nashorn anyway, haven't tried graal) where
even the methods that are declared in the superclass can have trouble
referring to each other as this.method() even though calling them from
outside as bigobj.method() works fine. The dynamic nature of javascript
allows a workaround: they can refer to each other as bigobj.method().

I think that should be workable, as we will only be making one instance
of one of these objects, and probably saving that in an outer var, maybe
pgxs, and so if its JS-implemented methods internally refer to each other
it can just look like methoda: function() { pgxs.methodb(); }.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 26 Aug 2020 10:14:45 +0530

Hi!

> it can just look like methoda: function() { pgxs.methodb(); }.

I had done the similar experimentation as you did with similar results in
Nahorn and Graal.
Both did not work when I tried to use this. I was able to call the methods
externally
but not inside each other. But that outer variable work around gives the
desired
results and I will use it that way. Thanks.

Also, I had another question: is there any particular reason that compiling
and linking should
be two different commands. To me it appears to be another one of nar maven
quirks.
Compilers by default compile and link in a single step. It seems nar maven
overrides it due to
reason. Since, there is no configuration to suggest that this is desired I
am a bit confused.
Do we want to compile and link in different steps ?

Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Wed, 26 Aug 2020 01:09:19 -0400

> Do we want to compile and link in different steps ?

Yes ... because not only is it done by the nar-maven-plugin (which is not
what we want to imitate), it is also done that way by the PGXS makefiles
(which are what we want to imitate). In fact it has been the usual practice
for about as long as there have been makefiles.

One reason is that compiling C programs is a kind of local activity.
Each .c file is transformed into a .o file, without needing to know
very much about the others. (What little it does need to know, it
gets from .h files.)

Conceptually, it's a different operation once you have all of those .o
files and you want to edit and link all of them together into some kind
of binary image with the references resolved. On top of that, there are
choices in what kind of result to produce. A standalone executable?
A static library? A dynamic library? A loadable plugin? (The Mac OS
linker really makes a distinction between a regular dynamic library
and a plugin. The unresolved references in a dynamic library have to
be findable in the other libraries given as its dependencies. For a
plugin, the assumption is they will be findable in the program that
the plugin gets loaded into. The Mac linker actually checks that at
link time, which is why there is a -bundle_loader option added in
the Mac OS profile in pljava-so.)

Even when you can just pile all the compiling and linking options onto
one gcc command line, that still is just the gcc wrapper command being
clever and sorting the options out and running the preprocessor and
compiler and then the linker. But for clarity in makefiles, it is very
common to separate the rules for compiling the components from the rules
for linking the final product.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Wed, 26 Aug 2020 22:25:08 +0530

Hi,

> for linking the final product.

Understood, that makes sense.
-------------------------------------------
I have made some progress on the plugin. The Mac OS X configuration is
working and passing the CI. I have also begun work on completing the
Windows configuration. I am having some issues with the File.separator
being used in paths. Once I am able to resolve it, we should have all the
basic functionality ready. Also, I noticed that running pg_config in the
Windows command line (MSVC) environment gives values of cc, cflags and
ldflags so on as not recorded.
Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Wed, 26 Aug 2020 15:25:02 -0400

> ldflags so on as not recorded.

That rings a bell. I seem to recall that PostgreSQL's own build process,
when using MSVC, operates rather differently from other build environments.
A lot of what would be passed as command-line arguments to other compilers
or linkers will be used to generate 'project' or 'solution' files instead,
and MSVC reads those:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/tools/msvc/README;hb=HEAD

I have not delved into whether the nar-maven-plugin is doing something
similar under the hood.

PostgreSQL's src/tools/msvc seems to have around 2400 lines of Perl
dedicated to writing properly-formatted project and solution files.

(In reality, it's not really that bad, because that's the build system
/for PostgreSQL itself/, and a lot of the lines in Solution.pm are
devoted to writing the files like pg_config.h with the right values
in them. When building an extension, that just gets #include'd; it was
already written when PostgreSQL was built, so we don't have to generate it.)

The info on the (slightly less involved) needs for simply building an
extension with MSVC is here:

https://wiki.postgresql.org/wiki/Building_and_Installing_PostgreSQL_Extension_Modules#Windows_with_Microsoft_Visual_Studio

To me, translating a couple thousand lines of Perl to Java in order to
generate project and solution files would have a very unappetizing sound.

If the nar-maven-plugin is also generating such files when it uses MSVC
to build, then probably it also has some classes to help generating those
files (or it uses a dependency that has some classes to help generating
those files). It might not be too bad to end up adding that as a dependency
of the PGXS plugin (added by a profile conditioned on Windows os) in order
to reuse that part of its code, if it is accessible.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 26 Aug 2020 18:04:32 -0400

> to reuse that part of its code, if it is accessible.

That stuff seems to be here, and accessible:

https://maven-nar.github.io/apidocs/com/github/maven_nar/cpptasks/ide/ProjectWriter.html

... but I have never really watched the nar-maven-plugin closely enough
on Windows to see if it even creates such a file for pljava, or just passes
everything on the command line. I don't know enough about MSVC to know if
the project file is mandatory.

In your commit cb0ceb9 I see that maybe you could get by with only the
command line.

Of course I would handle the /DNDEBUG, /DWindows, /DWIN32, and
/DPLJAVA_SO_VERSION=1.6.0-SNAPSHOT by defines.add (without the /D)
and overriding formatDefines to supply /D rather than -D. And really,
the PLJAVA_SO_VERSION=project.version should be among the base defines
supplied to any template.

I would rather have the probing be done in the probe method; it could
just have an OS name parameter and return a boolean.

"Windows-MinGW-w64" : {
  probe : function(os) {
   return os.startsWith('Windows') && System.gete...GW64");

"Windows-MSVC" : {
  probe : function(os) {
   return os.startsWith('Windows') && System.gete...VCINSTALLDIR");

or something like that, where the outer container of the templates would
be a [ ] rather than a { } so there would be a reliable order for probing
the templates. The driving code could just go down the list calling
probe(os) until one returned true, and use that one.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 26 Aug 2020 18:18:42 -0400

> being used in paths. Once I am able to resolve it, we should have all the

Serves you right!  :)

>   stuff working, but will eventually have to go, as in f413312.

Remember that when you write something like this in the Maven config:

var source_path = java.nio.file.Paths.get("${basedir}", "src", "main", "c");

Maven has no idea that this config string is JavaScript code, and Maven
knows nothing about JavaScript syntax. It's just going to stick the value
of ${basedir} into the middle of this long text value that happens to be
JavaScript.

And if that value contains \ characters, those have special meaning in JS
string literals. Maven didn't know any better.

All of those string interpolations have to go, and become, for example,

var source_path = java.nio.file.Paths.get(project.basedir, "src", "main", "c");

as was done in f413312.

Regards,
-Chap

---

From: Chapman Flack <[email-masked]>
Date: Wed, 26 Aug 2020 20:42:11 -0400

On 08/26/20 12:55, Kartik Ohri wrote:
> working and passing the CI.

This is going to be good. I noticed that a couple of recent spurious
failures in Travis on my branch were because something went wrong
with travis_install_openssl.sh ... in one case, port install openssl
just hung and the job timed out; in another it said "Port openssl not
found".

What's going to be good is I don't even think that should be necessary
at all. The postgresql build being used came from homebrew, not macports,
and the log shows the homebrew package for openssl was installed at the same
time as a dependency. But homebrew's openssl package is "keg-only", which
means the files don't appear in the usual places; you have to specify
their package-specific locations.

That's probably why there seemed to be a problem early on and installing
(another) openssl package from macports seemed to solve it.

But I see that running pg_config of the homebrew-built PostgreSQL
already shows:

CPPFLAGS = ... -I/usr/local/opt/[email-masked]/include ...
...
LDFLAGS = -L/usr/local/opt/[email-masked]/lib ...

so as soon as our build is actually using the options from pg_config,
the build should succeed using the expected homebrew openssl package,
and the whole travis_install_openssl.sh script can just go away.

It's nice to see immediate benefits of the work....

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Thu, 27 Aug 2020 17:22:36 +0530

> probe(os) until one returned true, and use that one.

Yes, I am going to incorporate these changes in the configuration block. I
had hardcoded the values for the sake of getting a basic implementation
ready. A basic implementation is ready and now is working on for all
configurations except MinGW with a weird error
https://ci.appveyor.com/project/amCap1712/pljava/builds/34883248/job/0s01axnd9t76o6da
.

It seems there is some issue in linking but the logs do not hint this.
Another possible reason could be the packaging module not being able to
find the .dll. But again no sign of this from the logs. Do you have any
idea about what could be the issue here ?

Thanks
Regards,
Kartik

---

From: Kartik Ohri <[email-masked]>
Date: Thu, 27 Aug 2020 17:24:06 +0530

> var source_path = java.nio.file.Paths.get(project.basedir, "src", "main",

 Yeah, this was the issue all along and I realised it a bit late. I have
already replaced the maven interpolated properties now and pushed the
changes.

---

From: Kartik Ohri <[email-masked]>
Date: Thu, 27 Aug 2020 18:47:23 +0530

>
>
>> Yes, I am going to incorporate these changes in the configuration block.
> I had hardcoded the values for the sake of getting a basic implementation

I was able to solve the issue. The dll file should was prefixed with lib
and it seems that is wrong. Removing lib from the  dll name and it works
fine.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Thu, 27 Aug 2020 09:30:21 -0400

> idea about what could be the issue here ?

The naming of the link target has different conventions depending on
the os. On Unix-like systems, it has a 'lib' prefix and extension .so

On Mac the .so extension changes to .bundle (except that in PostgreSQL's
PGXS they keep .so instead, which we now have the option of changing to,
but we've used the Mac convention of .bundle for several years).

On Windows, the extension changes to .dll and the lib prefix goes away.

You can see these names in the different profiles that set module.pathname
in -packaging/pom.xml. It has to explicitly add .bundle on Mac (because
PostgreSQL would otherwise assume .so). On Windows, it doesn't have to add
.dll (because that is the assumed extension on Windows), but it does drop
the lib prefix.

Your build has produced a file that ends with .dll but still has the lib
prefix.

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Thu, 27 Aug 2020 21:54:38 +0530

Hi!
I have pushed the code incorporating most of the changes you had suggested.
There may a syntax error here and there in the Windows caught which might
have gone uncaught. I'll build on Windows soon to be sure that it works.
Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Thu, 27 Aug 2020 20:22:43 -0400

Hi,

> and it seems that is wrong. Removing lib from the  dll name and it works

I am looking over the code now.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

How about compile() and link() returning int, and just returning the
exit status of the command, so the common code can compare to zero
or throw MojoFailureException. Just info()ing it isn't enough; yes,
the build would probably fail at a later step, but it wouldn't be
immediately clear where it had gone wrong.

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

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

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

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Fri, 28 Aug 2020 09:29:54 +0530

On Fri, Aug 28, 2020 at 5:52 AM Chapman Flack <[email-masked]> wrote:

> Yeah, actually it was intentional. I was not sure whether to remove that
altogether or put it as a comment in the javascript code.

> Yeah, 😅. Will fix it.

> Regarding this, I was thinking if we could return a ProcessBuilder rather
and execute the command in the common code. The only issue I thought that
may arise is when we try to PIPE the output of the ProcessBuilder because I
am sure how that is done. What do you think ?

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

I will fix all the issues you pointed out and get back to you.
Thanks.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Fri, 28 Aug 2020 08:13:21 -0400

> am sure how that is done. What do you think ?

I think that when reading the piped output from the process, the different
platform implementations will want to process it differently, because the
messages from different compilers will match different patterns. That
might involve having runCommand take another parameter that is a filter
for messages, and each implementation would pass its own filter. That
means it is probably easier to continue invoking runCommand within the
implementation.

But returning the exit code so the common code can check it and throw
MojoFailureException would be easy.

It occurs to me that now that the pljava.libjvmdefault business is
implemented in the script, it would be worth testing to make sure
it works on all platforms. After #289 is merged, that would just
mean passing the -Dpljava.libjvmdefault="$libjvm" on the mvn command
line (after computing the right $libjvm), and removing the
pljava.libjvm_location settings from [1] and [2], and making sure
everything still passes.

Also, unit testing. Have you added jUnit tests in a Maven-built
project before? It is fairly straightforward; you can add some
Java files under src/test/java and if Maven finds any there, it
will build them and run them as tests.

There are, so far, very few examples in the PL/Java codebase, so there
is not a very high existing bar for you to meet, but it would be
worthwhile to start the ball rolling by having at least one file in
pljava-pgxs/src/test/java with a small handful of tests in it.

An existing example you can look at is in pljava-api/src/test/java.

One good candidate for testing would be getPgConfigPropertyAsList.
It should be tested on some systematically-constructed examples
as well as on a real-world example like [3].

Regards,
-Chap

[1] https://github.com/tada/pljava/blob/943152b/.travis.yml#L112
[2] https://github.com/tada/pljava/blob/943152b/appveyor.yml#L130
[3] https://travis-ci.com/github/amCap1712/pljava/jobs/378459461#L2217

---

From: Kartik Ohri <[email-masked]>
Date: Sat, 29 Aug 2020 22:52:28 +0530

Hi!
I just noticed on Travis that the source build
<https://travis-ci.com/github/amCap1712/pljava/jobs/379057991> is failing
due to some issue with the linker flags. I'll take a look soon and update
you with more details.
Regards,
Kartik

---

From: Chapman Flack <[email-masked]>
Date: Sat, 29 Aug 2020 13:27:57 -0400

> due to some issue with the linker flags. I'll take a look soon and update

Aha! I suspect you will find this connected to the topic below. :)

On 08/28/20 08:13, Chapman Flack wrote:
> Also, unit testing. ...

Regards,
-Chap

---

From: Kartik Ohri <[email-masked]>
Date: Sun, 30 Aug 2020 01:43:19 +0530

While I am fixing the above things, should I also remove references to the
old profiles from the docs (linkpglibs, runpath and wnosign) ?

---

From: Chapman Flack <[email-masked]>
Date: Sat, 29 Aug 2020 16:15:21 -0400

> old profiles from the docs (linkpglibs, runpath and wnosign) ?

Yes, thank you. Good catch.

Regards,
-Chap