public inbox for [email protected]
help / color / mirror / Atom feedFrom: Kartik Ohri <[email protected]>
To: Chapman Flack <[email protected]>
Cc: [email protected]
Subject: Re: the ScriptingMojo
Date: Fri, 28 Aug 2020 09:29:54 +0530
Message-ID: <CAASLQ4Nqyt0DR=AhxS2zrwBgz1arWJ8aCkq81pBmf99aJ7y2RA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAASLQ4Mva7dAXiS_c9OLhV4VzNHF=XNKJosuS5gtZTB6b4TW1Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAASLQ4MBWQeFLdAPNcsgcpLyekU5X=ExQUuGgE49AUFdEYJr8g@mail.gmail.com>
<[email protected]>
<CAASLQ4P5oEx0DLoDLEOAkZSwffU2eS2+NKbevgYqpknHGucaTw@mail.gmail.com>
<[email protected]>
<CAASLQ4Nr3MPQuZziVU+89ULZ=BgQjaPPnCLsyPR3mhW-eWoOBw@mail.gmail.com>
<[email protected]>
<CAASLQ4PkP017rtyUswKO_TCiNmnica-nrQHBgggFw-U_9MstUg@mail.gmail.com>
<[email protected]>
<CAASLQ4P47TNiEsuj3ga-vuYeCSBJZmHrPM-49E-X=0T5RTmhKw@mail.gmail.com>
<[email protected]>
<CAASLQ4MH5JaVbrG9O_S+9dnp=tj_8N3jArxxpf-dJD1ezd5SfQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAASLQ4Npudth33qcg8GHaKd0amJ8oSQro_sxgb72VkbkBj+oXQ@mail.gmail.com>
<CAASLQ4NcDozHC8J8RAAosc-X1k7JSTxUdaz-1vFLhpdE1nuXqg@mail.gmail.com>
<[email protected]>
On Fri, Aug 28, 2020 at 5:52 AM Chapman Flack <[email protected]> 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 lib
> > and it seems that is wrong. Removing lib from the dll name and it 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 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, 😅. 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 <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
>
I will fix all the issues you pointed out and get back to you.
Thanks.
Regards,
Kartik
view thread (78+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: the ScriptingMojo
In-Reply-To: <CAASLQ4Nqyt0DR=AhxS2zrwBgz1arWJ8aCkq81pBmf99aJ7y2RA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox