Code Review Request for MacOS X build change (7117748)

[Vladimir Kozlov]

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.
>>