Re: [EXTERNAL] JDK-8234076 bug fix candidate
Hi,
The launcher fix looks good, despite the launcher fix being simple, please
note,
it sometimes requires a lot more effort to write a test for it, please read
on
Henry excellent catch wrt. isTerminalOp the launcher!!!, that tickled my
memory and I recalled
this change for VM's native memory tracking.
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/37d1442d53bc/test/tools/launcher/TestSpecialArgs.java#l243
Inspecting the above changeset, we might have to add another test for JVM
native memory tracking,
since this is a terminal argument.
Maybe TestHelper could create a simple module, which prints its args,
http://hg.openjdk.java.net/jdk/jdk/file/31882abe1494/test/jdk/tools/launcher/TestHelper.java
Kumar Srinivasan
On Fri, Dec 6, 2019 at 2:44 PM Henry Jen wrote:
> Thanks for the work, the test case covers that when —module= is used
> before other options, which is good.
>
> But we need another to make sure that when —module= is put in an argument
> file or environment variable, that should not be allowed. The fix is
> simple, we need to add that to isTerminalOp() method.
>
> Cheers,
> Henry
>
> > On Dec 6, 2019, at 12:28 PM, Nikola Grcevski <
> nikola.grcev...@microsoft.com> wrote:
> >
> > Thank you Henry!
> >
> > I have modified the fix in checkArg to use JLI_StrCCmp as suggested
> below and I have extended the BasicTest.java (in
> test/jdk/tools/launcher/modules/basic) with few additional tests.
> >
> > I don't have author status yet, therefore I'm attaching the patch right
> after my signature. I also updated my external webrev if that's easier to
> review (https://grcevski.github.io/JDK-8234076/webrev/)
> >
> > Thanks again to everyone for your help with this bug. If there are any
> additional comments or suggestions please let me know.
> > Nikola
> >
> > diff -r bd436284147d src/java.base/share/native/libjli/args.c
> > --- a/src/java.base/share/native/libjli/args.cWed Nov 20
> 08:12:14 2019 +0800
> > +++ b/src/java.base/share/native/libjli/args.cFri Dec 06
> 15:11:36 2019 -0500
> > @@ -130,6 +130,8 @@
> > }
> > } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
> > stopExpansion = JNI_TRUE;
> > +} else if (JLI_StrCCmp(arg, "--module=") == 0) {
> > +idx = argsCount;
> > }
> > } else {
> > if (!expectingNoDashArg) {
> > diff -r bd436284147d src/java.base/windows/native/libjli/java_md.c
> > --- a/src/java.base/windows/native/libjli/java_md.c Wed Nov 20
> 08:12:14 2019 +0800
> > +++ b/src/java.base/windows/native/libjli/java_md.c Fri Dec 06
> 15:11:36 2019 -0500
> > @@ -34,6 +34,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include "java.h"
> > @@ -1015,6 +1016,17 @@
> >
> > // sanity check, match the args we have, to the holy grail
> > idx = JLI_GetAppArgIndex();
> > +
> > +// First arg index is NOT_FOUND
> > +if (idx < 0) {
> > +// The only allowed value should be NOT_FOUND (-1) unless
> another change introduces
> > +// a different negative index
> > +assert (idx == -1);
> > +JLI_TraceLauncher("Warning: first app arg index not found,
> %d\n", idx);
> > +JLI_TraceLauncher("passing arguments as-is.\n");
> > +return NewPlatformStringArray(env, strv, argc);
> > +}
> > +
> > isTool = (idx == 0);
> > if (isTool) { idx++; } // skip tool name
> > JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx,
> stdargs[idx].arg);
> > diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
> > --- a/test/jdk/tools/launcher/modules/basic/BasicTest.javaWed Nov 20
> 08:12:14 2019 +0800
> > +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.javaFri Dec 06
> 15:11:36 2019 -0500
> > @@ -29,6 +29,7 @@
> > * jdk.jlink
> > * @build BasicTest jdk.test.lib.compiler.CompilerUtils
> > * @run testng BasicTest
> > + * @bug 8234076
> > * @summary Basic test of starting an application as a module
> > */
> >
> > @@ -40,6 +41,8 @@
> >
> > import jdk.test.lib.compiler.CompilerUtils;
> > import jdk.test.lib.process.ProcessTools;
> > +import jdk.test.lib.process.OutputAnalyzer;
> > +import jdk.test.lib.Utils;
> >
> > import org.testng.annotations.BeforeTest;
> > import org.testng.annotations.Test;
> > @@ -70,6 +73,8 @@
> > // the module main class
> > private static final String MAIN_CLASS = "jdk.test.Main";
> >
> > +// for Windows specific launcher tests
> > +static final boolean IS_WINDOWS = System.getProperty("os.name",
> "unknown").startsWith("Windows");
> >
> > @BeforeTest
> > public void compileTestModule() throws Exception {
> > @@ -259,4 +264,87 @@
> > assertTrue(exitValue != 0);
> > }
> >
> > +
> > +/**
> > + * Helper method that creates a ProcessBuilder with command line
> arguments
> > + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
> > + */
> > +private ProcessBuilder
Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)
Another update: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3/ And a delta of the changes since last version (v2) here: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3_delta/ The javadoc has been updated inline here: http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html Summary of changes: * fixed some (cosmetic) issues in FileChannelmpl following offline discussion with Alan * changed tests group definition, so that now jdk_foreign is part of tier1_part3 * updated javadoc in various places to fix code samples (as per Paul comments) * updated javadoc in MemoryHandles to talk about access modes (as per Paul comments) - I added a new section on top, and then referred to in from all relevant places * some changes to use Objects.hash (as per Paul comments), and some minor refactor in the AddreddGenerator (to use switch expression) * tightened javadoc for MemoryAddress::copy - the method now is specified to throw IAE if the range of source/dest addresses overlap - I've fixed the impl and added a test (as per Raffaello comments) * tightened byte buffer VarHandle view - if the view is created from a byte buffer obtained from a segment (!!!) we should do a segment check - added tests And here's a list of the pending API-related issues. Since these are IMHO minor issues, I suggest we defer them to a followup minor cleanup, so that we can move ahead with finalization of the CSR with the current API. Here's the list: * Should MemoryAddress implement Comparable? I think the answer is "probably not" - an address doesn't have a 'length' so you can't really do a range comparison (maybe memory segments though?). For now, users can project to byte buffer and take it from there (since ByteBuffer <: Comparable) * should we have a way to ask a Layout if its size is specified ? (Paul) * MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much distance between these two semantically different operations (Paul suggested using 'add' - which I'm not enthusiastic about because it's not adding two addresses - it's adding a long to an address...) * MemorySegment::isAccessible() -- as the A* word is overloaded, some other name should be found? * MemorySegment::acquire() -- replace with MemorySegment::fork? Thanks Maurizio On 06/12/2019 10:43, Maurizio Cimadamore wrote: Hi, here's an updated version of the patch: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2/ And a delta of the changes since last version here: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2_delta/ The javadoc has been updated inline here: http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html Summary of changes: * fixed spurious protected methods in AbstractLayout and subclasses which leaked into API * removed library_call.cpp changes, as these are being tracked separately by Vlad * compacted ILOAD code in AddressVarHandleGenerator * replaced uses of ++i/--i with i + 1/i - 1 in MemoryScope I have made no changes to the *name* of the methods in the API. In fact, I suggest we keep a list of the names we'd like to revisit, and we address them all at once at the end of the review (once we're happy with the contents). Here's a list of the current open naming issues: * MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much distance between these two semantically different operations * MemorySegment::isAccessible() -- as the A* word is overloaded, some other name should be found? * MemorySegment::acquire() -- replace with MemorySegment::fork? Cheers Maurizio On 05/12/2019 21:04, Maurizio Cimadamore wrote: Hi, as part of the effort to upstream the changes related to JEP 370 (foreign memory access API) [1], I'd like to ask for a code review for the corresponding core-libs and hotspot changes: http://cr.openjdk.java.net/~mcimadamore/panama/8234049/ A javadoc for the memory access API is also available here: http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html Note: the patch passes tier1, tier2 and tier3 testing (**) Here is a brief summary of the changes in java.base and hotspot (the remaining new files are implementation classes and tests for the new API): * ciField.cpp - this one is to trust final fields in the foreign memory access implementation (otherwise VM doesn't trust memory segment bounds) * Modules.gmk - these changes are needed to require that the incubating module is loaded by the boot loader (otherwise the above changes are useless) * library_call.cpp - this one is a JIT compiler change to treat Thread.currentThread() as a well-known constant - which helps a lot in the confinement checks (thanks Vlad!) * various Buffer-related changes; these changes are needed because the memory access API allows a memory segment to be projected into a byte
Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Looks good! Thanks, David On 7/12/2019 11:34 pm, gerard ziemski wrote: Hi all, Please review this revision 2 of a cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself. The rev2 differs from rev1 in that it's simpler due to JDK-8234185, which was just checked in yesterday. Waiting for this fix to go in first, allowed us not to have to delete the jdk_util.h file, which in turn means that we don't have to touch all those files that used that include. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2 tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...) cheers
Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"
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 subset of (1) - ie one or more specific platforms that support jpackage If the tests being updated are all of type (2) then the @requires platform may still be merited but probably that is not the case - it sounds like (1), so I think Andy should add what you suggest but I expect the @requires is harmless to leave in too ... -phil. On 12/7/19 1:47 PM, Igor Ignatev wrote: 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 jtreg. — Igor On Dec 7, 2019, at 1:43 PM, Phil Race wrote: 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 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, 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 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 some helper code which decides if it is applicable but that will be more work upfront and many jpackage tests are already platform specific so @requires is not going away. -Phil. On Dec 6, 2019, at 2:33 PM, Alexander Matveev wrote: 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 [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] https://bugs.openjdk.java.net/browse/JDK-8235453 [2] http://cr.openjdk.java.net/~herrick/8235453/ /Andy
Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"
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 jtreg. — Igor > On Dec 7, 2019, at 1:43 PM, Phil Race wrote: > > 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 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, 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 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 some helper code which > decides if it is applicable but that will be more work upfront and many > jpackage tests are already platform specific so @requires is not going > away. > > > -Phil. > >> On Dec 6, 2019, at 2:33 PM, Alexander Matveev >> wrote: >> >> 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 [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] https://bugs.openjdk.java.net/browse/JDK-8235453 [2] http://cr.openjdk.java.net/~herrick/8235453/ /Andy >> >
Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"
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 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, 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 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 some helper code which decides if it is applicable but that will be more work upfront and many jpackage tests are already platform specific so @requires is not going away. -Phil. > On Dec 6, 2019, at 2:33 PM, Alexander Matveev > wrote: > > 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 [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] https://bugs.openjdk.java.net/browse/JDK-8235453 >>> >>> [2] http://cr.openjdk.java.net/~herrick/8235453/ >>> >>> /Andy >>> >
Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"
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, 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 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 some helper code which >>> decides if it is applicable but that will be more work upfront and many >>> jpackage tests are already platform specific so @requires is not going away. >>> >>> >>> -Phil. >>> On Dec 6, 2019, at 2:33 PM, Alexander Matveev wrote: 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 [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] https://bugs.openjdk.java.net/browse/JDK-8235453 >> >> [2] http://cr.openjdk.java.net/~herrick/8235453/ >> >> /Andy >>
Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"
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 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 some helper code which decides if it is applicable but that will be more work upfront and many jpackage tests are already platform specific so @requires is not going away. -Phil. On Dec 6, 2019, at 2:33 PM, Alexander Matveev wrote: 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 [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] https://bugs.openjdk.java.net/browse/JDK-8235453 [2] http://cr.openjdk.java.net/~herrick/8235453/ /Andy
Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Looks good. Mandy On 12/7/19 5:34 AM, gerard ziemski wrote: Hi all, Please review this revision 2 of a cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself. The rev2 differs from rev1 in that it's simpler due to JDK-8234185, which was just checked in yesterday. Waiting for this fix to go in first, allowed us not to have to delete the jdk_util.h file, which in turn means that we don't have to touch all those files that used that include. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2 tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...) cheers
Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)
On 06/12/2019 21:04, Paul Sandoz wrote:
GroupLayout.java
—
80 OptionalLong sizeof(List elems) {
81 long size = 0;
82 for (MemoryLayout elem : elems) {
83 if (AbstractLayout.optSize(elem).isPresent()) {
84 size = sizeOp.applyAsLong(size, elem.bitSize());
85 } else {
86 return OptionalLong.empty();
87 }
88 }
89 return OptionalLong.of(size);
90 }
FWIW you can do this:
OptionalLong sizeof(List elems) {
return elems.stream().filter(e ->
AbstractLayout.optSize(e).isPresent()).mapToLong(MemoryLayout::bitSize)
.reduce(sizeOp);
Looked at this more carefully - the code you suggest has a slightly
different behavior than the one I wrote - note that the original code
short-circuit when the first layout member with no size is encountered.
IIUC your code will just drop unsized member layouts, and compute size
on the rest - which is not what I had in mind. Am I understanding correctly?
Maurizio
RE: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Hi Gerard, this looks good. Cheers Christoph > -Original Message- > From: gerard ziemski > Sent: Samstag, 7. Dezember 2019 14:34 > To: hotspot-dev developers ; core-libs- > d...@openjdk.java.net; Claes Redestad ; > Mandy Chung ; David Holmes > ; Sergey Bylokhov > ; Langer, Christoph > > Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove > JDK_GetVersionInfo0 and the supporting code" > > Hi all, > > Please review this revision 2 of a cleanup, where we remove > JDK_GetVersionInfo0 and related code, since we can get build versions > directly from within the VM itself. > > The rev2 differs from rev1 in that it's simpler due to JDK-8234185, > which was just checked in yesterday. Waiting for this fix to go in > first, allowed us not to have to delete the jdk_util.h file, which in > turn means that we don't have to touch all those files that used that > include. > > > bug: https://bugs.openjdk.java.net/browse/JDK-8223261 > webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2 > tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...) > > > cheers
Re: RFR: JDK-8235453: tools/jpackage/junit/junit.java failed due to "module not found: jdk.incubator.jpackage"
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 some helper code which decides if it is applicable but that will be more work upfront and many jpackage tests are already platform specific so @requires is not going away. -Phil. On Dec 6, 2019, at 2:33 PM, Alexander Matveev wrote: 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 [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] https://bugs.openjdk.java.net/browse/JDK-8235453 [2] http://cr.openjdk.java.net/~herrick/8235453/ /Andy
Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Looks good to me! /Claes On 2019-12-07 14:34, gerard ziemski wrote: Hi all, Please review this revision 2 of a cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself. The rev2 differs from rev1 in that it's simpler due to JDK-8234185, which was just checked in yesterday. Waiting for this fix to go in first, allowed us not to have to delete the jdk_util.h file, which in turn means that we don't have to touch all those files that used that include. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2 tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...) cheers
Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"
Hi all, Please review this revision 2 of a cleanup, where we remove JDK_GetVersionInfo0 and related code, since we can get build versions directly from within the VM itself. The rev2 differs from rev1 in that it's simpler due to JDK-8234185, which was just checked in yesterday. Waiting for this fix to go in first, allowed us not to have to delete the jdk_util.h file, which in turn means that we don't have to touch all those files that used that include. bug: https://bugs.openjdk.java.net/browse/JDK-8223261 webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2 tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...) cheers
Re: JDK 14 RFR of JDK-8235514: Update record serialization tests to not use hard coded source versions
Thanks for doing this Joe. The changes look good.
-Chris.
> On 6 Dec 2019, at 21:15, Joe Darcy wrote:
>
> Hello,
>
> Please review the patch below to update to the records serialization tests to
> avoid hard-coded values for the -source argument during programmatic
> invocations of the compiler.
>
> Thanks,
>
> -Joe
>
> diff -r 3b9efbac1b50
> test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java
> --- a/test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java Fri Dec
> 06 12:13:25 2019 -0800
> +++ b/test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java Fri Dec
> 06 13:13:45 2019 -0800
> @@ -59,6 +59,7 @@
> * constructor cannot be found during deserialization.
> */
> public class BadCanonicalCtrTest {
> +private static final String VERSION =
> Integer.toString(Runtime.version().feature());
>
> // ClassLoader for creating instances of the records to test with.
> ClassLoader goodRecordClassLoader;
> @@ -79,7 +80,7 @@
> {
> byte[] byteCode = InMemoryJavaCompiler.compile("R1",
> "public record R1 () implements java.io.Serializable {
> }",
> -"--enable-preview", "-source", "14");
> +"--enable-preview", "-source", VERSION);
> goodRecordClassLoader = new ByteCodeLoader("R1", byteCode,
> BadCanonicalCtrTest.class.getClassLoader());
> byte[] bc1 = removeConstructor(byteCode);
> missingCtrClassLoader = new ByteCodeLoader("R1", bc1,
> BadCanonicalCtrTest.class.getClassLoader());
> @@ -89,7 +90,7 @@
> {
> byte[] byteCode = InMemoryJavaCompiler.compile("R2",
> "public record R2 (int x, int y) implements
> java.io.Serializable { }",
> -"--enable-preview", "-source", "14");
> +"--enable-preview", "-source", VERSION);
> goodRecordClassLoader = new ByteCodeLoader("R2", byteCode,
> goodRecordClassLoader);
> byte[] bc1 = removeConstructor(byteCode);
> missingCtrClassLoader = new ByteCodeLoader("R2", bc1,
> missingCtrClassLoader);
> @@ -101,7 +102,7 @@
> "public record R3 (long l) implements
> java.io.Externalizable {" +
> "public void writeExternal(java.io.ObjectOutput out)
> { }" +
> "public void readExternal(java.io.ObjectInput in)
> { } }",
> -"--enable-preview", "-source", "14");
> +"--enable-preview", "-source", VERSION);
> goodRecordClassLoader = new ByteCodeLoader("R3", byteCode,
> goodRecordClassLoader);
> byte[] bc1 = removeConstructor(byteCode);
> missingCtrClassLoader = new ByteCodeLoader("R3", bc1,
> missingCtrClassLoader);
> diff -r 3b9efbac1b50
> test/jdk/java/io/Serializable/records/ProhibitedMethods.java
> --- a/test/jdk/java/io/Serializable/records/ProhibitedMethods.java Fri Dec 06
> 12:13:25 2019 -0800
> +++ b/test/jdk/java/io/Serializable/records/ProhibitedMethods.java Fri Dec 06
> 13:13:45 2019 -0800
> @@ -69,6 +69,7 @@
> * record objects.
> */
> public class ProhibitedMethods {
> +private static final String VERSION =
> Integer.toString(Runtime.version().feature());
>
> public interface ThrowingExternalizable extends Externalizable {
> default void writeExternal(ObjectOutput out) {
> @@ -106,7 +107,7 @@
> {
> byte[] byteCode = InMemoryJavaCompiler.compile("Foo",
> "public record Foo () implements java.io.Serializable {
> }",
> -"--enable-preview", "-source", "14");
> +"--enable-preview", "-source", VERSION);
> byteCode = addWriteObject(byteCode);
> byteCode = addReadObject(byteCode);
> byteCode = addReadObjectNoData(byteCode);
> @@ -115,7 +116,7 @@
> {
> byte[] byteCode = InMemoryJavaCompiler.compile("Bar",
> "public record Bar (int x, int y) implements
> java.io.Serializable { }",
> -"--enable-preview", "-source", "14");
> +"--enable-preview", "-source", VERSION);
> byteCode = addWriteObject(byteCode);
> byteCode = addReadObject(byteCode);
> byteCode = addReadObjectNoData(byteCode);
> @@ -125,7 +126,7 @@
> byte[] byteCode = InMemoryJavaCompiler.compile("Baz",
> "import java.io.Serializable;" +
> "public record Baz Serializable>(U u, V v) implements Serializable { }",
> -"--enable-preview", "-source", "14");
> +"--enable-preview", "-source", VERSION);
> byteCode = addWriteObject(byteCode);
> byteCode = addReadObject(byteCode);
> byteCode = addReadObjectNoData(byteCode);
> diff -r 3b9efbac1b50
>
