> 23 juni 2018 kl. 00:22 skrev Ekaterina Pavlova <ekaterina.pavl...@oracle.com>: > > Fixed and regenerated webrev at the same location: > http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
Better, but not done yet. In JtregGraalUnit.gmk: line 52-53 should be on a single line. The SRC list for BUILD_VM_COMPILER_TESTS looks insane, but the root cause here is the weird layout on disk. But the .test part really surprises me, I thought we had a clear rule to have no test source in src?! In lib-tests.m4: Copy paste error: AC_MSG_ERROR([You must specify the path to tonga jar]) The addition in RunTests.gmk and TestCommon.gmk should be guarded by: ifeq ($(INCLUDE_GRAAL), true) /Magnus > > Erik, > thanks again for your detailed reviews! > > -katya > >> On 6/22/18 2:38 PM, Erik Joelsson wrote: >> Hello Katya, >> This looks much better, thanks! >> A few suggestions still: >> Main.gmk: instead of repeating the assignment in both if and else block: >> ifeq ($(INCLUDE_GRAAL), true) >> JVM_TEST_IMAGE_TARGETS += test-image-hotspot-jtreg-graal >> endif >> I think it's fine to do that without the ?= because an alternative JVM >> implementation is unlikely to be using graal anyway. >> In JtregGraalUnit.gmk: Some minor style nits: >> 54: align )) with first eval line as you have done with the other eval calls >> 118: Since 117 is a one line parameter, please move comma to 117 >> 133: Same as for 118 >> 130-132: Please indent 4 spaces for continuation >> /Erik >>> On 2018-06-22 14:16, Ekaterina Pavlova wrote: >>> Erik, Doug, >>> >>> thank you a lot for your reviews and advises. >>> I fixed everything what Erik has pointed out, please see my answers inlined. >>> As about moving more staff in 'updategraalinopenjdk' can we consider this >>> as next step? >>> I am not quite familiar with 'updategraalinopenjdk' and didn't contribute >>> into Graal ws yet, >>> so I will prefer to do this improvement/refactoring as part of separate JDK >>> issue. >>> >>> New webrev is here: >>> webrev: http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 >>> testing: tested by building and running graal unit tests >>> >>> >>>> On 6/19/18 2:00 PM, Erik Joelsson wrote: >>>> Hello, >>>> >>>> Please always include build-dev when reviewing build changes. >>>> >>>> In general this looks pretty good but there are some details that need >>>> fixing. >>>> >>>> (Aside from this particular review, I must state my reservation against >>>> all the special treatment graal is enjoying from a source and test layout >>>> and build perspective. My understanding here is that someone will have to >>>> regularly update the wrapper jtreg tests manually using the script. This >>>> in addition to having to implement this very convoluted redirection logic >>>> because the tests are in the wrong place. Wouldn't it make a lot more >>>> sense to put the complicated logic in the import procedure for graal >>>> source so that it would fit nicely into the OpenJDK source/build/test >>>> model, instead of adding more and more complicated workarounds in the >>>> OpenJDK build and test procedures?) >>>> >>>>> On 2018-06-18 22:26, Ekaterina Pavlova wrote: >>>>> Hi All, >>>>> >>>>> please review porting of Graal unit tests under jtreg. The main idea of >>>>> this porting is to keep Graal unit tests as is >>>>> (located in src/jdk.internal.vm.compiler/share/classes/*.test) and run >>>>> them similar way they are run in Graal project. >>>>> To achieve this >>>>> test/hotspot/jtreg/compiler/graalunit/common/GraalUnitTestLauncher.java >>>>> helper class has been written >>>>> which simply launches com.oracle.mxtool.junit.MxJUnitWrapper to run >>>>> specified set of Graal unit tests. The groups of >>>>> Graal unit tests to run are defined in auto-generated >>>>> test/hotspot/jtreg/compiler/graalunit/*.java jtreg tests. >>>>> >>>>> New make/test/JtregGraalUnit.gmk is used to build Graal unit tests into >>>>> jdk.vm.compiler.tests.jar and then install >>>>> it in $(TEST_IMAGE_DIR)/hotspot/jtreg/graal/. >>>>> >>>> GRAALUNIT_LIB is never defined (in the open). Since this is used in open >>>> makefiles, we need an open configure option to set it. >>> >>> ok, I created open/make/autoconf/lib-tests.m4 and did put Graal related >>> staff there. >>> >>>> To make things clearer, I would rename LIB_DIR to something like >>>> LIB_OUTPUTDIR to signal that this is a location to put files in, rather >>>> than read them from. >>> >>> ok, renamed >>> >>>> >>>> FLATTEN has no effect in the SetupCopyFiles call since all the jar files >>>> found by wildcard can only be in one directory anyway. >>> >>> ok, removed >>> >>>> BUILDTOOLS_OUTPUTDIR is meant for tools used during the build. Tests >>>> classes and jars should be built into $(SUPPORT_OUTPUTDIR)/test/..> Please >>>> create a suitable sub directory there for the output from this makefile. >>> >>> ok, introduced COMPILE_OUTPUTDIR := $(SUPPORT_OUTPUTDIR)/test/graalunit to >>> be used to build graal unit test classes. >>> >>>> The all and test-image targets are never called so no need to declare them. >>>> >>>> There are some style issues too. (Please see >>>> http://openjdk.java.net/groups/build/doc/code-conventions.html for >>>> details.) >>>> * 43 logic indent 2 spaces >>>> * 52 we like to put the ending )) on a new line with ', \' at the end of >>>> the line before >>>> * 58 continuation indent 4 spaces >>>> * 88, 89 please break long lines >>>> * 90 continuation indent 4 spaces >>>> * 99 continuation indent 4 spaces >>>> * 120 break before )) >>> >>> I think I fixed everything >>> >>>>> make/Main.gmk adds proper dependencies for build-test-hotspot-jtreg-graal >>>>> and test-image-hotspot-jtreg-graal. >>>>> >>>> The target build-test-hotspot-jtreg-graal only needs to depend on >>>> exploded-image-optimize. >>> >>> fixed >>> >>>> The new graal specific targets should be guarded by INCLUDE_GRAAL in >>>> Main.gmk. Otherwise those targets will silently do nothing if invoked >>>> without graal. That means adding them to JVM_TEST_IMAGE_TARGETS needs to >>>> be conditional too. >>> >>> fixed >>> >>>>> test/TestCommon.gmk passes TEST_IMAGE_GRAAL_DIR to jtreg so jtreg knows >>>>> where to find Graal tests and libs. >>>>> >>>> This needs to be duplicated for make/RunTest.gmk so that these tests work >>>> with "make run-test" as well. >>> >>> added >>> >>> thanks, >>> -katya >>> >>>> /Erik >>>>> test/hotspot/jtreg/compiler/graalunit/TestPackages.txt file defines >>>>> "testName -> testPrefix [requiresStatement]" mapping >>>>> which is used by test/hotspot/jtreg/compiler/graalunit/generateTests.sh >>>>> script to generate jtreg tests. >>>>> >>>>> test/hotspot/jtreg/compiler/graalunit/com.oracle.mxtool.junit was ported >>>>> from mx project without any changes. >>>>> >>>>> >>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8205207 >>>>> webrev: >>>>> http://cr.openjdk.java.net/~epavlova//8205207/webrev.00/index.html >>>>> testing: new tests were executed as part of automatic HS testing for >>>>> several months. >>>>> >>>>> >>>>> thanks, >>>>> -katya >>>>> >>>>> p.s. >>>>> Igor Ignatyev volunteered to sponsor this change. >>>> >>> >
