RFR (S): 7125793: MAC: test_gamma should always work

[John Coomes]

James Melvin (james.melvin at oracle.com) wrote:
> Hi Dan,
> 
> Final webrev to reflect your comments...
> 
> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.02
> 
> Minor changes this round:
> 
>    make/bsd/makefiles/buildtree.make   # Fail gracefully on Apple BOOTDIR
>    make/bsd/makefiles/launcher.make    # Link with framework only on Mac
>    src/os/bsd/vm/os_bsd.cpp            # Just spelling fix
> 
> Lastly, I wanted to reply to John Coomes comments earlier about the
> test_gamma script simplification. Although I also value economy of
> expression, in this case I think the use of more advanced shell
> constructs increases the time for fresh eyes to decipher. Given
> performance and such is not an issue, I'd prefer to keep the simpler
> version I'm proposing with this change on Mac OS X, to make it easier on
> future maintenance. This should be a model for the other platforms when
> we reconcile. I've attached the before and after copies should there be
> further comments on the simplified short script.

As mentioned before, I have problems with you're proposed change
beyond just brevity.  First, it makes reconciling changes with the
other platforms more difficult, so do all (if you get agreement) or do
none.  Second, any semantic changes are lost in the reformatting
noise.  Do the reformatting separately from the semantic change.

And the expanded version is waaaay to bloated.  I'm open to some code
expansion, but the whitespace decorators around comment blocks for
each trivial statement is too much.  It reminds me of beginning cobol.

-John

> On 1/9/12 6:37 PM, Daniel D. Daugherty wrote:
> > On 1/7/12 9:38 AM, James Melvin wrote:
> >> WEBREV:
> >> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.01
> >
> > make/bsd/Makefile
> > No comments.
> >
> > make/bsd/makefiles/buildtree.make
> > No comments.
> >
> > make/bsd/makefiles/defs.make
> > Thanks for fixing this one for BSD platforms.
> >
> > make/bsd/makefiles/launcher.make
> > line 60: typo: 'inadvertenly' -> 'inadvertently'
> >
> > Sorry I missed this in my first review, but the addition
> > of '-framework CoreFoundation' to LFLAGS_LAUNCHER is
> > probably MacOS X specific. I think:
> >
> > ifeq ($(OS_VENDOR), Darwin)
> > else
> > endif
> >
> > will work in launcher.make also.
> >
> > make/bsd/makefiles/vm.make
> > No comments.
> >
> > src/os/bsd/vm/os_bsd.cpp
> > line 2544: typo: 'overriden' -> 'overridden'
> > line 2588: typo: 'overriden' -> 'overridden'
> >
> > Looks like old code line 2576 depended on the 'hotspot'
> > symlink to refer to either 'client' or 'server' or whatever
> > JVM you wanted to run. I'm fairly certain that the 'hotspot'
> > symlink was retired; I'm just not sure when.
> >
> > src/os/posix/launcher/java_md.c
> > No comments.
> >
> > Dan
> >
> >
> 
> ----------------------------------------------------------------------
> #!/bin/sh
> # Generated by /Users/jmelvin/dev/testing/make/bsd/makefiles/buildtree.make
> . ./env.sh
> if [ "" != "" ]; then { echo Cross compiling for ARCH , skipping gamma run.; exit 0; }; fi
> if [ -z $JAVA_HOME ]; then { echo JAVA_HOME must be set to run this test.; exit 0; }; fi
> if ! ${JAVA_HOME}/bin/java -d32 -fullversion 2>&1 > /dev/null
> then
>   echo JAVA_HOME must point to 32bit JDK.; exit 0;
> fi
> rm -f Queens.class
> ${JAVA_HOME}/bin/javac -d . /Users/jmelvin/dev/testing/make/test/Queens.java
> [ -f gamma_g ] && { gamma=gamma_g; }
> ./${gamma:-gamma} -Xbatch -showversion Queens < /dev/null
> exit 0
> 
> ----------------------------------------------------------------------
> #!/bin/sh
> 
> # Generated by /Users/jmelvin/dev/7125793/make/bsd/makefiles/buildtree.make
> 
> #
> # Include environment settings for gamma run
> #
> 
> . ./env.sh
> 
> 
> #
> # Do not run gamma test for cross compiles
> #
> 
> if [ -n "" ]; then 
>   echo Cross compiling for ARCH , skipping gamma run.
>   exit 0
> fi
> 
> 
> #
> # Make sure JAVA_HOME is set as it is required for gamma
> #
> 
> if [ -z "${JAVA_HOME}" ]; then 
>   echo JAVA_HOME must be set to run this test.
>   exit 0
> fi
> 
> 
> #
> # Report JAVA_HOME version to be used for the test
> #
> 
> FULL_VERSION="`${JAVA_HOME}/bin/java -d64 -fullversion`"
> if [ $? -ne 0 ]; then 
>   echo JAVA_HOME must point to a 64-bit OpenJDK.
>   exit 0
> fi
> echo "${FULL_VERSION}" | awk '{printf("%s",$0);}'
> 
> 
> #
> # Use gamma_g if it exists
> #
> 
> GAMMA_PROG=gamma
> if [ -f gamma_g ]; then 
>   GAMMA_PROG=gamma_g
> fi
> 
> 
> #
> # Ensure architecture for gamma and JAVA_HOME is the same.
> # NOTE: gamma assumes the OpenJDK directory layout.
> #
> 
> GAMMA_ARCH="`file ${GAMMA_PROG} | awk '{print $NF}'`"
> JVM_LIB="${JAVA_HOME}/jre/lib/libjava.dylib"
> if [ ! -f ${JVM_LIB} ]; then
>   JVM_LIB="${JAVA_HOME}/jre/lib/amd64/libjava.dylib"
> fi
> if [ ! -f ${JVM_LIB} ] || [ -z "`file ${JVM_LIB} | grep ${GAMMA_ARCH}`" ]; then 
>   echo JAVA_HOME must point to a 64-bit OpenJDK.
>   exit 0
> fi
> 
> 
> #
> # Compile Queens program for test
> #
> 
> rm -f Queens.class
> ${JAVA_HOME}/bin/javac -d . /Users/jmelvin/dev/7125793/make/test/Queens.java
> 
> 
> #
> # Set library path solely for gamma launcher test run
> #
> 
> LD_LIBRARY_PATH=.:${JAVA_HOME}/jre/lib/amd64/native_threads:${JAVA_HOME}/jre/lib/amd64:
> DYLD_LIBRARY_PATH=.:${JAVA_HOME}/jre/lib/native_threads:${JAVA_HOME}/jre/lib:${JAVA_HOME}/jre/lib/amd64/native_threads:${JAVA_HOME}/jre/lib/amd64:
> export DYLD_LIBRARY_PATH
> 
> 
> #
> # Use the gamma launcher and JAVA_HOME to run the test
> #
> 
> ./${GAMMA_PROG} -Xbatch -showversion Queens < /dev/null