Hi Felix, none of the jdk tests which fail w/ NCDFE: jdk/test/lib/process/StreamPumper depend on StreamPumper directly, they get this dependency transitively from jdk/test/lib/process/ProcessTools, so I don't see how you will find this good definition of "explicit" even for the failures at hand.
I recommend to work around this the same way we did it in hotspot, which reliably removed almost all our NCDFE failures, -- remove explicit @build, if not all for all classes, then at least for jdk/test/lib/** classes. -- Igor > On Jun 1, 2017, at 6:52 PM, Felix Yang <felix.y...@oracle.com> wrote: > > Hi Igor and Ioi, > > I partially agree with you. As initially stated in the proposal and > bug(JDK-8181299 <https://bugs.openjdk.java.net/browse/JDK-8181299>), I don't > think this patch is a fix but a quick workaround to make them runnable. > > "explicit" is reasonable for me. But "explicit" should not be restricted > as "explicit all, including dependencies", as it is not productive or even > realistic in the long term. > Thanks, > Felix > On 2017/6/2 7:58, Igor Ignatyev wrote: >>> For example: doing this may be enough for now: >>> >>> * @build jdk.test.lib.process.* >>> >>> But what if in the future, jdk.test.lib.process is restructured to have a >>> private package jdk.test.lib.process.hidden? To work around >>> CODETOOLS-7901986, all the test cases that must be modified to the >>> following, which unnecessarily exposes library implementation details to >>> the library users: >>> >>> * @build jdk.test.lib.process.* jdk.test.lib.process.hidden.* >> >> and in fact, there is already similar problem and >> http://cr.openjdk.java.net/~xiaofeya/8181299/webrev.01/ >> <http://cr.openjdk.java.net/%7Exiaofeya/8181299/webrev.01/> does not address >> it. >> jdk/test/lib/process/ProcessTools depends on jdk/test/lib/Utils so all tests >> which have '@build jdk.test.lib.process.ProcessTools' will have to have >> '@build jdk.test.lib.Utils'. then we have OutputAnalyzer which depends on >> ProcessTools so all tests which @build jdk.test.lib.process.OutputAnalyzer >> will @build ProcessTools and Utils explicitly. many testlibrary classes >> which on jdk.test.lib.process.OutputAnalyzer, so one will have to specify >> OutputAnalyzer ProcessTools and Utils in the tests which depends on other >> testlibrary classes. to make things even worse, Utils depends on >> OutputAnalyzer and there are lots of tests and test library classes which >> depend on Utils, so all of them will have to have at least '@build >> jdk.test.lib.Utils jdk.test.lib.process.OutputAnalyzer >> jdk.test.lib.process.ProcessTools'. and they will work stable till someone >> refactors them and extract some new classes. that is to say, it's nearly >> impossible to have all explicit @build actions. >> >> Cheers, >> -- Igor >> >>> On Jun 1, 2017, at 3:37 PM, Ioi Lam <ioi....@oracle.com >>> <mailto:ioi....@oracle.com>> wrote: >>> >>> >>> >>> On 6/1/17 1:17 PM, Igor Ignatyev wrote: >>>>> On Jun 1, 2017, at 1:20 AM, Chris Hegarty <chris.hega...@oracle.com >>>>> <mailto:chris.hega...@oracle.com>> wrote: >>>>> >>>>> Igor, >>>>> >>>>>> On 1 Jun 2017, at 04:32, Igor Ignatyev <igor.ignat...@oracle.com >>>>>> <mailto:igor.ignat...@oracle.com>> wrote: >>>>>> >>>>>> Hi Felix, >>>>>> >>>>>> I have suggested the exact opposite change[1-3] to fix the same problem. >>>>> I’m sorry, but this is all just too confusing. After your change, who, or >>>>> what, is >>>>> responsible for building/compiling the test library dependencies? >>>> jtreg is responsible, there is an implicit build for each @run, and jtreg >>>> will analyze a test class to get transitive closure for static >>>> dependencies, hence you have to have @build only for classes which are not >>>> in constant pool, e.g. used only by reflection or whose classnames are >>>> only used to spawn a new java instance. >>> >>> >>> I suspect the problem is caused by a long standing bug in jtreg that >>> results in library classes being partially compiled. Please see my >>> evaluation in >>> >>> https://bugs.openjdk.java.net/browse/CODETOOLS-7901986 >>> <https://bugs.openjdk.java.net/browse/CODETOOLS-7901986> >>> >>> In the bug report, there is test case that can reliably reproduce the >>> NoClassDefFoundError problem. >>> >>> I think adding all the @build commands in the tests are just band-aids. >>> Things will break unless every test explicitly uses @build to build every >>> class in every library that they use, including all the private classes >>> that are not directly accessible by the test cases. >>> >>> For example: doing this may be enough for now: >>> >>> * @build jdk.test.lib.process.* >>> >>> But what if in the future, jdk.test.lib.process is restructured to have a >>> private package jdk.test.lib.process.hidden? To work around >>> CODETOOLS-7901986, all the test cases that must be modified to the >>> following, which unnecessarily exposes library implementation details to >>> the library users: >>> >>> * @build jdk.test.lib.process.* jdk.test.lib.process.hidden.* >>> >>> Just imagine this -- "in order to use malloc() you must explicitly build >>> not only malloc(), but also sbrk() ... and every other function in libc". >>> That seems unreasonable to me. >>> >>> By the way, we made a fix in the HotSpot tests (see >>> https://bugs.openjdk.java.net/browse/JDK-8157957 >>> <https://bugs.openjdk.java.net/browse/JDK-8157957>) that got rid of many >>> (but not all) of the NoClassDefFoundErrors by *removing* the @build lines >>> ..... >>> >>> My proposal is, instead of just adding @build for band-aid, we should fix >>> CODETOOLS-7901986 instead. >>> >>> Thanks >>> - Ioi >>> >>> >>>>> >>>>> Test library code has no @modules tags, so does not explicitly declare its >>>>> module dependencies. Instead module dependencies, required by test >>>>> library code, are declared in the test using the library. If we wildcard, >>>>> or >>>>> otherwise leave broad build dependencies, from tests then there is no >>>>> way to know what new module dependencies may be added in the future. >>>>> That is, one of, the reason(s) I asked Felix to be explicit about the >>>>> build >>>>> dependencies. >>>> having explicit builds does not really help w/ module dependency, if >>>> someone change a testlibrary class so it starts to depend on another >>>> testlibrary class, jtreg will implicitly build it and if this class has >>>> some module dependencies, you will have to reflect them in the test. >>>> >>>> generally speaking, I don't like having explicit build actions because >>>> build actions themselves are implicit, so they don't really help, it's >>>> still will be hard to spot missed explicit builds. not having (unneeded) >>>> explicit builds is an easy rule to follow and we can easily find all >>>> places which don't follow this rule by grep. >>>> >>>> -- Igor >>>>> -Chris. >>>>> >>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8181391 >>>>>> <https://bugs.openjdk.java.net/browse/JDK-8181391> >>>>>> [2] >>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-June/048012.html >>>>>> >>>>>> <http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-June/048012.html> >>>>>> [3] http://cr.openjdk.java.net/~iignatyev//8181391/webrev.00/index.html >>>>>> <http://cr.openjdk.java.net/%7Eiignatyev//8181391/webrev.00/index.html> >
