Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jzCIA-0003OL-DB for pljava-dev@arkaria.postgresql.org; Sat, 25 Jul 2020 05:00:14 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jzCI7-0006jd-4W for pljava-dev@arkaria.postgresql.org; Sat, 25 Jul 2020 05:00:11 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jzCI6-0006jW-Kq for pljava-dev@lists.postgresql.org; Sat, 25 Jul 2020 05:00:10 +0000 Received: from mail-ed1-x541.google.com ([2a00:1450:4864:20::541]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jzCHz-00089n-Gh for pljava-dev@lists.postgresql.org; Sat, 25 Jul 2020 05:00:09 +0000 Received: by mail-ed1-x541.google.com with SMTP id bm28so8430093edb.2 for ; Fri, 24 Jul 2020 22:00:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=YmydCWyFfBOpP0Ze3+RIpnwpt144j3xR/yp8NotR17o=; b=fZ8YGyF8ZM302UAq2l2jBhPrhCKKURJIIZSaoJ0YP8iQU8QfKsFy7jQlGFjd+3Bymq o0AUnPML+WnCmO6RsNdPrIMr/0CwYdsVFEZIIK/vqhjm++nGcNF7aX0BHTkxB2doOcYS cWBnYloiKCEvbjtOFuwzrC9xYf1x/V06KK74OX/KF821wGU1AQ2Gwuw+p4nfC/ZkEQx9 1g/I3BeItFkjTKbZkjMPTsmzUubk2V1xHI1VhvxeAAUwUoXAfOLLjcFvctjyivFh9STb yBayvnObz9zK/Ipsxatj9HwhvV22XBvlfwOsE5CbH14lOHJMHGeWhG1tZThVm1ioJlof bpkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=YmydCWyFfBOpP0Ze3+RIpnwpt144j3xR/yp8NotR17o=; b=XVYBiD9Ux7wXlOfFjlS0xGv5316fe23FwYFlEGL1gZ1J5XyiS5bCT4dxAKZ57spwUN XUmNLkdR7iUSq6EoG7kIyem0+vabaANLCXhQ09rQOW4MpRlE21HD2koNZzqPzXX/s7Qa fdwcqPeZiEeq6wlU/GEl0Yt+wdOyMJwf/caGw9S3onVoUB+NzhzbhTsgEsJouauIL+3s Hra7cx3xZJ4pFL3kWivHMSIGBOLoBAXuv1XM6UmZkA8EV+FOq4oF7Ntaa/dNhBw+5v1t LI82y9ria0Guf1LXUeeruy2olZMPXW8JLMAPOivw2deHjQdRbo34H0hawvU0Vb34u2rJ 4RXg== X-Gm-Message-State: AOAM533sqTABRV1JLO+qRaVfS1BL+ACR0l+bz6IJbpbO3VWMOwuIb+rY +RqM5gH6+vyvfLHPT6yH36f9+5T4HTMHdSmbqQysFvyNFDM= X-Google-Smtp-Source: ABdhPJwgeK/OJ26aP41QN1sKQKYhKm/TsP1uqnYDq8SSFqUhv/glaOc4/6BQAfnSvBILpdMfjAcRfa7EZQgseKiYzL0= X-Received: by 2002:aa7:c4d8:: with SMTP id p24mr11570531edr.323.1595653200497; Fri, 24 Jul 2020 22:00:00 -0700 (PDT) MIME-Version: 1.0 References: <5F14C34E.4020704@anastigmatix.net> <5F14E15A.4040202@anastigmatix.net> <5F1664C5.2070700@anastigmatix.net> <5F1735ED.6020707@anastigmatix.net> <5F1753C9.4060500@anastigmatix.net> <5F175F40.1000905@anastigmatix.net> <5F18C58F.8060204@anastigmatix.net> <5F19C36C.70905@anastigmatix.net> In-Reply-To: <5F19C36C.70905@anastigmatix.net> From: Kartik Ohri Date: Sat, 25 Jul 2020 10:29:48 +0530 Message-ID: Subject: Re: PL/Java new build plugin To: Chapman Flack Cc: pljava-dev@lists.postgresql.org Content-Type: multipart/alternative; boundary="00000000000033514205ab3cf626" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000033514205ab3cf626 Content-Type: text/plain; charset="UTF-8" On Thu, Jul 23, 2020 at 10:35 PM Chapman Flack 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 {@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)} for printing to standard output. > - *

> ++ *

> + * This is how, for example, to prepare, then print the results > of, a > + * {@code PreparedStatement}: > - *

> ++       * 
> +        * PreparedStatement ps = conn.prepareStatement("select foo(?,?)");
> +        * ps.setInt(1, 42);
> +        * ps.setString(2, "surprise!");
> +        * qp(ps, ps::execute);
>  -       *

> ++       *
> + * The {@code Statement} will be closed. > + */ > - public static void qp(Statement s, Callable work) throws > Exception > ++ public static void qp (Statement s, Callable 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 --00000000000033514205ab3cf626 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Thu, Jul 23, 2020 at 10:35 PM Chapman = Flack <chap@anastigmatix.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 st= ory.

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 squashe= d
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 need= s 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.<= br>
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&q= uot; ... "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<= br> 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:

=C2=A0-=C2=A0 =C2=A0 =C2=A0 public static void qp(Connection c, String sql)= throws Exception
++=C2=A0 =C2=A0 =C2=A0 public static void qp (Connection c, String sql) thr= ows Exception
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qp(q(c, sql));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/**
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Invoke some {@code execute} method on a {@co= de Statement} and pass
=C2=A0-=C2=A0 =C2=A0 =C2=A0 =C2=A0* the {@link #q(Statement,Callable) resul= t stream}
++=C2=A0 =C2=A0 =C2=A0 =C2=A0* the {@link #q(Statement, Callable) result st= ream}
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * to {@link #qp(Stream<Object>)} for pri= nting to standard output.
=C2=A0-=C2=A0 =C2=A0 =C2=A0 =C2=A0*<p>
++=C2=A0 =C2=A0 =C2=A0 =C2=A0* <p>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * This is how, for example, to prepare, then p= rint the results of, a
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * {@code PreparedStatement}:
=C2=A0-=C2=A0 =C2=A0 =C2=A0 =C2=A0*<pre>
++=C2=A0 =C2=A0 =C2=A0 =C2=A0* <pre>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * PreparedStatement ps =3D conn.prepareStateme= nt("select foo(?,?)");
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * ps.setInt(1, 42);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * ps.setString(2, "surprise!");
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * qp(ps, ps::execute);
=C2=A0-=C2=A0 =C2=A0 =C2=A0 =C2=A0*</pre>
++=C2=A0 =C2=A0 =C2=A0 =C2=A0* </pre>
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * The {@code Statement} will be closed.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
=C2=A0-=C2=A0 =C2=A0 =C2=A0 public static void qp(Statement s, Callable<= Boolean> work) throws Exception
++=C2=A0 =C2=A0 =C2=A0 public static void qp (Statement s, Callable<Bool= ean> 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.
=C2=A0
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 cherr= y picked the relevant commits because I had made changes to the commits bef= ore it was forked. You can check the commits and let me know if any more ch= anges are needed.=C2=A0

----

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 coupl= e 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 as= sume
you're not going for the "write a GNU Makefile parser" approa= ch.)

There are probably some reasonable ways you could decide to transcribe
different platform 'recipes' from Makefile.shlib into some JavaScri= pt
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-plu= gin#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,<= br> 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=C2=A0implementat= ion to see how it invokes the JavaDoc. From the commit messages, I can gath= er that the pain point is the lack of ability to configure it. I'll exp= ose a method through the=C2=A0mojo just like the=C2=A0previous one to fix t= hat.=C2=A0
Regards,
-Chap

Regard,
Kartik=C2=A0
--00000000000033514205ab3cf626--