Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-09 Thread Philip Race
+1 -phil On 12/9/19, 6:33 AM, Andy Herrick wrote: Please review this revised fix at [3]. This fix only adds "@modules jdk.incubator.jpackage" to junit.jar. [3] http://cr.openjdk.java.net/~herrick/8235453/webrev.02/ /Andy On 12/6/2019 1:33 PM, Andy Herrick wrote: Please review this

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-09 Thread Andy Herrick
Please review this revised fix at [3]. This fix only adds "@modules jdk.incubator.jpackage" to junit.jar. [3] http://cr.openjdk.java.net/~herrick/8235453/webrev.02/ /Andy On 12/6/2019 1:33 PM, Andy Herrick wrote: Please review this jpackager test fix for bug [1] at [2]. the fix adds

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-07 Thread Phil Race
Ok. That makes sense. I was under the impression other tests were failing. So only the one update is needed for this and it is arguably just an omission. jpackage has a couple of types of requirement 1) run only on platforms that support jpackage - the @modiules will do this. 2) run only on a

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-07 Thread Igor Ignatev
As far as I can see only junit.java test is executed on Solaris and is the only one failing. jtreg uses @modules during test selection/filtering phase, so tests which have @modules A won’t be run on jdk which doesn’t have module A, hence it should be sufficient. If it’s not, we have a bug in

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-07 Thread Phil Race
All these tests specify this already so it doesn’t seem sufficient. -Phil. > On Dec 7, 2019, at 12:07 PM, Igor Ignatyev wrote: > > can we just add '@modules jdk.incubator.jpackage' to > test/jdk/tools/jpackage/junit/junit.java to solve that? or some of the tests > run by

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-07 Thread Igor Ignatyev
can we just add '@modules jdk.incubator.jpackage' to test/jdk/tools/jpackage/junit/junit.java to solve that? or some of the tests run by test/jdk/tools/jpackage/junit/junit.java don't need jdk.incubator.jpackage module? -- Igor > On Dec 7, 2019, at 11:57 AM, Philip Race wrote: > > Yes,

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-07 Thread Philip Race
Yes, since only a (relatively) small number of tests needed to be updated this is fine with me at least for now. So +1 -phil. On 12/7/19, 5:48 AM, Andy Herrick wrote: Phil - are you approving this change ? - I think you are the only registered Reviewer. /Andy On 12/6/2019 8:11 PM, Phil Race

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-07 Thread Andy Herrick
Phil - are you approving this change ? - I think you are the only registered Reviewer. /Andy On 12/6/2019 8:11 PM, Phil Race wrote: Well we could deprecate and remove the solaris port :-) But until that is done this is the only way I know of. we could require all jpackage tests to include

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-06 Thread Phil Race
Well we could deprecate and remove the solaris port :-) But until that is done this is the only way I know of. we could require all jpackage tests to include some helper code which decides if it is applicable but that will be more work upfront and many jpackage tests are already platform

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-06 Thread Alexander Matveev
Looks good, but is there better way to exclude tests on Solaris? I do not like idea adding @requires for all tests. Thanks, Alexander On 12/6/2019 10:35 AM, Alexey Semenyuk wrote: Looks good. - Alexey On 12/6/2019 1:33 PM, Andy Herrick wrote: Please review this jpackager test fix for bug

Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-06 Thread Alexey Semenyuk
Looks good. - Alexey On 12/6/2019 1:33 PM, Andy Herrick wrote: Please review this jpackager test fix for bug [1] at [2]. the fix adds "@requires (os.family == "linux") | (os.family == "mac") | (os.family == "windows")" to all jpackage tests that were not already filtered with "@requires

RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"

2019-12-06 Thread Andy Herrick
Please review this jpackager test fix for bug [1] at [2]. the fix adds "@requires (os.family == "linux") | (os.family == "mac") | (os.family == "windows")" to all jpackage tests that were not already filtered with "@requires (os.family == "XXX")" [1]