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:[email protected]]
*Sent:* Mittwoch, 8. Februar 2017 11:11
*To:* Baesken, Matthias <[email protected]>;
[email protected]
*Cc:* Langer, Christoph <[email protected]>
*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
