Thanks Kelly. See below for some follow ups...
On 2/22/12 12:51 PM, Kelly O'Hair wrote:
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.
Based on Mike Swingler's advice, I've changed all the affected
callsites for this change to use .contains("OS X").
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. :^(
Good observation. Let me update the comment with something more
appropriate.
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.
Agreed. The likely time to consolidate is when we reconcile the Unix
platforms. I imagine we'll commit to this at some point.
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.
Although it's unlikely corba will revert to using native code, so this
change is mainly to be consistent with the jdk repo.
- Jim
-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
