Fine with me.
/Erik
On 2017-02-03 12:08, Baesken, Matthias wrote:
Hi Erik, thanks for your ideas and comments.
While your change is ok and can certainly be pushed on its own, there is
so much more needing cleanup here.
I would like to do the other suggested cleanups in a separate change.
Is this fine, can the original change be pushed ?
Best regards, Matthias
-----Original Message-----
From: Erik Joelsson [mailto:erik.joels...@oracle.com]
Sent: Donnerstag, 2. Februar 2017 17:45
To: Baesken, Matthias <matthias.baes...@sap.com>; build-
d...@openjdk.java.net
Cc: Simonis, Volker <volker.simo...@sap.com>
Subject: Re: RFR [XXS] : cleanup macosx jspawnhelper build settings
Hello Matthias,
While your change is ok and can certainly be pushed on its own, there is
so much more needing cleanup here. If you don't mind, here is what I
think needs doing:
* 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.
Thanks
/Erik
On 2017-02-02 17:25, Baesken, Matthias wrote:
Hello,
could I please have a review for the following small change that
cleans
up the special macosx BUILD_JSPAWNHELPER_DST_DIR setting that is not
really
needed any more after CR 8066474: "Remove the lib/ directory from
Linux and Solaris images" changed the default setting .
Bug :
https://bugs.openjdk.java.net/browse/JDK-8173834
webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8173834.0/
Thanks, Matthias
