Code Review Request for MacOS X build change (7117748)

[Karen Kinnear]

If so - Mandy would be the one to ask ...

thanks,
Karen

On Dec 4, 2011, at 4:36 PM, Daniel D. Daugherty wrote:

> On 12/4/11 2:21 PM, Vladimir Kozlov wrote:
>> Dan,
>> 
>> I understand that module code was there before this fix but I am still concern that if lib/modules is present the makefile will continue execution even if SA_CLASSPATH does not exits.
> 
> Yes, I think that is intentional. But I will figure out who added
> the modules logic and check with them.
> 
> 
>> It looks like current code works for us because we don't have lib/modules in our build environment.
> 
> No, the current code works when SA_APPLE_BOOT_JAVA=true is specified
> on the command line and the boot JDK is in Apple's format because the
> JDI classes are found in classes.jar. When in the boot JDK is not in
> Apple's format and SA_APPLE_BOOT_JAVA is not specified, then the code
> works because the JDI classes are found in tools.jar.
> 
> As long as one of the two expected boot JDKs are provided, then the
> modules code doesn't come into play. Even if lib/modules did exist
> in either of the boot JDKs, as long as either classes.jar or tools.jar
> is found, then all is good.
> 
> 
>> Someone should explain why we need this modules code. Can we remove it?
> 
> I'll check into why that code is there, but I think it is an
> initial stab at modules support for the future...
> 
> Dan
> 
> 
>> 
>> I am fine with the rest changes.
>> 
>> Thanks,
>> Vladimir
>> 
>> On 12/4/11 10:22 AM, Daniel D. Daugherty wrote:
>>> Vladimir,
>>> 
>>> Thanks for the quick review!
>>> 
>>> 
>>> On 12/3/11 11:58 PM, Vladimir Kozlov wrote:
>>>> Sorry for my ignorance (and I can't access bugster now)
>>> 
>>> And the bug hasn't appeared on bug.sun.com yet... sigh...
>>> 
>>> 
>>>> but why we should care about building current Hotspot with Apple's JDK? Is is because Macs in JPRT have only Apple's JDK?
>>> 
>>> The default release in JPRT is 'jdk7' which means that the import
>>> JDK is 'jdk7'. However, the boot JDK for a 'jdk7' release job is
>>> 'jdk6u18'. For MacOS X, we use Apple's bits for the 'jdk6u18'
>>> boot JDK. For a 'jdk8' release job, we use 'jdk7' bits as the
>>> boot JDK. For MacOS X, we use Oracle's 'jdk7' bits...
>>> 
>>> This means that my fix has to work with an Apple JDK or an Oracle
>>> JDK as the boot JDK.
>>> 
>>> 
>>>> What is _JUNK_ line for?
>>> 
>>> Just a dummy variable so that the "INFO" mesg can be output. I tend
>>> to output INFO lines whenever an "ALT_*" variable is used.
>>> 
>>> 
>>>> Why there is no separate check for -f $(SA_CLASSPATH)? Current check also requires presence of lib/modules directory.
>>> 
>>> For these lines:
>>> 
>>> 88 $(QUIETLY) if [ ! -f "$(SA_CLASSPATH)" -a ! -d $(MODULELIB_PATH) ] ; then \
>>> 89 echo "Cannot find JDI classes. Use 1.6.0 or later version of JDK."; \
>>> 
>>> all I did was add quotes around $(SA_CLASSPATH) on line 88
>>> and change the output message on line 89. Since SA_CLASSPATH
>>> can now be empty, I needed to make sure that the '-f' check
>>> didn't blow up due to the empty variable and the error mesg
>>> was confusing with the empty variable also.
>>> 
>>> As for the lib/modules directory, 1) I didn't add that logic and
>>> 2) the logic doesn't require the lib/modules directory. The logic
>>> issues the error message and fails if the file named by
>>> SA_CLASSPATH does not exist and if the lib/modules directory
>>> does not exist. Whoever put that logic in the Makefile is assuming
>>> that if the lib/modules directory exists, then the JDI classes
>>> will be found (somehow).
>>> 
>>> Please let me know if I've resolved your concerns.
>>> 
>>> Dan
>>> 
>>> 
>>>> 
>>>> thanks,
>>>> Vladimir
>>>> 
>>>> On 12/3/11 8:13 PM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>> 
>>>>> I have a fix that allows HSX-23 to be built on MacOS X via JPRT
>>>>> without specifying SA_APPLE_BOOT_JAVA or ALWAYS_PASS_TEST_GAMMA
>>>>> on the command line. I'm targeting this fix at RT_Baseline for
>>>>> the HSX-23-B08 snapshot.
>>>>> 
>>>>> Here is the webrev URL:
>>>>> 
>>>>> http://cr.openjdk.java.net/~dcubed/7117748-webrev/0/
>>>>> 
>>>>> I tested this fix with the default JPRT boot JDK (JDK6 from Apple)
>>>>> and with JDK7 boot JDK (JDK7 bits from Oracle).
>>>>> 
>>>>> Thanks, in advance, for any reviews.
>>>>> 
>>>>> Dan
>>>>> 
>>>>> P.S.
>>>>> This fix does _not_ get "gamma" working on MacOS X.
>>>>> That work is being done separately.