RFR (S): 8006965: test_gamma should run with import JDK

[David Holmes]

On 26/02/2013 4:42 AM, Christian Thalinger wrote:
> On Feb 24, 2013, at 2:54 PM, David Holmes <David.Holmes at oracle.com> wrote:
>> On 23/02/2013 1:55 PM, Christian Thalinger wrote:
>>> I talked to a lot of people about this today.  What we really want is to not run tests when we build.  Mikael and I were looking into how we could do that without gamma and there is a way:
>>>
>>> http://cr.openjdk.java.net/~twisti/8006965/
>>>
>>> This would be the first of three fixes:
>>>
>>> Fix 1)  The patch above removes test_gamma and uses some weirdness in the VM (-Dsun.java.launcher=gamma) to run it with an existing JDK; add test_{product,fastdebug,debug} targets
>>
>> This logic is not suitable:
>>
>> 541 # Testing the built JVM
>> 542 ifeq ($(JAVA_HOME),)
>> 543   RUN_JVM=JAVA_HOME=$(JDK_IMPORT_PATH) $(JDK_IMPORT_PATH)/bin/java -d$(ARCH_DATA_MODEL) -server -XXaltjvm=$(MISC_DIR)/$(VM_FLAVOR) -Dsun.java.launcher=gamma
>> 544 else
>> 545   RUN_JVM=$(JAVA_HOME)/bin/java -d$(ARCH_DATA_MODEL) -server -XXaltjvm=$(MISC_DIR)/$(VM_FLAVOR) -Dsun.java.launcher=gamma
>> 546 endif
>>
>> I have JAVA_HOME set in my environment for use by other tools/scripts and it points at JDK7. The existing logic does not use my environments JAVA_HOME setting so neither should the revised logic!
>
> That's not entirely correct.  test_gamma uses your JAVA_HOME setting:

This is so confusing. Our makefiles are an abomination!

So this all started because the makefile has:

JAVA_HOME=$(ABS_BOOTDIR)

which was flagged as wrong because gamma would run in the boot JDK. But 
now it seems the make variable JAVA_HOME is irrelevant anyway because 
the test_gamma script will use the JAVA_HOME environment variable.

So how did the boot JDK come back into this???

> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ export JAVA_HOME=/java/re/jdk/8/latest/binaries/linux-x64
> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ ./test_gamma
> Using java runtime at: /java/re/jdk/8/latest/binaries/linux-x64/jre
> <snip>
> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ export JAVA_HOME=/foo
> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ ./test_gamma
> JAVA_HOME must point to a 64-bit OpenJDK.
>
> And here comes this little snippet into play:
>
> -MAKE_ARGS += JAVA_HOME=$(ABS_BOOTDIR)
> +MAKE_ARGS += BOOTDIR=$(ABS_BOOTDIR)
>
> Only setting JAVA_HOME to ABS_BOOTDIR make test_gamma work even if you have a JAVA_HOME set.

I still don't get this. What has BOOTDIR got to do with JAVA_HOME? Where 
is this BOOTDIR value being used? There is no use of it in 
http://cr.openjdk.java.net/~twisti/8006965/8006965.patch and I don't see 
it pre-existing ??

Thanks,
David

> I have added this logic so that users can control what JDK is used when running the test.  In fact they should use ALT_JDK_IMPORT_PATH if they want to control that.
>
>>
>> I also don't see why the above sets JAVA_HOME at #543 - what will read that environment variable?
>
> It's the odd logic in os::jvm_path guarded by Arguments::created_by_gamma_launcher().  A clean-up of that logic would be part of Fix 3.
>
>>
>> I still have concerns over what JDK_IMPORT_PATH will point to for different JDK builders.
>
> It's either JDK_IMPORT_PATH or JDK_IMAGE_DIR.  Since most people don't want to export their libjvm into a JDK we have to use JDK_IMPORT_PATH.  We could add some logic that checks if JDK_IMAGE_DIR exists and use that one.
>
> -- Chris
>
>>
>> And this addition still makes no sense to me:
>>
>> MAKE_ARGS += BOOTDIR=$(ABS_BOOTDIR)
>>
>> Why do you need to add BOOTDIR to the MAKE_ARGS?
>>
>> David
>>
>>
>>> Fix 2)  Remove gamma and all the ugly code that comes with it (copies of the jdk launcher in hotspot and other pieces); make the hotspot script work like the test targets in Fix 1
>>>
>>> Fix 3)  Remove the -Dsun.java.launcher=gamma and possibly replace the existing -Dsun.boot.library.path weirdness by explicit command line options like -Xbootlibrarypath:{/p,/a}
>>>
>>> -- Chris
>>>
>>> On Feb 22, 2013, at 3:21 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>
>>>>
>>>> On Feb 22, 2013, at 12:58 AM, Staffan Larsen <staffan.larsen at oracle.com> wrote:
>>>>
>>>>> I'm not sure what the correct solution is, but when you do find out, the jdkpath.sh target should also be updated.
>>>>
>>>> How many are actually using the hotspot script?  Would people be very sentimental if we would remove the gamma launcher altogether?
>>>>
>>>> Taking to people here it seems that most are copying their libjvm into a JDK and use java anyway.
>>>>
>>>> -- Chris
>>>>
>>>>>
>>>>> Thanks,
>>>>> /Staffan
>>>>>
>>>>> On 22 feb 2013, at 03:40, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>
>>>>>> http://cr.openjdk.java.net/~twisti/8006965
>>>>>>
>>>>>> 8006965: test_gamma should run with import JDK
>>>>>> Reviewed-by:
>>>>>>
>>>>>> Right now test_gamma runs with the boot JDK which is JDK n-1 (where
>>>>>> JDK n is the version we are actually compiling for).  This setup is
>>>>>> unsupported and thus should not be done during HotSpot builds.
>>>>>>
>>>>>> The fix is to always use JDK_IMPORT_PATH instead of JAVA_HOME when
>>>>>> running test_gamma.
>>>>>>
>>>>>> make/bsd/makefiles/buildtree.make
>>>>>> make/defs.make
>>>>>> make/linux/makefiles/buildtree.make
>>>>>> make/solaris/makefiles/buildtree.make
>>>>>>
>>>>>
>>>>
>>>
>