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

[David Holmes]

Thanks Jim, I'm much happier now.

David
-----

On 8/01/2012 2:38 AM, James Melvin wrote:
> 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