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

[James Melvin]

Staffan,

Thanks for the review.  Coming back to this work after a few other
initiatives.  Comments inline...

On 1/20/12 2:17 PM, Staffan Larsen wrote:
> Sorry if I am late to the game, but I was just wishing for this
> functionality - and there was the web rev!
>
> I've looked at this version of the patch:
> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.03/
>
> It looks good. Only two small comments:
>
> java_md.c:GetJVMPath - I don't think the code inside "#ifndef GAMMA"
> needs to be changed. The code is always compiled with GAMMA defined. The
> "#ifndef GAMMA" is just there to facilitate comparison with the original
> code in the JDK.

Correct. The code in the jdk has also changed to something similar, but
with ifdefs instead of code logic. I'm not sure of the history behind
the gamma launcher and how much effort has been expended to keep the jdk
and hotspot launchers in sync in these sections. There are additional
changes proposed to the jdk launcher. They are in review at the moment.
I'd like to keep the current code in place and revise later to reflect
the jdk changes when the dust settles.

>
> os_bsd.cpp:2580 - (nit) incorrect indentation

Hmmm... This must be a webrev/browser thing. I'm looking in xemacs and
the webrev via Firefox on Mac and I don't see an indentation problem.
I'll send you the source file in a private email to inspect. Let me know
if you still see the problem.

>
> I have also verified that the "hotspot" launcher works after this change.

Great! Note, the gamma launcher expects an OpenJDK JAVA_HOME layout.
Therefore, it will not work with the Apple layout. This is detected in
the test_gamma script and a more appropriate error message is displayed.
Just change JAVA_HOME and you will see what I mean.

Again, thanks for the review!

Jim


>
> /Staffan
>
>
> On 10 jan 2012, at 22:05, James Melvin wrote:
>
>> Hi Dan,
>>
>> Final webrev to reflect your comments...
>>
>> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.02
>>
>> Minor changes this round:
>>
>> make/bsd/makefiles/buildtree.make # Fail gracefully on Apple BOOTDIR
>> make/bsd/makefiles/launcher.make # Link with framework only on Mac
>> src/os/bsd/vm/os_bsd.cpp # Just spelling fix
>>
>> Lastly, I wanted to reply to John Coomes comments earlier about the
>> test_gamma script simplification. Although I also value economy of
>> expression, in this case I think the use of more advanced shell
>> constructs increases the time for fresh eyes to decipher. Given
>> performance and such is not an issue, I'd prefer to keep the simpler
>> version I'm proposing with this change on Mac OS X, to make it easier on
>> future maintenance. This should be a model for the other platforms when
>> we reconcile. I've attached the before and after copies should there be
>> further comments on the simplified short script.
>>
>> Thanks,
>>
>> Jim
>>
>>
>> On 1/9/12 6:37 PM, Daniel D. Daugherty wrote:
>>> On 1/7/12 9:38 AM, James Melvin wrote:
>>>> WEBREV:
>>>> http://cr.openjdk.java.net/~jmelvin/7125793/webrev.01
>>>
>>> make/bsd/Makefile
>>> No comments.
>>>
>>> make/bsd/makefiles/buildtree.make
>>> No comments.
>>>
>>> make/bsd/makefiles/defs.make
>>> Thanks for fixing this one for BSD platforms.
>>>
>>> make/bsd/makefiles/launcher.make
>>> line 60: typo: 'inadvertenly' -> 'inadvertently'
>>>
>>> Sorry I missed this in my first review, but the addition
>>> of '-framework CoreFoundation' to LFLAGS_LAUNCHER is
>>> probably MacOS X specific. I think:
>>>
>>> ifeq ($(OS_VENDOR), Darwin)
>>> else
>>> endif
>>>
>>> will work in launcher.make also.
>>>
>>> make/bsd/makefiles/vm.make
>>> No comments.
>>>
>>> src/os/bsd/vm/os_bsd.cpp
>>> line 2544: typo: 'overriden' -> 'overridden'
>>> line 2588: typo: 'overriden' -> 'overridden'
>>>
>>> Looks like old code line 2576 depended on the 'hotspot'
>>> symlink to refer to either 'client' or 'server' or whatever
>>> JVM you wanted to run. I'm fairly certain that the 'hotspot'
>>> symlink was retired; I'm just not sure when.
>>>
>>> src/os/posix/launcher/java_md.c
>>> No comments.
>>>
>>> Dan
>>>
>>>
>> <test_gamma.before><test_gamma.after>
>