Old build problem: unpack200 failure when setting OTHER_CXXFLAGS in environment

[David Holmes]

On 13/03/2012 9:03 AM, Kelly O'Hair wrote:
> Just seems like a very poorly written Makefile to me.
>
> Where is the actual webrev or patch, I could not see it.

http://mail.openjdk.java.net/pipermail/build-dev/attachments/20070511/62a3961e/attachment.bin

Mangled by the mail servers. Here's my own patch:

iff --git a/make/com/sun/java/pack/Makefile 
b/make/com/sun/java/pack/Makefile
--- a/make/com/sun/java/pack/Makefile
+++ b/make/com/sun/java/pack/Makefile
@@ -72,14 +72,14 @@
  	     $(ZIPOBJDIR)/inffast.$(OBJECT_SUFFIX)

    ZINCLUDE=-I$(SHARE_SRC)/native/java/util/zip/zlib-$(ZLIB_VERSION)
-  OTHER_CXXFLAGS += $(ZINCLUDE)
+  CXXFLAGS_COMMON += $(ZINCLUDE)
    LDDFLAGS += $(ZIPOBJS)
   else
    LDDFLAGS += -lz
    OTHER_CXXFLAGS += -DSYSTEM_ZLIB
   endif
  else
-  OTHER_CXXFLAGS += -DNO_ZLIB -DUNPACK_JNI
+  CXXFLAGS_COMMON += -DNO_ZLIB -DUNPACK_JNI
    OTHER_LDLIBS += $(JVMLIB)
  endif

This has now been flagged as part of 7153072.

David
-----

> -kto
>
> On Mar 11, 2012, at 9:48 PM, David Holmes wrote:
>
>> This is a blast from the past:
>>
>> http://mail.openjdk.java.net/pipermail/build-dev/2007-May/000026.html
>>
>> but the above issue and patch seem to have been ignored. I just ran into this myself.
>>
>> /export/users/dh198349/jdk8/builds/b01/se8-linux-i586-ea/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/main.o: In function `unpacker::run(int, char**)':
>> main.cpp:(.text+0xe0b): undefined reference to `gunzip::init(unpacker*)'
>> main.cpp:(.text+0xe1d): undefined reference to `gunzip::start(int)'
>> collect2: ld returned 1 exit status
>> make[7]: *** [/export/users/dh198349/jdk8/builds/b01/se8-linux-i586-ea/bin/unpack200] Error 1
>>
>> The basic issue is one of recursive makes, with conditionally set variables that might also be set externally in the environment. Here's an example Makefile:
>>
>> build: unpack
>>
>> ifdef STANDALONE
>>   FLAGS+=-XstandAlone
>> else
>>   FLAGS+=-Xcombined
>> endif
>>
>> unpack:
>>         @make STANDALONE=true unpack_exe
>>
>> build:
>>         @echo build FLAGS = $(FLAGS)
>> unpack_exe:
>>         @echo unpack_exe FLAGS = $(FLAGS)
>>
>> .phony: build unpack unpack_exe
>>
>> ---
>>
>> Here's a normal run:
>>
>>> make build
>> make[1]: Entering directory `/scratch/dh198349'
>> unpack_exe FLAGS = -XstandAlone
>> make[1]: Leaving directory `/scratch/dh198349'
>> build FLAGS = -Xcombined
>>
>> which is what we would expect. But if you now give FLAGS an initial external value:
>>
>>> FLAGS=external make build
>> make[1]: Entering directory `/scratch/dh198349'
>> unpack_exe FLAGS = external -Xcombined -XstandAlone
>> make[1]: Leaving directory `/scratch/dh198349'
>> build FLAGS = external -Xcombined
>>
>> Yikes! Now unpack_exe sees both the STANDALONE and non-STANDALONE value of FLAGS. This is because make re-exports any variable that came in from the environment. So when the top-level make is called, FLAGS==external, and to that the Makefile adds -Xcombined, so the sub-make effectively becomes:
>>
>> make FLAGS="external -Xcombined" STANDALONE=true unpack_exe
>>
>> Here's one way to fix this:
>>
>> # save original incoming FLGS
>> ORIG_FLAGS := $(FLAGS)
>> # hide any locally modified value of FLAGS
>> unexport FLAGS
>>
>> build: unpack
>>
>> ifdef STANDALONE
>>   # override to allow sub-make to add to FLAGS
>>   override FLAGS+=-XstandAlone
>> else
>>   FLAGS+=-Xcombined
>> endif
>>
>> unpack:
>>         # Send in ORIG_FLAGS as FLAGS
>>         @make FLAGS=$(ORIG_FLAGS) STANDALONE=true unpack_exe
>>
>> build:
>>         @echo build FLAGS = $(FLAGS)
>> unpack_exe:
>>         @echo unpack_exe FLAGS = $(FLAGS)
>>
>> .phony: build unpack unpack_exe
>>
>> ----
>>
>> Or as per the original Patch, use a different variable in the Makefile to that set in the environment.
>>
>> David
>> -----
>