Re: the ScriptingMojo

On Fri, Aug 28, 2020 at 5:52 AM Chapman Flack <chap(at)anastigmatix(dot)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 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