On Thu, 27 Feb 2025 17:49:48 GMT, Mikhail Yankelevich 
<myankelev...@openjdk.org> wrote:

>> Refactor the following to run fully in java:
>> test/java/security//Security/ClassLoaderDeadlock/ClassLoaderDeadlock.sh
>> test/java/security//Security/ClassLoaderDeadlock/Deadlock.sh
>
> Mikhail Yankelevich has updated the pull request with a new target base due 
> to a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - typo
>  - Merge branch 'master' into JDK-8349348
>  - JDK-8349348: Refactor ClassLoaderDeadlock.sh and Deadlock.sh to run fully 
> in java

test/jdk/java/security/Security/ClassLoaderDeadlock/ClassLoaderDeadlock.java 
line 28:

> 26: * @bug 5094825
> 27: * @summary verify no deadlock if crypto provider in other classloader is 
> used to verify signed jars
> 28: * @modules java.base/java.security

Hello Mikhail, `java.security` package is an exported package from `java.base`. 
So you don't need this `@modules` declaration.

test/jdk/java/security/Security/ClassLoaderDeadlock/ClassLoaderDeadlock.java 
line 30:

> 28: * @modules java.base/java.security
> 29: * @library ./Deadlock.jar
> 30: * @compile -g provider/HashProvider.java

Is the `-g` needed?

test/jdk/java/security/Security/ClassLoaderDeadlock/ClassLoaderDeadlock.java 
line 31:

> 29: * @library ./Deadlock.jar
> 30: * @compile -g provider/HashProvider.java
> 31: * @run main/othervm/timeout=30 -Djava.awt.headless=true 
> ClassLoaderDeadlock

The `-Djava.awt.headless=true` is likely not needed but since it was used in 
the original test script, I think it's OK to carry it over.

test/jdk/java/security/Security/ClassLoaderDeadlock/Deadlock.java line 29:

> 27:  * @summary make sure we do not deadlock loading signed JAR with 
> getInstance()
> 28:  * @library ./Deadlock.jar
> 29:  * @build Deadlock

`Deadlock` here is the class name of the test itself. So you don't need to 
`@build` it explicitly.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23440#discussion_r1983175334
PR Review Comment: https://git.openjdk.org/jdk/pull/23440#discussion_r1983177523
PR Review Comment: https://git.openjdk.org/jdk/pull/23440#discussion_r1983177327
PR Review Comment: https://git.openjdk.org/jdk/pull/23440#discussion_r1983179883

Reply via email to