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.