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