Sorry I'm slow on the review. Just a few comments, not sure any of it would cause me to reject your changes.
On Feb 21, 2012, at 3:08 PM, James Melvin wrote: > Thanks for the reviews! Updated webrevs with all comments... > > http://cr.openjdk.java.net/~jmelvin/7130404/hotspot/webrev.01 Just FYI. I use startsWith("Mac"). But I'm not sure I have an opinion what you should use. Seems like this comment was wrong to begin with, and is wrong now: 55 /* Returns "sparc" if on SPARC, "x86" if on x86. */ 56 public static String getCPU() throws UnsupportedPlatformException { It returns a wide variety of values. :^( > http://cr.openjdk.java.net/~jmelvin/7130404/jdk/webrev.01 There are 4 critical ARCH weapons, all of them have inconsistencies and risks when sharpened or changed: 1. LIBARCH or the name in the install trees, e.g. lib/sparc, lib/i386, lib/amd64 2. os.arch, or what the Java property value is, e.g. x86, sparc, i586, i386?, sparcv9, amd64, x86_64,... 3. The arch name used in the bundle names, e.g. sparc, x86, x64, etc. 4. The arch name used in the build directories, e.g. build/solaris-sparc, build/solaris-x64, ... However, like the Monty Python Spanish Inquisition, http://www.youtube.com/watch?v=vt0Y39eMvpI, nobody really remembers what the real weapon list is. :^( But I digress... In the Makefiles, I was trying to limit the ARCH values, and for 64bit x86 I have tried to use x64 or X64 and have been trying to avoid amd64 or x86_64. This is separate from the above weapon list, they all do not and probably can never all be the same, but where we can avoid repeating the same ARCH with a different spelling, we should. I think the bottom line is that the Defs-macos.gmk and Platform.gmk file probably could be trimmed down for Mac, but I don;t see a pressing need right now. > http://cr.openjdk.java.net/~jmelvin/7130404/corba/webrev.01 Not sure this change is necessary, corba no longer compiles any native code. But it seems harmless. -kto > > Main changes were to refine the use of string compares for Mac OS X... > > < osName.startsWith("Mac OS") > > osName.contains("OS X") > > Any final comments? A decision has not yet been made to include this > change or not. I'll provide an update when there's progress. > > - Jim > > > > On 2/20/12 4:02 PM, James Melvin wrote: >> Hi, >> >> To maintain compatibility with Apple JDKs, a proposal will be made to >> change the 'os.arch' system property from 'amd64' to 'x86_64' on Mac OS >> X. Minor changes are required to the following repositories, for which >> I've provided webrevs... >> >> WEBREV: >> http://cr.openjdk.java.net/~jmelvin/7130404/hotspot/webrev.00 >> http://cr.openjdk.java.net/~jmelvin/7130404/jdk/webrev.00 >> >> TESTING: >> JPRT job (2012-02-20-203901.jmelvin.hotspot) >> Notepad, SwingSet2, SPECjbb2005 >> >> This change will also impact a small number of internal tests and RE >> scripts. The bundle names will also reflect the change amd64 -> x86_64. >> HotSpot changes can be integrated first, with the JDK changes in the >> following promotion. Should the proposal be rejected for 7u4, I >> obviously withdraw the bugfix. >> >> Feedback welcome, >> >> Jim
