On 2018-06-27 00:29, Ekaterina Pavlova wrote:
well, INCLUDE_GRAAL is not defined at the time we run tests.
I can try to guard by something like
ifeq ($(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU), $(filter
$(OPENJDK_TARGET_OS)-$(OPENJDK_TARGET_CPU),linux-x64 macosx-x64
windows-x64))
but I am not sure we should proceed this way. It looks too much
complicated.
It is safe to pass -e:TEST_IMAGE_GRAAL_DIR to jtreg even it will not
be used.
We also do similar way for example for -e:JDK8_HOME
I agree, no need to guard the addition of the env variable pass through.
I have uploaded new webrev at the same location:
http://cr.openjdk.java.net/~epavlova//8205207/webrev.01/index.html
Looks good to me now. (Magnus is on vacation so you probably don't want
to wait for him to respond)
/Erik
thanks for reviewing,
-katya
/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.
