On Fri, 13 Aug 2021 15:30:26 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Abdul Kolarkunnu has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8266182: Automate manual steps listed in the test
>> jdk/sun/security/pkcs12/ParamsTest.java
>
> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 91:
>
>> 89: opensslPath = OpensslArtifactFetcher.fetchOpenssl();
>> 90: }
>> 91: if (opensslPath != null && verifyOpensslVerion(opensslPath)) {
>
> The code here is a little different from your description above. If user
> provides a non-1.1.* openssl through `test.openssl.path` then you won't go
> look into elsewhere.
>
> Also, `getSystemOpensslPath()` and `OpensslArtifactFetcher.fetchOpenssl()`
> already ensure version is 1.1.* so the check above is duplicated.
Though I had kept it in intentional like that, to make sure to give proper
openssl path if user is supplying it explicitly.
Anyhow I changed it now.
1. Check whether property test.openssl.path is set and it's the
preferred version(1.1.*) of openssl, then return that path.
2. Else look for already installed openssl (version 1.1.*) in system
path /usr/bin/openssl or /usr/local/bin/openssl, then return that
path.
3. Else try to download openssl (version 1.1.*) from the artifactory
and return that path, if download fails then return null.
> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 614:
>
>> 612: }
>> 613:
>> 614: private static boolean verifyOpensslVerion(String path) {
>
> Oops, a typo in the method name. `s/Verion/Version/`.
corrected
> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 618:
>
>> 616: ProcessTools.executeCommand(path, "version")
>> 617: .shouldHaveExitValue(0)
>> 618: .shouldMatch("1.1.*");
>
> Precisely the regex should be `1\.1\..*`, or maybe you can simply call
> `.shouldContain("1.1.")`.
Changed it in to .shouldContain("1.1.")
> test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java line 24:
>
>> 22: */
>> 23:
>> 24: package jdk.test.lib.artifacts.openssl;
>
> The file is not in this package. Somehow I don't think if it's not worth
> creating a new package for it. Do you expect we will have more classes here?
ya, it was by mistake. I tested from different local workspace, that' why I
couldn't catch it in my testing.
Now moved to package jdk.test.lib.artifacts.
> test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java line 35:
>
>> 33: public class OpensslArtifactFetcher {
>> 34:
>> 35: public static String fetchOpenssl() {
>
> Can we rename this to `fetchOpenssl1.1.1`? Someday we will need to download
> other versions. If so, you can probably move `getSystemOpensslPath`,
> `verifyOpensslVersion`, and `System.getProperty("test.openssl.path")` here as
> well. It's quite likely that they will be used together.
Moved getSystemOpensslPath, verifyOpensslVersion and
System.getProperty("test.openssl.path") to OpensslArtifactFetcher class. And
renamed fetchOpenssl to getOpenssl1dot1dotStar.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4413
