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

David Holmes david.holmes at oracle.com
Sun Jan 8 15:51:19 PST 2012

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 $(JAVAFLAG) 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 FULLVERSION? 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 $(LIBRARYSUFFIX) instead of hardcoded .dylib. However, I really don't have a way of testing this for those other BSDs. line 484: The DYLDLIBRARYPATH part is MacOS X specific. Will still need to set LDLIBRARYPATH for other BSDs. Also, a good point. I've re-added LDLIBRARYPATH 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/osbsd.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 $JNILIBSUFFIX 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/javamd.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 $JAVAHOME/jre/lib. Because Mac OS X files are case-insensitive by default, we collide on $FRAMEWORK/libJPEG.dylib and ${JAVAHOME}/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/. 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 testgamma script assumes an Oracle JDK layout for JAVAHOME, derived from ALTBOOTDIR. Using an Apple JDK for ALTBOOTDIR will fail the testgamma script gracefully, as libjava.dylib is in a different, unexpected place. 3) Modify testgamma script to set library path only for gamma launch Setting DYLDLIBRARYPATH 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

More information about the macosx-port-dev mailing list