If it's more "normal" to probe in the nothing case I would be fine with that.
Can you push this to jdk8/build? Your original commit, 8007129, hasn't made it to TL yet. Mike. On Jun 3 2013, at 01:10 , Erik Joelsson wrote: > > > On 2013-05-31 20:28, Mike Duigou wrote: >> 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 >> > This looks good to me and works as described, approved. > > Just noting that I would have expected this behavior (though I'm fine with > the way you did it too): > > --without-jtreg disables > --with-jtreg=no disables > (nothing) probe and set if found either through variable or > on path, but don't fail if not found. > --with-jtreg=path uses path > $JT_HOME --with-jtreg uses $JT_HOME location and fails if not found > (on PATH) --with-jtreg uses PATH location and fails if not found > > /Erik > >>> /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
