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 1kBVYk-0008P8-VF for pljava-dev@arkaria.postgresql.org; Fri, 28 Aug 2020 04:00:15 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1kBVYi-0007iA-8h for pljava-dev@arkaria.postgresql.org; Fri, 28 Aug 2020 04:00:12 +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 1kBVYh-0007ht-SQ for pljava-dev@lists.postgresql.org; Fri, 28 Aug 2020 04:00:12 +0000 Received: from mail-ej1-x641.google.com ([2a00:1450:4864:20::641]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kBVYe-0004Jw-VZ for pljava-dev@lists.postgresql.org; Fri, 28 Aug 2020 04:00:10 +0000 Received: by mail-ej1-x641.google.com with SMTP id d11so10463032ejt.13 for ; Thu, 27 Aug 2020 21:00:08 -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=egL4SQOrPkVUSc6J67nKtqbCAaVsE7NLA8ICE5m8llA=; b=As398G9RSJjaWowmN6thNm4L1rG+NO2a4vonx9qLDQlK3AHYqB/boQlM8DNwkS0BGH dhRS9zpmGqMSTrC0/hnj42e+4FeVFqzl4rq1u/p2w8nuTm9NrpSG6g2TmgSR3VDPT/iZ 62pPb0dhg7KbmYRId90kRfj7xcF9f7l+P2LoRGuC3pzip8xAmB0ADuJ0p3I9zAnrzlco A2Rorhwkm2Pf4cpPB5AUzwhg800LxMyLFyqV3Fj4ADI+9iA9UWik6QIrRpVqiIHCZ+Nd LP7Ax32aVZ2A1jsQxqd0HauQ+O0QSNtZ4gQWRDIhMpZzSgOWJUD0QR+oRYwMJtKJ4c/+ mJNg== 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=egL4SQOrPkVUSc6J67nKtqbCAaVsE7NLA8ICE5m8llA=; b=QqBgeCYT5HYpZsgxZdR2OOAGoK2Zm4u9wG/rD+BYKU9jiPL9kOVUf3+zUnZEC6TXDp izDEbuFmw1FMvz6Z+bW5a9VhANmxKZ5F4spzhJQ3zvDhmIOLGJGfdXbPPeDQAksilWPO jirPGXj2/tS3EDK6bPBc1QkKVlnqlHBKBghtobLH7Pp+BE5qsBykXsyzVGVug239Lakj rSy0j/kV/++E0cwqbpRssiDODcqnKY5kEhKIHel/H/lkmiSz8freJtBvw4JFn9OP+zEt xg5+jch+IXzcokvkfmwCdRIXN55fDav3SQb40P5Ey9hbg6ZWBgXg4J1l7VP1iNdef+v8 +TJg== X-Gm-Message-State: AOAM5336/PJS4K6ornvfXdx5uI8YPEug8BgMM44tefLgfNp8a4php9Cb tGXAUAjjFJqQ5xxIwWTCjBhttOZ1LJfo1y6obrrx8JKXwDM= X-Google-Smtp-Source: ABdhPJz7nx/yHeVJdOLwEsmizLLyEqM7F6Ymal4aGZUTuu4RqFPep9DxxMyzxtx5H+uwSnnra+aTF9FDunh3Qb3AH7o= X-Received: by 2002:a17:906:f9cf:: with SMTP id lj15mr24900272ejb.520.1598587206231; Thu, 27 Aug 2020 21:00:06 -0700 (PDT) MIME-Version: 1.0 References: <5F3D3D2E.4070109@anastigmatix.net> <5F3D43BD.4020900@anastigmatix.net> <5F3DA48D.7090408@anastigmatix.net> <5F41943D.9080707@anastigmatix.net> <5F427604.5010103@anastigmatix.net> <5F42A71F.8000002@anastigmatix.net> <5F459B6E.3020803@anastigmatix.net> <5F45EE7F.5020609@anastigmatix.net> <5F46B70E.6070601@anastigmatix.net> <5F46DC70.1080404@anastigmatix.net> <5F484E53.1010209@anastigmatix.net> In-Reply-To: <5F484E53.1010209@anastigmatix.net> From: Kartik Ohri Date: Fri, 28 Aug 2020 09:29:54 +0530 Message-ID: Subject: Re: the ScriptingMojo To: Chapman Flack Cc: pljava-dev@lists.postgresql.org Content-Type: multipart/alternative; boundary="00000000000091efe105ade816ea" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000091efe105ade816ea Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Aug 28, 2020 at 5:52 AM Chapman Flack wrote= : > Hi, > > On 08/27/20 09:17, Kartik Ohri wrote: > > I was able to solve the issue. The dll file should was prefixed with li= b > > and it seems that is wrong. Removing lib from the dll name and it work= s > > fine. > > 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. :) > > Yeah, actually it was intentional. I was not sure whether to remove that altogether or put it as a comment in the javascript code. > 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? > > Yeah, =F0=9F=98=85. Will fix it. 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. > > 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 would add a blank line between implementations, and between methods > in an implementation, just to set them apart a little bit. > > I think this isn't setting any properties anymore, so > its 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 , and ScriptingMojo should > be changed to have defaultPhase =3D COMPILE. > > > I think that's all I've got for now. > > Regards, > -Chap > I will fix all the issues you pointed out and get back to you. Thanks. Regards, Kartik --00000000000091efe105ade816ea Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Fri, Aug 28, 2020 at 5:52 AM Chapman F= lack <chap@an= astigmatix.net> wrote:
Hi,

On 08/27/20 09:17, Kartik Ohri wrote:
> I was able to solve the issue. The dll file should was prefixed with l= ib
> and it seems that is wrong. Removing lib from the=C2=A0 dll name and i= t works
> fine.

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 w= ould
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 han= dled.
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??), an= d
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. :)

Yeah, actually it was intentional. I was not sure whe= ther to remove that altogether or put it as a comment in the javascript cod= e.
=C2=A0
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 an= ything,
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

=C2=A0 l.addAll(of("-bundle_loader", bindir.resolve("postgre= s")));
=C2=A0 l.addAll(of("-o",=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0"lib" + library_name + ".bundle"));

It's also fair to group a slew of fixed unrelated single options that w= ay
too, "/MANIFEST", "/NOLOGO", "/DLL", "/S= UBSYSTEM: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?

Yeah,=C2=A0=F0=9F=98=85. Will fix it.=C2=A0

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.

Regarding this, I was thinking if we could return a P= rocessBuilder rather and execute the command in the common code. The only i= ssue I thought that may arise is when we try to PIPE the output of the Proc= essBuilder because I am sure how that is done. What do you think ?
=C2=A0=C2=A0
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 =3D COMPILE.


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

Regards,
-Chap

I will fix all the issues you poi= nted out and get back to you.
Thanks.
Regards,
Kartik=C2=A0 --00000000000091efe105ade816ea--