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
