8350964: Add an ArtifactResolver.fetch(clazz) method #23989

mpdonova · 2025-03-11T15:21:09Z

In this PR, I created a new method, ArtifactResolver.fetchOne(), to consolidate duplicate code across tests.

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-8350964: Add an ArtifactResolver.fetch(clazz) method (Enhancement - P4)

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23989

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-11T15:21:52Z

👋 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 · 2025-03-11T15:22:52Z

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

8350964: Add an ArtifactResolver.fetch(clazz) method

Reviewed-by: weijun

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

0454406: 8351987: ProblemList the failing JFR streaming tests on macOS
be36b23: 8351778: JIT compiler fails when running -XX:AOTMode=create
7fc776e: 8270265: LineBreakMeasurer calculates incorrect line breaks with zero-width characters
3da5e3f: 8349350: Unable to print using InputSlot and OutputBin print attributes at the same time
aa047ee: 8319192: Remove javax.swing.plaf.synth.SynthLookAndFeel.load(URL url)
cd1be91: 8319055: JCMD should not buffer the whole output of commands
248c373: 8351266: JFR: -XX:StartFlightRecording:report-on-exit
03ef79c: 8346470: Improve WriteBarrier JMH to have old-to-young refs
b50fe9b: 8280818: Expand bug8033699.java to iterate over all LaFs
771e160: 8351323: Parameterize compiler and linker flags for iconv
... and 44 more: https://git.openjdk.org/jdk/compare/0de2cddf3a7be23f67af93972875af1235f3107e...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 · 2025-03-11T15:23:30Z

@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.

mlbridge · 2025-03-11T15:26:03Z

Webrevs

myankelev · 2025-03-11T15:25:40Z

test/lib/jdk/test/lib/artifacts/ArtifactResolver.java

+        } catch (ArtifactResolverException e) {
+            Throwable cause = e.getCause();
+            if (cause == null) {
+                // if property doesn't exist


As per our discussion, do you think doing it in a way similar to this would be easier to read during a debug?

I updated the message.

myankelev · 2025-03-11T15:39:17Z

test/jdk/sun/security/provider/acvp/Launcher.java

        }
    }

    private static Path fetchACVPServerTests(Class<?> clazz) {


Is there a point in this method? It's used in 1 spot only it seems and you can just directly call fetchOne

It encapsulates all of the logic involved in getting the tests. Specifically, what to do if the tests can't be fetched. It could be done in main() but this is a little cleaner.

I'd rather just let ArtifactResolver.fetchOne throw the SkippedException. Is that always what we need?

I can't think of a reason why a test would want to continue if the file wasn't accessible. I moved the skippedexception into thefetchOne method.

fguallini · 2025-03-11T15:59:53Z

test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java

+            try {
+                String opensslPath = OpensslArtifactFetcher.getOpensslPath();
                // if the current version of openssl is available, perform all
                // keytool <-> openssl interop tests
                generateInitialKeystores(opensslPath);
                testWithJavaCommands();
                testWithOpensslCommands(opensslPath);


should only try catch the Artifact fetching line, as other test methods could potentially throw an IOException and it could get hidden with a SkippedException

Suggested change

try {

String opensslPath = OpensslArtifactFetcher.getOpensslPath();

// if the current version of openssl is available, perform all

// keytool <-> openssl interop tests

generateInitialKeystores(opensslPath);

testWithJavaCommands();

testWithOpensslCommands(opensslPath);

String opensslPath;

try {

opensslPath = OpensslArtifactFetcher.getOpensslPath();

} catch (IOException exc) {

String exMsg = "Can't find the version: "

+ OpensslArtifactFetcher.getTestOpensslBundleVersion()

+ " of openssl binary on this machine, please install"

+ " and set openssl path with property 'test.openssl.path'";

throw new SkippedException(exMsg);

}

// if the current version of openssl is available, perform all

// keytool <-> openssl interop tests

generateInitialKeystores(opensslPath);

testWithJavaCommands();

testWithOpensslCommands(opensslPath);

Yep, that makes sense.

looks good thanks

…tool test

myankelev · 2025-03-12T10:36:45Z

LGTM

wangweij · 2025-03-12T13:27:49Z

test/jdk/sun/security/pkcs11/PKCS11Test.java

    }

-    private static Path fetchNssLib(String osId, Path libraryName) {
+    private static Path fetchNssLib(String osId, Path libraryName) throws IOException {


Is the IOException caught later and wrapped as a SkippedException?

I changed ArtifactResolver.fetchOne() to throw a SkippedException but this method still throws an IOException if the nss directory/artifact doesn't contain the necessary files. That case is handled by different tests and I didn't want to change it.

wangweij · 2025-03-12T13:29:08Z

test/jdk/sun/security/provider/acvp/Launcher.java

        }
    }

    private static Path fetchACVPServerTests(Class<?> clazz) {


I'd rather just let ArtifactResolver.fetchOne throw the SkippedException. Is that always what we need?

wangweij · 2025-03-12T13:31:56Z

test/lib/jdk/test/lib/artifacts/ArtifactResolver.java

+            if (artifact != null) {
+                message = "Cannot find the artifact " + artifact.name();
+            } else {
+                message = "Class " + klass.getName() + " missing @Artifact annotation.";


I assume it's a bug if klass is not annotated with @Artifact. I'd rather throw an NPE here so we can notice it.

Do you mean to just assume artifact is never null and let the NPE be thrown if it is?

I updated the code to reflect that.

wangweij · 2025-03-12T16:07:46Z

test/jdk/sun/security/provider/acvp/Launcher.java

 import jdk.test.lib.json.JSONValue;
 import jtreg.SkippedException;

+import java.io.IOException;


Useless now. The import jtreg.SkippedException is also useless.

wangweij · 2025-03-12T16:17:32Z

test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java

-                testWithJavaCommands();
-                testWithOpensslCommands(opensslPath);
-            } else {
+            if (opensslPath == null) {


Can we modify OpensslArtifactFetcher.getOpensslPath() so that the SkippedException is thrown inside it when the version check fails at the last line? You are already throwing this exception when fetchOpenssl is called there.

This makes sure the return value is never null, and it's consistent to ArtifactResolver.fetchOne. You can add a @throws javadoc there for clarity.

wangweij · 2025-03-12T16:20:41Z

test/lib/jdk/test/lib/artifacts/ArtifactResolver.java


+import jtreg.SkippedException;
+
+import java.io.IOException;


Useless now.

wangweij · 2025-03-12T16:21:54Z

test/lib/jdk/test/lib/artifacts/ArtifactResolver.java

+     * Retrieve an artifact/library/file from a repository or local file system.
+     * <p>
+     * Artifacts are defined with the {@link jdk.test.lib.artifacts.Artifact}
+     * annotation. The file name should have the format ARTIFACT_NAME-VERSION.EXT


It could also be a directory, right? So maybe path name is more precise.

Also, you haven't defined what each piece in ARTIFACT_NAME-VERSION.EXT is. On the other hand, the names in Artifact are name, revision, and extension.

This sentence doesn't really belong here anyway so I removed it entirely

wangweij · 2025-03-12T16:22:27Z

test/lib/jdk/test/lib/artifacts/ArtifactResolver.java

+     * annotation. The file name should have the format ARTIFACT_NAME-VERSION.EXT
+     * <p>
+     * If you have a local version of a dependency that you want to use, you can
+     * specify that by setting the System property:


No need to capitalize system.

wangweij · 2025-03-12T16:25:30Z

test/lib/jdk/test/lib/artifacts/ArtifactResolver.java

+     * @param klass a class annotated with {@link jdk.test.lib.artifacts.Artifact}
+     * @return the local path to the artifact. If the artifact is a compressed
+     * file that gets unpacked, this path will point to the root
+     * directory of the uncompressed file.


Maybe file(s)?

wangweij · 2025-03-12T16:29:33Z

test/lib/jdk/test/lib/security/OpensslArtifactFetcher.java

 package jdk.test.lib.security;

-import java.io.File;
+import java.io.IOException;


Useless now.

wangweij · 2025-03-12T16:39:41Z

test/jdk/sun/security/pkcs11/PKCS11Test.java

-        return path;
+    private static Path fetchNssLib(Class<?> clazz, Path libraryName) throws IOException {
+        Path p = ArtifactResolver.fetchOne(clazz);
+        return findNSSLibrary(p, libraryName);


So this method should never return null. Can we change fetchNssLib(String osId, Path libraryName) to also never return null (default throws SkippedException)? Then inside getNSSLibPath(String library) there is no need for the null check.

Yep, i removed the unnecessary null-checks

wangweij · 2025-03-12T20:31:58Z

test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java

import jtreg.SkippedException; is useless.

wangweij · 2025-03-12T20:33:39Z

test/jdk/sun/security/pkcs11/PKCS11Test.java

There are 2 useless imports now:

import java.nio.file.Paths; import jdk.test.lib.artifacts.ArtifactResolverException;

wangweij · 2025-03-12T20:34:40Z

test/jdk/sun/security/pkcs11/SecmodTest.java

Copyright year.

mpdonova · 2025-03-17T11:58:41Z

/integrate

openjdk · 2025-03-17T11:59:18Z

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

dbf47d6: 8351876: RISC-V: enable and fix some float round tests
d207ed3: 8352066: JVM.commit() and JVM.flush() exhibit race conditions against JFR epochs
0450ba9: 8351999: JFR: Incorrect scaling of throttled values
e5666f5: 8351976: assert(vthread_epoch == current_epoch) failed: invariant
2eecf15: 8351967: JFR: AnnotationIterator should handle num_annotations = 0
c8913d2: 8345555: Improve layout of search results
e29d405: 8352110: [BACKOUT] C2: Print compilation bailouts with PrintCompilation compile command
9f8d833: 8346194: Improve G1 pre-barrier C2 cost estimate
2672c40: 8351167: ZGC: Lazily initialize livemap
63bf791: 8351992: JFR: Improve robustness of the SettingControl examples
... and 63 more: https://git.openjdk.org/jdk/compare/0de2cddf3a7be23f67af93972875af1235f3107e...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-17T11:59:30Z

@mpdonova Pushed as commit e62becc.

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

8350964: Add an ArtifactResolver.fetch(clazz) method

43df679

openjdk bot added the rfr Pull request is ready for review label Mar 11, 2025

openjdk bot added the security security-dev@openjdk.org label Mar 11, 2025

myankelev reviewed Mar 11, 2025

View reviewed changes

fguallini reviewed Mar 11, 2025

View reviewed changes

Changed exception message in Artifact resolver and fixed logic in key…

83d479c

…tool test

wangweij reviewed Mar 12, 2025

View reviewed changes

Changed ArtifactResolver.fetchOne() to throw a skipped exception

4791105

wangweij reviewed Mar 12, 2025

View reviewed changes

updated code to remove extraneous null checks

c1b7973

wangweij reviewed Mar 12, 2025

View reviewed changes

fixed imports and copyright year

76f3b31

wangweij approved these changes Mar 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 17, 2025

openjdk bot closed this Mar 17, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 17, 2025

8350964: Add an ArtifactResolver.fetch(clazz) method #23989

8350964: Add an ArtifactResolver.fetch(clazz) method #23989

Uh oh!

Conversation

mpdonova commented Mar 11, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 11, 2025

Uh oh!

openjdk bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 11, 2025

Uh oh!

mlbridge bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

myankelev Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myankelev commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdonova commented Mar 11, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 11, 2025 •

edited

Loading

mlbridge bot commented Mar 11, 2025 •

edited

Loading

myankelev Mar 11, 2025 •

edited

Loading

wangweij Mar 12, 2025 •

edited

Loading