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

[James Melvin]

Hi Dan,

Finally getting back on the trail to fix the gamma launcher. Sorry for
the delayed response. Thanks for the review, Dan and David. Replies
inline...


On 1/3/12 1:39 PM, Daniel D. Daugherty wrote:
>> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.00
>
> Jim,
>
> Thanks for diving in and improving the MacOS X port!
> Comments below.
>
> Dan
>
>
> make/bsd/makefiles/buildtree.make
>
> line 422: The new 'java -fullversion' invocation does not include
> the $(JAVA_FLAG) option like the old code did. Any particular
> reason for the change?
>
> Looks like that means the '-d32' or '-d64' options won't be
> specified as they were before.

Originally, this no longer made sense as both -d32 and -d64 were mapped
to 64-bit. After further review, I'm going to readd this option in case
we ever change our minds and decide to support both 32 and 64-bit JVMs
on Mac OS X.


>
> line 447: Why not just echo FULL_VERSION? Why pipe to awk?

To preserve the original script output, I needed to trim the extra
newline... 1 from $FULLVERSION and 1 from echo.

>
> line 465: The 'jre/lib/libjava.dylib' part of the new check is
> MacOS X specific. Other BSDs don't necessarily use the
> '.dylib' extension (instead of .so) and I don't think that
> other BSDs have dropped the "arch" subdir.

To be more friendly to other BSDs, I've added a $(LIBARCH) subdir check
and $(LIBRARY_SUFFIX) instead of hardcoded .dylib. However, I really
don't have a way of testing this for those other BSDs.

>
> line 484: The DYLD_LIBRARY_PATH part is MacOS X specific. Will
> still need to set LD_LIBRARY_PATH for other BSDs.

Also, a good point. I've re-added LD_LIBRARY_PATH with it's original
setting.

>
> line 492: You switched from $(TESTFLAGS) to literal flag values,
> but you left the TESTFLAGS variable around. Any reason for
> the switch?

Nice find.  Cut-paste overwrite.  Fixed by restoring $(TESTFLAGS).

>
>
> make/bsd/makefiles/launcher.make
> Please add a comment explaining why '-framework CoreFoundation'
> is needed. Your explanatory block below is a really good start.

Done.

>
>
> make/bsd/makefiles/vm.make
> No comments.
>
>
> src/os/bsd/vm/os_bsd.cpp
> line 2585: Uses a suffix of ".so". That shouldn't work on MacOS X
> since MacOS X uses '.dylib'. That's OK for other BSDs, but not
> MacOS X. Also the comments that mention '.so' should be updated
> to include '.dylib' (not caused by your changes).

I've replaced .so with $JNI_LIB_SUFFIX defined earlier in the source.
In the area comments, I've just dropped .so extension altogether to
cheaply ambiguate.

>
> To David H. - Yes, this change added another '#fdef __APPLE__'. It
> is not the first and it likely won't be the last since we're
> not done yet with the MacOS X port. There are a number of
> things that need to be cleaned up and we're tracking them.
> However, as you know, we don't have enough folks to handle all
> of the work so we'll just have to live with the warts for now.

For this particular change to fix gamma, I've managed to resolve David's
concerns by adding support for no-arch paths in the code rather than
using #ifdefs. However, ifdefs are sprinkled everywhere and this will
need to be resolved whenever we reconcile the unix platforms into a more
common codebase.


>
> src/os/posix/launcher/java_md.c
> No comments.
>

Thanks for the review comments. I've also added a 1 line change in
make/bsd/makefiles/defs.make to fix a build warning around duplicate
targets for Xusage.txt due to a variable expansion. This has already
been resolved for other platforms.


Changes included in new webrev.  More feedback welcome.

WEBREV:
   http://cr.openjdk.java.net/~jmelvin/7125793/webrev.01

TESTS RUN:
   JPRT 2012-01-07-064433.jmelvin.7125793
   local Mac OS X builds/tests


- Jim


>
> On 12/31/11 1:39 AM, James Melvin wrote:
>> Hi,
>>
>> This change fixes the 'gamma' simple launcher for HotSpot on Mac OS X.
>> There were 3 primary changes required to re-enable gamma...
>>
>> 1) Statically link with CoreFoundation framework to resolve symbols
>>
>> The gamma launcher dlopen()s libjava.dylib from $JAVA_HOME/jre/lib.
>> Because Mac OS X files are case-insensitive by default, we collide on
>> $FRAMEWORK/libJPEG.dylib and ${JAVA_HOME}/jre/lib/libjpeg.dylib. This
>> resulted in unresolved symbols in the Mac OS X framework libraries. The
>> solution for gamma was to statically link with CoreFoundation framework
>> to properly resolve framework symbols and allow gamma to successfully
>> dlopen() libjava.dylib.
>>
>> 2) Adjust various paths to reflect no arch subdirs
>>
>> On Mac OS X, there are no arch subdirs, e.g jre/lib vs jre/lib/<arch>.
>> Instead, one can use universal binaries to package multiple
>> architectures in a single binary. At the moment though, we are only
>> building 64-bit non-universal binaries. Note, the test_gamma script
>> assumes an Oracle JDK layout for JAVA_HOME, derived from ALT_BOOTDIR.
>> Using an Apple JDK for ALT_BOOTDIR will fail the test_gamma script
>> gracefully, as libjava.dylib is in a different, unexpected place.
>>
>> 3) Modify test_gamma script to set library path only for gamma launch
>>
>> Setting DYLD_LIBRARY_PATH adversely affects the real java launcher(s).
>> Instead, set this later in the script only for the gamma launcher test
>> run. While in there, I took the liberty of decrypting the script to make
>> it more maintainable and more easily merged whenever we reconcile the
>> unix ports into a single codebase. There is no change to the script
>> output.
>>
>> Feedback welcome...
>>
>> WEBREV:
>> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.00
>>
>> TESTS RUN:
>> JPRT 2011-12-31-061123.jmelvin.7125793
>> local Mac OS X builds/tests
>>
>>
>> Thanks and Happy New Year!
>>
>> Jim