Code Review Request for MacOS X build change (7117748)

[Daniel D. Daugherty]

Thanks Vladimir!

Dan


On 12/5/11 1:11 PM, Vladimir Kozlov wrote:
> Thank you, Dan, for digging the history of modules code.
> Yes, I'm OK with your fix.
>
> Thanks,
> Vladimir
>
> On 12/5/11 8:57 AM, Daniel D. Daugherty wrote:
>> make/bsd/makefiles/sa.make was created by cloning
>> make/linux/makefiles/sa.make. The modules logic was
>> added to sa.make by the following changeset:
>>
>>> changeset: 1547:0e7d2a08b605
>>> user: mchung
>>> date: Wed Jul 07 15:35:58 2010 -0700
>>> files: make/linux/makefiles/sa.make make/solaris/makefiles/sa.make 
>>> src/os/linux/vm/os_linux.cpp
>>> src/os/solaris/vm/os_solaris.cpp src/share/vm/runtime/os.cpp
>>> description:
>>> 6967423: Hotspot support for modules image
>>> Summary: Add hotspot support for modules image
>>> Reviewed-by: acorn
>>
>> The diffs for Linux sa.make look like:
>>
>> diff -r 5087ecc10458 -r 0e7d2a08b605 make/linux/makefiles/sa.make
>> --- a/make/linux/makefiles/sa.make Wed Jul 07 14:12:08 2010 -0400
>> +++ b/make/linux/makefiles/sa.make Wed Jul 07 15:35:58 2010 -0700
>> @@ -40,6 +40,9 @@
>> # tools.jar is needed by the JDI - SA binding
>> SA_CLASSPATH = $(BOOT_JAVA_HOME)/lib/tools.jar
>>
>> +# TODO: if it's a modules image, check if SA module is installed.
>> +MODULELIB_PATH= $(BOOT_JAVA_HOME)/lib/modules
>> +
>> # gnumake 3.78.1 does not accept the *s that
>> # are in AGENT_FILES1 and AGENT_FILES2, so use the shell to expand them
>> AGENT_FILES1 := $(shell /usr/bin/test -d $(AGENT_DIR) && /bin/ls 
>> $(AGENT_FILES1))
>> @@ -65,7 +68,7 @@
>> echo "ALT_BOOTDIR, BOOTDIR or JAVA_HOME needs to be defined to build 
>> SA"; \
>> exit 1; \
>> fi
>> - $(QUIETLY) if [ ! -f $(SA_CLASSPATH) ] ; then \
>> + $(QUIETLY) if [ ! -f $(SA_CLASSPATH) -a ! -d $(MODULELIB_PATH) ] ; 
>> then \
>> echo "Missing $(SA_CLASSPATH) file. Use 1.6.0 or later version of JDK";\
>> echo ""; \
>> exit 1; \
>>
>>
>> The logic before Mandy's change says:
>>
>> - if the $(SA_CLASSPATH) file doesn't exist, then complain and exit
>>
>> Mandy's changeset updated that logic to:
>>
>> - if the $(SA_CLASSPATH) file doesn't exist AND if the lib/modules
>> directory does not exist, then complain and exit
>>
>> Mandy also added a TODO note about needing to add a check to see
>> if the SA module is installed.
>>
>> So what does all this mean?
>>
>> - the modules logic that Mandy added is partial
>> - there is a TODO note about what else needs to be added
>> - if a proper boot JDK is provided, then the modules
>> logic that Mandy added is never executed
>> - if an improper boot JDK is provided AND the lib/modules
>> directory exists, then the error message and exit will
>> be skipped, but the build will fail later because the
>> JDI classes won't be found
>>
>> In short (ha - too late for that), I think the SA_CLASSPATH
>> logic is fine as it is. More modules changes will be needed
>> in future, but that's not relevant to this fix.
>>
>> Vladimir, are you OK with this fix as it is?
>>
>> Dan
>>
>>
>>
>> On 12/4/11 2:42 PM, Karen Kinnear wrote:
>>> 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.
>>>