RFR : 8015510 : (s) Improve JTReg location detection and provide location to test/Makefile

[Mike Duigou]

On May 31 2013, at 07:14 , Erik Joelsson wrote:

> 
> 
> On 2013-05-31 15:47, Mike Duigou wrote:
>> On May 31 2013, at 01:06 , Erik Joelsson wrote:
>> 
>>> 
>>> On 2013-05-30 20:05, Mike Duigou wrote:
>>>> Thank you for the feedback. I have updated the webrev :
>>>> 
>>>> http://cr.openjdk.java.net/~mduigou/JDK-8015510/1/webrev/
>>>> 
>>>> It's a lot simpler now.
>>> Now my concern is that configure will fail without either specifying --without-jtreg or having it available. I doubt RE has it available on their build machines and it's not a requirement to just build. It would be better if it was optional by default and only required if you specified --with-jtreg.
>> It is optional. Not specifying anything is the same as specifying "--without-jtreg".
>> 
>> --without-jtreg            disables
>> --with-jtreg=no            disables
>> (nothing)                  disables
>> --with-jtreg=path          uses path
>> $JT_HOME --with-jtreg      uses $JT_HOME location
>> (on PATH) --with-jtreg     uses PATH location
>> 
> I tried applying your patch and ran just "bash configure" and it failed on missing jtreg, so i think you might have missed something in the logic for the default case.

One more try then...

http://cr.openjdk.java.net/~mduigou/JDK-8015510/2/webrev/

I added a default for absent in the AC_ARG_WITH

Thanks,

Mike


> /Erik
>>>> The one part I couldn't make perfect was the check
>>>> 
>>>>       if test ! -f "$JTREGEXE"; then
>>>>         AC_MSG_ERROR([JTReg executable does not exist: $JTREGEXE])
>>>>       fi
>>>> 
>>>> which only checks for whether the executable exists and not whether it is executable. I understand the "-x" text condition can't be used because it doesn't work on windows. Alternatives?
>>> You could try executing it with a known parameter that returns 0 and outputs something known (-version or similar), and see that it gives the expected result, but I wouldn't go that far.
>> OK, I will leave it as is then I think this patch is complete then.
>> 
>>> /Erik
>>>> Mike
>>>> 
>>>> On May 29 2013, at 01:06 , Erik Joelsson wrote:
>>>> 
>>>>> On 2013-05-29 00:30, Mike Duigou wrote:
>>>>>> Hello all;
>>>>>> 
>>>>>> As a follow on to JDK-8007129 recently pushed to the jdk8/build repo I've improved the detection logic for finding jtreg. In addition to using the provided path it will also try the location specified by JT_HOME and in the command path.
>>>>>> 
>>>>>> Attention people who just let magic happen and don't pay attention until something breaks: Currently if no location is defined for JT_HOME a default location of $(SLASH_JAVA)/re/jtreg/4.1/promoted/latest/binaries/jtreg is used. SLASH_JAVA is either /java on posix platforms or J:\\ on windows. This default location still works for now, but future changes will almost certainly remove it. You can avoid this pain by adding an appropriate --with-jtreg to your configure command today.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~mduigou/JDK-8015510/0/webrev/
>>>>> When trying to find a program on the path, please use the builtin AC_PATH_PROG or similar macro. Using "which" is bad as it works differently on all platforms and we have worked hard to remove its use in configure. It looks like at that point, JTREGEXE is required to be found, then you can use BASIC_REQUIRE_PROG which also fails with a suitable error message.
>>>>> 
>>>>> There is no need to repeat AC_SUBST calls and making them conditionals has no effect to my knowledge. I would move them out of the if statements and put them at the end of the block.
>>>>> 
>>>>> If JT_HOME is set in the environment, it is just accepted and not validated. Should it be?
>>>>>> If "--without-jtreg", "--with-jtreg=no" or no jtreg directive is used on the command line the spec.gmk vars JT_HOME and JTREGEXE will still present but will be empty. I am not sure if it's better (or possible) to avoid defining them in this case.
>>>>>> 
>>>>> If you want to have it undefined, you can achieve it with the following pattern (look at OPENJDK for an example):
>>>>> In .m4: SET_JT_HOME="JT_HOME=$JT_HOME"
>>>>> In spec.gmk.in: @SET_JT_HOME@
>>>>> 
>>>>> Keeping it undefined when not set would let the makefiles pick it up from the environment, a pattern the new build is trying to break.
>>>>> 
>>>>> /Erik
>>>>>> Also, since test/Makefile doesn't yet read spec.gmk it is necessary to provide it the JT_HOME location via an environment var in Main.gmk. This will eventually be fixed.
>>>>>> 
>>>>>> Rather than wait for 8007129 to propagate to TL I would like someone to sponsor this change into build repo.
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>>> Mike