RFR: 8236282: [macos] Find permanent solution to macOS test timeout problem JDK-8235738

Please review the jpackage fix for bug [1] at [2]. This is still a workaround for JDK-8236825. Implemented as per Roger suggestion to redirect output to temp file. I did a lot of testing using repro case and was not able to reproduce hang anymore. [1] https://bugs.openjdk.java.net/browse/JDK-

RFR: 8246592: Simplify checking of boolean file attributes

Hi, this patch proposes adding a FileSystem.getBooleanAttribute convenience method, which simplifies code at use sites. This also enables improvements when not requesting an attribute that requires extra work, such as getting the hidden bit on unix derivatives. Bug:https://bugs.openjdk.java.

Re: RFR: 8246592: Simplify checking of boolean file attributes

Hi Claes, Not a review... You'll need a CSR for the API change. FileSystem.java: You'll need proper javadoc for the new getBooleanAttribute method. 6/11 is Rampdown Phase One  (its a bit late for new APIs). Regards, Roger On 6/4/20 8:58 AM, Claes Redestad wrote: Hi, this patch proposes

Re: RFR: 8246592: Simplify checking of boolean file attributes

On 04/06/2020 14:22, Roger Riggs wrote: Hi Claes, Not a review... You'll need a CSR for the API change. FileSystem.java: You'll need proper javadoc for the new getBooleanAttribute method. FileSystem is not a public class so it's not actually an API change. I agree the public modifier is conf

Re: RFR: 8246592: Simplify checking of boolean file attributes

Hi Roger, maybe I've missed something, but java.io.FileSystem is package-private and not part of the public API. Is a CSR really required? /Claes On 2020-06-04 15:22, Roger Riggs wrote: Hi Claes, Not a review... You'll need a CSR for the API change. FileSystem.java: You'll need proper javad

Re: RFR: 8246592: Simplify checking of boolean file attributes

Hi Claes, Yep, my mistake, too many publics. Thanks, Roger On 6/4/20 9:25 AM, Claes Redestad wrote: Hi Roger, maybe I've missed something, but java.io.FileSystem is package-private and not part of the public API. Is a CSR really required? /Claes On 2020-06-04 15:22, Roger Riggs wrote: Hi

Re: RFR: 8246592: Simplify checking of boolean file attributes

On 2020-06-04 15:26, Alan Bateman wrote: On 04/06/2020 14:22, Roger Riggs wrote: Hi Claes, Not a review... You'll need a CSR for the API change. FileSystem.java: You'll need proper javadoc for the new getBooleanAttribute method. FileSystem is not a public class so it's not actually an API

Re: RFR: 8246592: Simplify checking of boolean file attributes

Hi Claes, The code looks ok but I would rename the new method to 'hasBooleanAttribute' since it is not returning the attribute but testing for it. I think you can leave the 'public' modifiers to a separate issue. Regards, Roger On 6/4/20 9:41 AM, Claes Redestad wrote: On 2020-06-04 15:26

JDK-8238204: run_tests.sh fails on macOS when called from test_jpackage.sh

Please review fix [2] for jpackage bug [1]. Replace xargs call with --no-run-if-empty parameter with bash expressions in run_tests.sh Call run_tests.sh from test_jpackager.sh in a way to avoid shebang interpretation that doesn't work properly in the environment without bash in the PATH. Both

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Mandy, I think this looks good. Just a few minor comments... The spec wording is hard, since the method is offering a conditional initialization. I guess I expected to see something like “initializes targetClass, if it has not been initialized already”, or some such. The input and output is th

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On Jun 3, 2020, at 7:31 PM, Mandy Chung wrote: > > > > On 6/3/20 5:16 PM, Paul Sandoz wrote: >> Hi Mandy, >> >> Did you consider an alternative name, such as: >> >> /** >> * Initializes {@code targetClass}, if not already initialized. >> * ... >> */ >> public Class initializeClass(Clas

Re: JDK-8238204: run_tests.sh fails on macOS when called from test_jpackage.sh

looks good. /Andy On 6/4/2020 12:21 PM, Alexey Semenyuk wrote: Please review fix [2] for jpackage bug [1]. Replace xargs call with --no-run-if-empty parameter with bash expressions in run_tests.sh Call run_tests.sh from test_jpackager.sh in a way to avoid shebang interpretation that doesn't

Re: RFR(S/T) : 8245874 : requires.extraPropDefns.vmOpts doesn't need -Xbootclasspath/a:bootClasses

piny? -- Igor > On Jun 1, 2020, at 7:54 AM, Igor Ignatyev wrote: > > ping? > -- Igor > >> On May 27, 2020, at 10:25 AM, Igor Ignatyev wrote: >> >> http://cr.openjdk.java.net/~iignatyev//8245874/webrev.00 >>> 8 lines changed: 2 ins; 0 del; 6 mod; >> >> Hi all, >> >> could you please review t

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

Hello, Could you please review new version of the patch: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v3/ I’ve moved all logic for creating TLS Channel Binding data out of GssKrb5Client.java file. All data is prepared inside TlsChannelBinding class. Internal property name is renamed to “

Re: RFR: 8246592: Simplify checking of boolean file attributes

Hi Roger, On 2020-06-04 18:05, Roger Riggs wrote: Hi Claes, The code looks ok but I would rename the new method to 'hasBooleanAttribute' since it is not returning the attribute but testing for it. good suggestion: http://cr.openjdk.java.net/~redestad/8246592/open.01/ I think you can lea

request for review JDK-8211290

Hi all, please review the following changeset: http://cr.openjdk.java.net/~iignatyev/isipka/8211974/webrev.01/index.html for the JBS issue https://bugs.openjdk.java.net/browse/JDK-8211974 which moves the files: open/test/jdk/lib/testlibrary/java/util/jar/Compiler.java open/test/jdk/lib/testlib

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 04/06/2020 17:55, Paul Sandoz wrote: : FWIW I keep tripping over the prefix “ensures”. As I understand the implementation initializes a class, if not already so (and it has to check as you point out below). Focusing on the main action seems appropriate in this regard. I expect John has mor

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On Jun 4, 2020, at 11:43 AM, Alan Bateman wrote: > > On 04/06/2020 17:55, Paul Sandoz wrote: >> : >> FWIW I keep tripping over the prefix “ensures”. As I understand the >> implementation initializes a class, if not already so (and it has to check >> as you point out below). Focusing on th

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Hi Mandy, What about this case: class Test { static { MethodHandles.lookup().ensureClassInitialized(Test.class); // not yet initialized } } (I see you do that for m/p1/A.java and Test.clinitInvokeEnsureClassInitialized.) Do we need mention this in the spec? e.g. this method does not

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 6/4/20 9:43 AM, Chris Hegarty wrote: Mandy, I think this looks good. Just a few minor comments... The spec wording is hard, since the method is offering a conditional initialization. I guess I expected to see something like “initializes targetClass, if it has not been initialized alread

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

JVMS 5.5 is the spec for class initialization.  Step 3 describes the recursive request for initialization. I have @jvms 5.5 in the javadoc. I can make it explicit: * Ensures that {@code targetClass} has been initialized. The class * to be initialized must be {@linkplain #acces

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Hi, Why can't it just return void? Florent On Thu, Jun 4, 2020 at 9:22 PM Mandy Chung wrote: > > > On 6/4/20 9:43 AM, Chris Hegarty wrote: > > Mandy, > > > > I think this looks good. Just a few minor comments... > > > > The spec wording is hard, since the method is offering a conditional > ini

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Lookup::accessClass sets the precedence to return Class for method chaining.  So this new method follows the existing convention. Mandy On 6/4/20 12:31 PM, Florent Guillaume wrote: Hi, Why can't it just return void? Florent On Thu, Jun 4, 2020 at 9:22 PM Mandy Chung

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

2020/6/3 19:31:13 -0700, mandy.ch...@oracle.com: > On 6/3/20 5:16 PM, Paul Sandoz wrote: >> Did you consider an alternative name, such as: >> >> /** >> * Initializes {@code targetClass}, if not already initialized. >> * ... >> */ >> public Class initializeClass(Class targetClass) ... >> >> ? >

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On Jun 4, 2020, at 12:31 PM, Mandy Chung wrote: > > JVMS 5.5 is the spec for class initialization. Step 3 describes the > recursive request for initialization. > Ok. (From quick offline chat we agreed its not worth explicitly highlighting this case.) > I have @jvms 5.5 in the javadoc.

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 6/4/20 12:39 PM, mark.reinh...@oracle.com wrote: I agree that the `ensure` prefix is useful here. This method can only ensure that a class is initialized, so including `Class` in the method name seems redundant. I’d go with the shorter `ensureInitialized`. - Mark Thanks for the advice.  

RFR: JDK-824662: Refactor JLinkBundlerHelper and StandardBundlerParam classes

Please review fix [2] for jpackage bug [1]. Move functionality to collect data about app (main class name, main jar, module name, etc) from JLinkBundlerHelper and StandardBundlerParam classes in dedicated LauncherData class. Remove ArgAction, ModFile and RelativeFileSet classes as they are not

RFR: JDK-8246627: Consolidate app image bundlers

Please review fix [2] for jpackage bug [1]. Move duplicated functionality from LinuxAppBundler, MacAppBundler and WinAppBundler classes in the base class. [2] webrev is on top of [3] webrev. - Alexey [1] https://bugs.openjdk.java.net/browse/JDK-8246627 [2] http://cr.openjdk.java.net/~asemen

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On Jun 4, 2020, at 12:39 PM, mark.reinh...@oracle.com wrote: > > I agree that the `ensure` prefix is useful here. > > This method can only ensure that a class is initialized, so including > `Class` in the method name seems redundant. I’d go with the shorter > `ensureInitialized`. +1

Re: RFR(S/T) : 8245874 : requires.extraPropDefns.vmOpts doesn't need -Xbootclasspath/a:bootClasses

Looks good. Leonid On 6/4/20 10:21 AM, Igor Ignatyev wrote: piny? -- Igor On Jun 1, 2020, at 7:54 AM, Igor Ignatyev wrote: ping? -- Igor On May 27, 2020, at 10:25 AM, Igor Ignatyev wrote: http://cr.openjdk.java.net/~iignatyev//8245874/webrev.00 8 lines changed: 2 ins; 0 del; 6 mod; Hi

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On Jun 3, 2020, at 7:38 PM, Mandy Chung wrote: > > On 6/3/20 6:24 PM, John Rose wrote: >> On Jun 3, 2020, at 5:16 PM, Paul Sandoz > > wrote: >>> >>> if (UNSAFE.shouldBeInitialized(cls)) { >>> UNSAFE.ensureClassInitialized(cls); >>> } >>> >>> Although it see

Re: JDK-8238204: run_tests.sh fails on macOS when called from test_jpackage.sh

Hi Alexey, Looks good. Thanks, Alexander On 6/4/20 9:21 AM, Alexey Semenyuk wrote: Please review fix [2] for jpackage bug [1]. Replace xargs call with --no-run-if-empty parameter with bash expressions in run_tests.sh Call run_tests.sh from test_jpackager.sh in a way to avoid shebang interpr

Re: RFR: JDK-824662: Refactor JLinkBundlerHelper and StandardBundlerParam classes

Hi Alexey, Looks good. Thanks, Alexander On 6/4/20 1:22 PM, Alexey Semenyuk wrote: Please review fix [2] for jpackage bug [1]. Move functionality to collect data about app (main class name, main jar, module name, etc) from JLinkBundlerHelper and StandardBundlerParam classes in dedicated Lau

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

P.S.C. (post-send clarification) > The workflow would be: > > static final MethodHandle MH_ensureInit > = publicLookup().findVirtual(L…, “ensureInitialized”…); > By that I mean the workflow of the dynamic language implementor. And after hitting “send” I realized that optimizing that one case

Re: RFR: 8236282: [macos] Find permanent solution to macOS test timeout problem JDK-8235738

looks good. /Andy On 6/4/2020 3:03 AM, alexander.matv...@oracle.com wrote: Please review the jpackage fix for bug [1] at [2]. This is still a workaround for JDK-8236825. Implemented as per Roger suggestion to redirect output to temp file. I did a lot of testing using repro case and was not a

Re: RFR: 8236282: [macos] Find permanent solution to macOS test timeout problem JDK-8235738

Looks good. - Alexey On 6/4/2020 3:03 AM, alexander.matv...@oracle.com wrote: Please review the jpackage fix for bug [1] at [2]. This is still a workaround for JDK-8236825. Implemented as per Roger suggestion to redirect output to temp file. I did a lot of testing using repro case and was no

Re: RFR: JDK-8246627: Consolidate app image bundlers

Hi Alexey, Looks good. Thanks, Alexander On 6/4/20 1:46 PM, Alexey Semenyuk wrote: Please review fix [2] for jpackage bug [1]. Move duplicated functionality from LinuxAppBundler, MacAppBundler and WinAppBundler classes in the base class. [2] webrev is on top of [3] webrev. - Alexey [1] h

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

I create https://bugs.openjdk.java.net/browse/JDK-8246634 to track this. thanks Mandy On 6/4/20 4:35 PM, John Rose wrote: P.S.C. (post-send clarification) The workflow would be: static final MethodHandle MH_ensureInit = publicLookup().findVirtual(L…, “ensureInitialized”…); By that I mea

Re: RFR(XL) 8198698: Support Lambda proxy classes in dynamic CDS archive

Hi Mandy, Thanks for taking another look. On 6/3/20 2:07 PM, Mandy Chung wrote: On 6/3/20 12:34 PM, Calvin Cheung wrote: I saw David has commented on this. So I'll leave the assert as before and I've added another assert (see line 1691): 1687   // The following ensures that the caller's

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

Hi Alexey, It's so unfortunate that different addressType must be used. I'm OK with the new TlsChannelBindingImpl class. One thing I'm not comfortable is the java.security.sasl/share/classes/module-info.java change. We've struggled hard to avoid these kind of secret tunnels. Is it possible to