Magnus, Erik, thanks for your reviews, pushed w/ a newline being added at L#654 of make/Main.gmk.
Cheers, -- Igor > On Jun 16, 2020, at 7:03 AM, Magnus Ihse Bursie > <magnus.ihse.bur...@oracle.com> wrote: > > On 2020-06-16 15:06, Erik Joelsson wrote: >> >> On 2020-06-15 17:39, Igor Ignatyev wrote: >>> @David, Erik, Magnus, >>> >>> please find the answers to your comments at the bottom of this email. >>> >>> @all, >>> >>> David's and Erik's comments made me realize that some parts of the original >>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry >>> for the confusion and inconvenience. I also agree w/ David that it's hard >>> to follow/review, and my gaffe just made it worse, therefore I'd like to >>> start it over again from a clean sate >>> >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02>Build changes >>> look good to me now, except for a missing newline in Main.gmk. (No need for >>> new review.) >> > > Ditto. > > /Magnus >> >> /Erik >> >>>> 833 lines changed: 228 ins; 591 del; 14 mod; >>> >>> could you please review the patch which puts all tests for common >>> testlibrary classes (which is ones located in /test/lib) into one location >>> under /test/lib-test? please note this intentionally doesn't move all >>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are >>> left in /test/hotspot/jtreg/testlibrary_tests/ctw . >>> >>> to simplify review, the patch has been split into several webrevs, with >>> each of them accomplishes a more-or-less distinct thing, but is not >>> necessary self-contained: >>> >>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites >>> to test/lib-test: >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/> >>> >>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and >>> "merge" of hotspot's and jdk's OutputAnalyzerTest: >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/> >>> >>> 2. simplification of TestNativeProcessBuilder tests: converts Test class >>> used by TestNativeProcessBuilder into a static nested class: >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder >>> >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder> >>> >>> 3. makefiles changes to build native parts of lib-test tests. in past, >>> there were no tests w/ native parts in this test suite, >>> TestNativeProcessBuilder is the 1st such test; this part also removes now >>> unneeded lines from hotspot test suite makefile: >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest> >>> >>> 4. clean ups in hotspot test suite: adjusted location of >>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test >>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should >>> not be moved to /test/lib-test; removed >>> TestMutuallyExclusivePlatformPredicates from TEST.groups: >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/> >>> >>> 5. LingeredAppTest fix (which apparently has never been run before): >>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/ >>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/> >>> >>> 6. changes to make test/lib-test a fully supported and working test suite, >>> and add in into tier1, includes adding ProblemList, TEST.groups file, and >>> 'randomness' k/w: >>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/ >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/> >>> >>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 >>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977 >>> <https://bugs.openjdk.java.net/browse/JDK-8211977> >>> testing: >>> - make test TEST=tier1 locally on macosx >>> - test/lib-test on {windows,linux,macosx}-x64 >>> - tier1 job (in progress) >>> >>> Thanks, >>> -- Igor >>> >>> >>>> On Jun 14, 2020, at 11:23 PM, David Holmes <david.hol...@oracle.com >>>> <mailto:david.hol...@oracle.com>> wrote: >>>> <...> >>> >>>> This doesn't seem to move everything under >>>> test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory. >>> this is intentionally, ctw test-library is hotspot specific, hence its >>> tests are to reside in hotspot test suite (until we decide to move the >>> library to /test/lib), the same is true for SimpleClassFileLoadHookTest. >>>> >>>> You did not modify hotspot/jtreg/TEST.groups but it refers to: >>>> testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \ >>>> Plus it should probably refer to the new native_sanity tests somewhere. >>> the actual version of the patch removed reference to >>> TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups and had >>> TestNativeProcessBuilder moved to /test/test-lib, so no updates w.r.t. >>> native_sanity are needed >>> >>>> The newly added copyright notice needs to have the correct initial >>>> copyright year based on when the file was first added to the repo. >>> >>> /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 2020-04-16, >>> hence the added copyright has 2020 as the initial copyright year. >>> >>>> You used the wrong license header - makefiles don't use the classpath >>>> exception. >>> JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which also >>> have classpath exception. quick grep showed that make directory has 794 >>> files which has '"Classpath" exception', out of 850 which contain 'GNU >>> General Public License version 2' string. And although I agree that >>> makefiles shouldn't have the classpath exception, I'd prefer to keep >>> JtregNativeLibTest.gmk in sync w/ the its siblings and would leave it up to >>> build team to decide how it's best to handle. >>> >>> >>>> On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie >>>> <magnus.ihse.bur...@oracle.com <mailto:magnus.ihse.bur...@oracle.com>> >>>> wrote: >>>> >>>> A few comments: >>>> >>>> This seems like code copied from elsewhere: >>>> 57 # This evaluation is expensive and should only be done if this target >>>> was >>>> 58 # explicitly called. >>>> 59 ifneq ($(filter build-test-libtest-jtreg-native, $(MAKECMDGOALS)), ) >>>> I don't agree that this is an expensive evaluation. Furthermore, the >>>> makefile is only called for building the testlib and for making images, so >>>> in worst case it's just the image part that would get a penalty (although >>>> I highly doubt there is any). >>> right, JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk >>> which has this comment, I don't know enough details to say if it's >>> expensive or not, yet if you insist and there is a consensus within build >>> team, I can remove the comment. >>> >>>> >>>> 82 $(eval $(call SetupCopyFiles,COPY_LIBTEST_JTREG_NATIVE, \ >>>> Please use space after comma. >>> this again was preexisting in JtregNativeJdk.gmk, added the space >>> nevertheless. >>> >>> >>>> On Jun 15, 2020, at 6:16 AM, Erik Joelsson <erik.joels...@oracle.com >>>> <mailto:erik.joels...@oracle.com>> wrote: >>>> >>>> In JtretNativeLibTest.gmk, lines 51-55 should probably be removed (or >>>> adjusted if linking to libjvm is actually needed). >>> the lines were updated to define >>> BUILD_LIBTEST_JTREG_EXECUTABLES_LIBS_exejvm-test-launcher >>> >>> >
