8269486: CallerAccessTest fails for non server variant #159

mychris · 2021-06-28T13:14:51Z

Hi,

please review this small fix. The test case uses a custom launcher and before launching the JVM, it adds the "lib" and "lib/server" directories to the environment variable which controls the native library search path. For non server variants, the second directory is not called "lib/server", but "lib/client", for instance.

I changed the test case to use the utility methods in Platform to get the correct paths, dependent on the VM variant.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8269486: CallerAccessTest fails for non server variant

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 84b2354
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 84b2354
Mandy Chung (@mlchung - Reviewer)
Jorn Vernee (@JornVernee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/159/head:pull/159
$ git checkout pull/159

Update a local copy of the PR:
$ git checkout pull/159
$ git pull https://git.openjdk.java.net/jdk17 pull/159/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 159

View PR using the GUI difftool:
$ git pr show -t 159

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/159.diff

bridgekeeper · 2021-06-28T13:15:01Z

👋 Welcome back cgo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-06-28T13:18:11Z

@mychris The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-06-28T13:21:52Z

Webrevs

01: Full - Incremental (71e0e5b6)
00: Full (84b23542)

dholmes-ora

This generally seems fine to me (assuming the Platform utility methods do as would be expected).

One suggestion below.

Thanks,
David

dholmes-ora · 2021-06-28T13:44:41Z

test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java

-        String libDir = libPath.toAbsolutePath().toString();
-        String serverDir = libPath.resolve("server").toAbsolutePath().toString();
+        String libDir = Platform.libDir().toString();
+        String serverDir = Platform.jvmLibDir().toString();


Perhaps vmDir would be a more suitable name?

Makes sense, thank you.

openjdk · 2021-06-28T13:46:48Z

@mychris This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8269486: CallerAccessTest fails for non server variant

Reviewed-by: dholmes, stuefe, mchung, jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

6b64a79: 8268350: Remove assert that ensures thread identifier remains the same
90eb118: 8269528: VectorAPI Long512VectorTest fails on X86 KNL target
a661686: 8269065: [REDO] vmTestbase/vm/mlvm/anonloader/stress/oome/metaspace/Test.java failed with OutOfMemoryError
0d745ae: 8269034: AccessControlException for SunPKCS11 daemon threads
d042029: 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails in Windows debug build
401cb0a: 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
b8a16e9: 8268884: C2: Compile::remove_speculative_types must iterate top-down
25f9f19: 8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}
0d83dc2: 8268699: Shenandoah: Add test for JDK-8268127
7010dfd: 8269517: compiler/loopopts/TestPartialPeelingSinkNodes.java crashes with -XX:+VerifyGraphEdges
... and 7 more: https://git.openjdk.java.net/jdk17/compare/d9cb068f4ba85e2be2ebc14e89e739b0a2d90409...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @tstuefe, @mlchung, @JornVernee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

tstuefe

Looks reasonable (Davids suggestion sounds good).

..Thomas

mychris · 2021-06-28T14:49:23Z

Thanks for the reviews.

assuming the Platform utility methods do as would be expected

Yes, the utility method does the switch between "bin" and "lib" for windows and non-windows

jdk17/test/lib/jdk/test/lib/Platform.java

Lines 346 to 350 in e4c5446

    
           if (Platform.isWindows()) { 
        
               return image.resolve("bin"); 
        
           } else { 
        
               return image.resolve("lib"); 
        
           }

and it does the switch for the variant

jdk17/test/lib/jdk/test/lib/Platform.java

Lines 361 to 369 in e4c5446

    
           if (Platform.isServer()) { 
        
               return "server"; 
        
           } else if (Platform.isClient()) { 
        
               return "client"; 
        
           } else if (Platform.isMinimal()) { 
        
               return "minimal"; 
        
           } else { 
        
               throw new Error("TESTBUG: unsupported vm variant"); 
        
           }

I only have a linux setup, so I tested this with a client and a server VM only on linux.
Sorry, I didn't enable github workflows for my jdk17 fork yet. I did so now and hopefully the next commit will trigger it.

JornVernee

I only have a linux setup, so I tested this with a client and a server VM only on linux.

The test also passes on my Windows machine with a server VM.

mlbridge · 2021-06-29T02:18:34Z

Mailing list message from David Holmes on core-libs-dev:

On 29/06/2021 12:52 am, Christoph G?ttschkes wrote:

On Mon, 28 Jun 2021 13:14:51 GMT, Christoph G?ttschkes <cgo at openjdk.org> wrote:

Hi,

please review this small fix. The test case uses a custom launcher and before launching the JVM, it adds the "lib" and "lib/server" directories to the environment variable which controls the native library search path. For non server variants, the second directory is not called "lib/server", but "lib/client", for instance.

I changed the test case to use the utility methods in `Platform` to get the correct paths, dependent on the VM variant.

Thanks for the reviews.

assuming the Platform utility methods do as would be expected

Yes, the utility method does the switch between "bin" and "lib" for windows and non-windows

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L346-L350

and it does the switch for the variant

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L361-L369

I only have a linux setup, so I tested this with a client and a server VM only on linux.
Sorry, I didn't enable github workflows for my jdk17 fork yet. I did so now and hopefully the next commit will trigger it.

You can also trigger it manually. Go to your fork, go to the Actions
tab, select the workflow, select the branch to run it on.

Cheers,
David

mlbridge · 2021-06-29T02:18:36Z

Mailing list message from David Holmes on core-libs-dev:

On 29/06/2021 12:52 am, Christoph G?ttschkes wrote:

On Mon, 28 Jun 2021 13:14:51 GMT, Christoph G?ttschkes <cgo at openjdk.org> wrote:

Hi,

please review this small fix. The test case uses a custom launcher and before launching the JVM, it adds the "lib" and "lib/server" directories to the environment variable which controls the native library search path. For non server variants, the second directory is not called "lib/server", but "lib/client", for instance.

I changed the test case to use the utility methods in `Platform` to get the correct paths, dependent on the VM variant.

Thanks for the reviews.

assuming the Platform utility methods do as would be expected

Yes, the utility method does the switch between "bin" and "lib" for windows and non-windows

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L346-L350

and it does the switch for the variant

https://github.com/openjdk/jdk17/blob/e4c5446340605b112e0918fa9dcb48aaeaa730c8/test/lib/jdk/test/lib/Platform.java#L361-L369

I only have a linux setup, so I tested this with a client and a server VM only on linux.
Sorry, I didn't enable github workflows for my jdk17 fork yet. I did so now and hopefully the next commit will trigger it.

You can also trigger it manually. Go to your fork, go to the Actions
tab, select the workflow, select the branch to run it on.

Cheers,
David

mychris · 2021-06-30T08:32:45Z

Thanks for the reviews and testing.
/integrate

openjdk · 2021-06-30T08:34:14Z

@mychris
Your change (at version 71e0e5b) is now ready to be sponsored by a Committer.

mlchung · 2021-06-30T17:07:40Z

/sponsor

openjdk · 2021-06-30T17:08:46Z

Going to push as commit 1da5d4b.
Since your change was applied there have been 22 commits pushed to the master branch:

be0ac92: 8269614: [s390] Interpreter checks wrong bit for slow path instance allocation
4b4bef4: 8269594: assert(_handle_mark_nesting > 1) failed: memory leak: allocating handle outside HandleMark
4ee400a: 8268320: Better error recovery for broken patterns in switch
ca283c3: 8265907: JVM crashes when matching VectorMaskCmp Node
c3c9189: 8269141: Switch statement containing pattern case label element gets in the loop during execution
6b64a79: 8268350: Remove assert that ensures thread identifier remains the same
90eb118: 8269528: VectorAPI Long512VectorTest fails on X86 KNL target
a661686: 8269065: [REDO] vmTestbase/vm/mlvm/anonloader/stress/oome/metaspace/Test.java failed with OutOfMemoryError
0d745ae: 8269034: AccessControlException for SunPKCS11 daemon threads
d042029: 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails in Windows debug build
... and 12 more: https://git.openjdk.java.net/jdk17/compare/d9cb068f4ba85e2be2ebc14e89e739b0a2d90409...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-06-30T17:09:06Z

@mlchung @mychris Pushed as commit 1da5d4b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8269486: CallerAccessTest fails for non server variant

84b2354

openjdk bot added the rfr Pull request is ready for review label Jun 28, 2021

openjdk bot added the core-libs core-libs-dev@openjdk.java.net label Jun 28, 2021

dholmes-ora approved these changes Jun 28, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 28, 2021

tstuefe approved these changes Jun 28, 2021

View reviewed changes

Renames serverDir to vmDir.

71e0e5b

mlchung approved these changes Jun 28, 2021

View reviewed changes

JornVernee approved these changes Jun 28, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 30, 2021

openjdk bot closed this Jun 30, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 30, 2021

8269486: CallerAccessTest fails for non server variant #159

8269486: CallerAccessTest fails for non server variant #159

mychris commented Jun 28, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jun 28, 2021

openjdk bot commented Jun 28, 2021

mlbridge bot commented Jun 28, 2021 •

edited

dholmes-ora left a comment

dholmes-ora Jun 28, 2021

mychris Jun 28, 2021

openjdk bot commented Jun 28, 2021 •

edited

tstuefe left a comment

mychris commented Jun 28, 2021

JornVernee left a comment

mlbridge bot commented Jun 29, 2021

mlbridge bot commented Jun 29, 2021

mychris commented Jun 30, 2021

openjdk bot commented Jun 30, 2021

mlchung commented Jun 30, 2021

openjdk bot commented Jun 30, 2021

openjdk bot commented Jun 30, 2021

8269486: CallerAccessTest fails for non server variant #159

8269486: CallerAccessTest fails for non server variant #159

Conversation

mychris commented Jun 28, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 28, 2021

openjdk bot commented Jun 28, 2021

mlbridge bot commented Jun 28, 2021 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jun 28, 2021

Choose a reason for hiding this comment

mychris Jun 28, 2021

Choose a reason for hiding this comment

openjdk bot commented Jun 28, 2021 • edited

tstuefe left a comment

Choose a reason for hiding this comment

mychris commented Jun 28, 2021

JornVernee left a comment

Choose a reason for hiding this comment

mlbridge bot commented Jun 29, 2021

mlbridge bot commented Jun 29, 2021

mychris commented Jun 30, 2021

openjdk bot commented Jun 30, 2021

mlchung commented Jun 30, 2021

openjdk bot commented Jun 30, 2021

openjdk bot commented Jun 30, 2021

mychris commented Jun 28, 2021 •

edited by openjdk bot

mlbridge bot commented Jun 28, 2021 •

edited

openjdk bot commented Jun 28, 2021 •

edited