Subject: Re: PL/Java new build plugin
To: Kartik Ohri <kartikohri13@gmail.com>
References: 
 <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
 <CAASLQ4NU3BgfCnVxvAbu9vK9+gtPT87xX1uBj57b0y2J6nxi+Q@mail.gmail.com>
 <5F14C34E.4020704@anastigmatix.net> <5F14E15A.4040202@anastigmatix.net>
 <CAASLQ4PjzGVCj559r60Kw7i3uRUTzJraxBSDDsNtAt8v8_-oWQ@mail.gmail.com>
 <5F1664C5.2070700@anastigmatix.net>
 <CAASLQ4PeivyN6DkBPbgYTZmoqoOVdCDwinDFrREsXqZi2HQnWg@mail.gmail.com>
 <5F1735ED.6020707@anastigmatix.net>
 <CAASLQ4Off+PnB8FqKLokKjAkvKbW00=fjH_LxuYJOGp0_C1YAw@mail.gmail.com>
 <CAASLQ4PcU=P4ShuMJOEnQj6p7ZU0VcgJ9St6PMpXcNB_1jPQ+w@mail.gmail.com>
 <5F1753C9.4060500@anastigmatix.net>
 <CAASLQ4P_v7u5z76XWfPmVeBvQtNdraiN0MmsRRfq-eNh1O1aTA@mail.gmail.com>
 <5F175F40.1000905@anastigmatix.net>
 <CAASLQ4Ouw1hCHVtSC5XG7Fb-GoO0YAf4_PwsQc=22CvEzhQo6g@mail.gmail.com>
 <CAASLQ4MfBz2zj5gm-t+OX-8puxJvf8bSneKeUG+NuX=+CbYCLw@mail.gmail.com>
 <5F18C58F.8060204@anastigmatix.net>
 <CAASLQ4NkEDo-UAmqSwUTkCdMu+3WedKCfva5gzNTC45YcWEwNA@mail.gmail.com>
Cc: pljava-dev@lists.postgresql.org
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <5F19C36C.70905@anastigmatix.net>
Date: Thu, 23 Jul 2020 13:05:48 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
MIME-Version: 1.0
In-Reply-To: 
 <CAASLQ4NkEDo-UAmqSwUTkCdMu+3WedKCfva5gzNTC45YcWEwNA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 {@code execute} method on a {@code Statement} and pass
 -	 * the {@link #q(Statement,Callable) result stream}
++	 * the {@link #q(Statement, Callable) result stream}
+ 	 * to {@link #qp(Stream<Object>)} for printing to standard output.
 -	 *<p>
++	 * <p>
+ 	 * This is how, for example, to prepare, then print the results of, a
+ 	 * {@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 {@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