Re: PL/Java new build plugin

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