Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8319128: sun/security/pkcs11 tests fail on OL 7.9 aarch64 #16547

Closed
wants to merge 11 commits into from

Conversation

mpdonova
Copy link
Contributor

@mpdonova mpdonova commented Nov 7, 2023

In this PR, I included logic to skip tests on Oracle Linux prior to version 8. The NSS binaries we are using for testing use a newer version of GLIBC than is included with OL 7.9.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8319128: sun/security/pkcs11 tests fail on OL 7.9 aarch64 (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16547/head:pull/16547
$ git checkout pull/16547

Update a local copy of the PR:
$ git checkout pull/16547
$ git pull https://git.openjdk.org/jdk.git pull/16547/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16547

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16547.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 7, 2023

👋 Welcome back mdonovan! 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 openjdk bot added the rfr Pull request is ready for review label Nov 7, 2023
@openjdk
Copy link

openjdk bot commented Nov 7, 2023

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

  • security

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.

@openjdk openjdk bot added the security security-dev@openjdk.org label Nov 7, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 7, 2023

Webrevs

@@ -629,7 +629,6 @@ sun/security/pkcs11/rsa/TestKeyPairGenerator.java 8295343 generic-
sun/security/pkcs11/rsa/TestKeyFactory.java 8295343 generic-all
sun/security/pkcs11/KeyStore/Basic.java 8295343 generic-all
sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java 8316183 linux-ppc64le
sun/security/pkcs11/Provider/MultipleLogins.sh 8319128 linux-aarch64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for me, I'm not familiar enough with this test previously to know why it was a shell script before. Can you elaborate a little? I also assume this is an "active" problem list from which you're removing this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shell scripts compiles Java files with additional flags and does some additional set up before executing the test. I put the script on the ProblemList because the pkcs11 tests were failing on aarch64 and throwing a SkippedException from MultipleLogins.java would cause the test to fail. Ultimately, I could have made the change that is part of this PR (catching the SkippedException in the java file) but I didn't see it at the time.

@mpdonova
Copy link
Contributor Author

Hello, I'm looking for a reviewer for this PR. Thanks!

default:
return null;
}
}

private static boolean isOracleLinux7() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not move that into some central place, e.g. test/lib/Platform.java ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea; I moved the method to the Platform class.

@@ -743,15 +746,68 @@ private static String fetchNssLib(String osId) {
return fetchNssLib(MACOSX_AARCH64.class);

case "Linux-amd64-64":
return fetchNssLib(LINUX_X64.class);
if (isOracleLinux7()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be the same for the older RedHat versions, I think they are rather close to Oracle Linux ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the method would also return true for RH 7.

@openjdk
Copy link

openjdk bot commented Dec 15, 2023

@mpdonova 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:

8319128: sun/security/pkcs11 tests fail on OL 7.9 aarch64

Reviewed-by: mbaesken

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 109 new commits pushed to the master branch:

  • 9fd855e: 8322971: KEM.getInstance() should check if a 3rd-party security provider is signed
  • b8ae4a8: 8320890: [AIX] Find a better way to mimic dl handle equality
  • e5aed6b: 8323276: StressDirListings.java fails on AIX
  • b922f8d: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888
  • 35e9662: 8314515: java/util/concurrent/SynchronousQueue/Fairness.java failed with "Error: fair=false i=8 j=0"
  • cb1d25f: 8323330: [BACKOUT] JDK-8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
  • 2b7fc05: 8264102: JTable Keyboards Navigation differs with Test Instructions.
  • af942a6: 8323188: JFR: Needless RESOURCE_ARRAY when sending EventOSInformation
  • 26de9e2: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile
  • b530c02: 8317804: com/sun/jdi/JdwpAllowTest.java fails on Alpine 3.17 / 3.18
  • ... and 99 more: https://git.openjdk.org/jdk/compare/27d5f5c237910bc3d2df62367d2e0a83c1132885...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 15, 2023
@mpdonova
Copy link
Contributor Author

Hello, this PR was approved but then I had to make some additional changes so I'd like to have an additional review/approval before merging it.

Thanks!

@@ -348,6 +349,20 @@ private static boolean isArch(String archnameRE) {
.matches();
}

public static boolean isOracleLinux7() {
if (System.getProperty("os.name").toLowerCase().contains("linux") &&
System.getProperty("os.version").toLowerCase().contains("el")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I checked and the el ("Enterprise Linux" ?) is shown in os.version on RHEL and Oracle Linux. But how reliable is it? Could this show up also on some other distros in os.version (maybe in the cpu-related string of uname output?) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of any conventions in the output of uname that would prevent other operating systems from including "el" followed by a number.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay , then let's keep it the way it is now and adjust later if needed.

@mpdonova
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jan 11, 2024

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

  • 9fd855e: 8322971: KEM.getInstance() should check if a 3rd-party security provider is signed
  • b8ae4a8: 8320890: [AIX] Find a better way to mimic dl handle equality
  • e5aed6b: 8323276: StressDirListings.java fails on AIX
  • b922f8d: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888
  • 35e9662: 8314515: java/util/concurrent/SynchronousQueue/Fairness.java failed with "Error: fair=false i=8 j=0"
  • cb1d25f: 8323330: [BACKOUT] JDK-8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
  • 2b7fc05: 8264102: JTable Keyboards Navigation differs with Test Instructions.
  • af942a6: 8323188: JFR: Needless RESOURCE_ARRAY when sending EventOSInformation
  • 26de9e2: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile
  • b530c02: 8317804: com/sun/jdi/JdwpAllowTest.java fails on Alpine 3.17 / 3.18
  • ... and 99 more: https://git.openjdk.org/jdk/compare/27d5f5c237910bc3d2df62367d2e0a83c1132885...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 11, 2024
@openjdk openjdk bot closed this Jan 11, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 11, 2024
@openjdk
Copy link

openjdk bot commented Jan 11, 2024

@mpdonova Pushed as commit c2e77e2.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
3 participants