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.
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.
FLATTEN has no effect in the SetupCopyFiles call since all the jar files
found by wildcard can only be in one directory anyway.
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.
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 ))
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.
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.
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.
/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.
