Code Review Request for MacOS X build change (7117748)

[Daniel D. Daugherty]

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.