RFR [XS] : jspawnhelper build settings cleanup

[Erik Joelsson]

Thank you, looks good!

/Erik


On 2017-02-09 09:53, Baesken, Matthias wrote:
>
> Hi Erik,  after your comments,  I created a new webrev  , please review :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8174086.1/ 
> <http://cr.openjdk.java.net/%7Embaesken/webrevs/8174086.1/>
>
> >You are correct that JEXEC is suffering from many of the same issues. You are very welcome 
> to fix that too, in this or another bug.
>
> I think this should go into another bug.
>
> Thanks and best regards, Matthias
>
> *From:*Erik Joelsson [mailto:erik.joelsson at oracle.com]
> *Sent:* Mittwoch, 8. Februar 2017 11:11
> *To:* Baesken, Matthias <matthias.baesken at sap.com>; 
> build-dev at openjdk.java.net
> *Cc:* Langer, Christoph <christoph.langer at sap.com>
> *Subject:* Re: RFR [XS] : jspawnhelper build settings cleanup
>
> Hello Matthias,
>
> Thanks for taking this on!
>
> You can still remove "INCLUDE_FILES := jspawnhelper.c".
>
> Another thing that struck me when looking at this is the inconsistency 
> of sometimes using $(MODULE) and sometimes explicitly using java.base 
> in the paths. I think $(MODULE) is better in this context.
>
> Finally you could adopt the newer formatting style of using a space 
> after comma at line 139 and end the eval-call with ', \' after the 
> last argument and the double parenthesis on a new line on its own.
>
> You are correct that JEXEC is suffering from many of the same issues. 
> You are very welcome to fix that too, in this or another bug.
>
> /Erik
>
> On 2017-02-08 09:00, Baesken, Matthias wrote:
>
>     Hello all ,
>
>     Erik suggested to do further cleanups in
>     make/launcher/Launcher-java.base.gmk
>
>      In the jspawnhelper build section.
>
>     Those were the suggestions :
>
>     * Inline BUILD_JSPAWNHELPER_SRC, JSPAWNHELPER_CFLAGS,
>
>     BUILD_JSPAWNHELPER_DST_DIR and LINK_JSPAWNHELPER_OBJECTS. Since these
>
>     variables aren't conditionally changed anywhere, there is really
>     no need
>
>     for the indirection.
>
>     * The whole business of "BUILD_JSPAWNHELPER_LDFLAGS +=
>
>     $(COMPILER_TARGET_BITS_FLAG)64" is confusing to me. Don't we trust
>     the
>
>     compiler for a 64 bit target to produce a 64 bit binary given the
>
>     standard CFLAGS_JDKEXE and LDFLAGS_JDKLIB? I suspect this is just
>     very
>
>     old and confused code
>
>     * The src dir only has the one src file, no need to explicitly
>     list it
>
>     for include.
>
>     * The adding of childproc.o to LIBS can be accomplished using the
>
>     parameter EXTRA_OBJECT_FILES. By using that you automatically get the
>
>     dependency declaration so you can remove the line
>
>     "$(BUILD_JSPAWNHELPER): $(LINK_JSPAWNHELPER_OBJECTS)"
>
>     * The ifeq ($(BUILD_JSPAWNHELPER), 1) is also annoying, just move the
>
>     single conditional into it's place.
>
>     I prepared a webrev for  this.
>
>     After removing the  BUILD_JSPAWNHELPER_LDFLAGS +=
>      $(COMPILER_TARGET_BITS_FLAG)64
>
>     I checked that  the generated executable is still 64bit on
>     AIX/Solaris/Mac-OSX  (however  the  -m64 seems to be gone on Mac
>     after this change
>
>     Compared to before when generating the executable).
>
>     Btw the BUILD_JEXEC  in    make/launcher/Launcher-java.base.gmk
>        looks also a bit strange (similar issues as the jspawnhelper
>     section).
>
>     Bug :
>
>     https://bugs.openjdk.java.net/browse/JDK-8174086
>
>       webrev  jdk10 :
>
>     http://cr.openjdk.java.net/~mbaesken/webrevs/8174086.0/
>     <http://cr.openjdk.java.net/%7Embaesken/webrevs/8174086.0/>
>
>     Please  review my change .
>
>     Thanks ,Matthias
>