> On 19 Jun 2018, at 23:00, Erik Joelsson <erik.joels...@oracle.com> 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?)
Yes, the updategraalinopenjdk command[1] can be modified in anyway seen fit to
make Graal a better citizen when copied to JDK. It would also be ok to add the
jtreg commands in test source headers.
-Doug
[1]
https://github.com/oracle/graal/blob/27288e546392f44ebf8107795647e0db155faf38/compiler/mx.compiler/mx_compiler.py#L1161
>
> 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.
>
