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

Tue Mar 19 18:22:48 PDT 2013

What is the final webrev?   Does this include "hotspot" fix to not use 
the launcher in the vm repository but still allow debugging with -gdb?   
Does TEST_IN_BUILD go away?
thanks,
Coleen

On 3/19/2013 9:16 PM, Christian Thalinger wrote:
> On Mar 13, 2013, at 4:44 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>
>> On Feb 26, 2013, at 6:09 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>
>>> On Feb 25, 2013, at 11:24 PM, David Holmes <David.Holmes at oracle.com> wrote:
>>>
>>>> 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!
>>> I couldn't agree more.
>>>
>>>> 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?
>> Ha!  I can remember.  It's used by jmvti.make (implicitly).  The logic in rules.make is:
>>
>> ifdef ALT_BOOTDIR
>>
>> COMPILE.JAVAC = $(ALT_BOOTDIR)/bin/javac
>>
>> else
>>
>> ifdef BOOTDIR
>>
>> COMPILE.JAVAC = $(BOOTDIR)/bin/javac
>>
>> else
>>
>> ifdef JAVA_HOME
>> else
>>
>> endif
>> endif
>> endif
>>
>> BOOTDIR is not defined in rules.make and JAVA_HOME is picked up (which is set to ABS_BOOTDIR).
> Back to my favorite review these days :-)  I put the BOOTDIR setting back because we need it.
>
> Any reviewers who approve or veto pushing this change?
>
> -- Chris
>
>> This all sucks and needs to be replaced by something completely different.  Soon.
>>
>> -- Chris
>>
>>>> There is no use of it in http://cr.openjdk.java.net/~twisti/8006965/8006965.patch and I don't see it pre-existing ??
>>> I talked to John Coomes about that yesterday and we can remove that line.  ABS_BOOTDIR is only used by Windows.
>>>
>>> -- Chris
>>>
>>>> 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
>>>>>>>>>>