RFR JDK-8164639: Configure PKCS11 tests to use user-supplied NSS libraries
sha.jiang at oracle.com
sha.jiang at oracle.com
Wed Aug 15 07:22:26 UTC 2018
On 2018/8/15 15:14, Weijun Wang wrote:
> I see.
>
> One more question: Since the osMap content is not changed, is there a special reason you move the code from a static block into a static method?
If preferable NSS lib paths have been provided, it looks no need to
create that map.
Best regards,
John Jiang
>
> Thanks
> Max
>
>> On Aug 15, 2018, at 3:07 PM, sha.jiang at oracle.com wrote:
>>
>> On 2018/8/15 14:54, Weijun Wang wrote:
>>> I notice one behavior change in PKCS11Test.java.
>>>
>>> 693 private static String[] getNssLibPaths(String osId) {
>>> 694 String[] preferablePaths = getPreferableNssLibPaths(osId);
>>> 695 if (preferablePaths.length != 0) {
>>> 696 return preferablePaths;
>>> 697 } else {
>>> 698 return getOsMap().get(osId);
>>> 699 }
>>> 700 }
>>>
>>> If getPreferableNssLibPaths(osId) returns a non-empty array, it seems you will not look at osMap at all. This means if the system property is set but it does not contain NSS libraries you will not look into osMap as a fallback. Is this intended?
>> Yes.
>>
>> I assume user have specified the full libs. If the specified libs are not enough, that may be a user's mistake.
>> If it allows the tests work with custom libs and system libs, that may be confused.
>>
>> Best regards,
>> John Jiang
>>
>>> Thanks
>>> Max
>>>
>>>
>>>> On Aug 15, 2018, at 2:45 PM, sha.jiang at oracle.com wrote:
>>>>
>>>> Hi Max,
>>>> Thanks for your comments very much!
>>>>
>>>> Please review this version: http://cr.openjdk.java.net/~jjiang/8164639/werbrev.03/
>>>> All of your comments are addressed.
>>>>
>>>> Assume external developers have no JIB jar, the artifact resolving fails quickly. The tests will be skipped for this case.
>>>>
>>>> Best regards,
>>>> John Jiang
>>>>
>>>> On 2018/8/15 11:23, Weijun Wang wrote:
>>>>> Two comments on PKCS11Test.java:
>>>>>
>>>>> First,
>>>>>
>>>>> 865 private static String fetchNssLib(Class<?> clazz) {
>>>>> 866 try {
>>>>> 867 String path = ArtifactResolver.resolve(clazz).entrySet().stream()
>>>>> 868 .findAny().get().getValue() + File.separator + "nsslib"
>>>>> 869 + File.separator;
>>>>> 870 return path;
>>>>> 871 } catch (ArtifactResolverException e) {
>>>>> 872 throw new RuntimeException("Fetch artifact failed: " + clazz
>>>>> 873 + "\nPlease make sure the artifact is available "
>>>>> 874 + "and JIB jar is in the classpath", e);
>>>>> 875 }
>>>>> 876 }
>>>>>
>>>>> If an external developer is running this test and cannot download the artifact (either because there's no JIB jar or cannot access the server), will this test fail?
>>>>>
>>>>> If yes, this is not a good idea. I'm OK with the test succeed with a warning (even if no one notice it) but failure is bad.
>>>>>
>>>>> Second,
>>>>>
>>>>> 666 osMap.put("SunOS-sparc-32",
>>>>> 667 getNssLibPaths(new String[] { "/usr/lib/mps/" }));
>>>>> ...
>>>>>
>>>>> We needn't call getNssLibPaths() for every platform, especially, the fetchNssLib() action should only run on the current platform. We can simply add something after these existing lines
>>>>>
>>>>> 309 String osid = osName + "-"
>>>>> 310 + props.getProperty("os.arch") + "-" + props.getProperty("sun.arch.data.model");
>>>>> 311 String[] nssLibDirs = osMap.get(osid);
>>>>>
>>>>> For example,
>>>>>
>>>>> String[] nssLibDirs = expandDirs(osMap.get(osid));
>>>>>
>>>>> and let expandDirs() to do the test.nss.lib.paths search and artifact downloading.
>>>>>
>>>>> REAME is good, although I would like to see more blank lines.
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>> On Aug 15, 2018, at 10:13 AM, sha.jiang at oracle.com wrote:
>>>>>>
>>>>>> Thanks for the comments!
>>>>>> Please take a look the updated webrev: http://cr.openjdk.java.net/~jjiang/8164639/webrev.02/
>>>>>> Only README was adjusted.
>>>>>>
>>>>>> Best regards,
>>>>>> John Jiang
>>>>>> On 2018/8/14 23:48, Rajan Halade wrote:
>>>>>>> Few minor comments on README:
>>>>>>>
>>>>>>> - Please leave an empty line after each numbered section
>>>>>>> - I would suggest to update #2 to have general instruction on use of artifactory. Something like
>>>>>>>
>>>>>>> 2. Pre-built NSS libraries from artifactory server
>>>>>>> If the value of system property test.nss.lib.paths is null then tests will try
>>>>>>> to download pre-built NSS libraries from artifactory server.
>>>>>>> Currently, test only looks for libraries for Windows and MacOSX on artifactory.
>>>>>>> Please note that, JIB jar MUST be present in classpath when downloading the libraries.
>>>>>>>
>>>>>>> Other changes look good to me.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Rajan
>>>>>>>
>>>>>>> On 8/14/18 4:40 AM, sha.jiang at oracle.com wrote:
>>>>>>>> Hi Max,
>>>>>>>> Please review the new webrev: http://cr.openjdk.java.net/~jjiang/8164639/webrev.01/
>>>>>>>>
>>>>>>>> The new system property has been renamed to test.nss.lib.paths, and it supports multiple paths.
>>>>>>>> Currently, it cannot download the artifacts outside Oracle network. This affects the test executions on Windows and MacOSX.
>>>>>>>> I added a block to README for clarifying something on getting NSS libraries.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> John Jiang
>>>>>>>> On 2018/8/13 16:48, Weijun Wang wrote:
>>>>>>>>> Sorry, more questions:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On Aug 13, 2018, at 3:36 PM, sha.jiang at oracle.com
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Is there an artifact server available on the open internet?
>>>>>>>>>>>
>>>>>>>>>> It's transparent to me. @Artifact tool delegates the downloading.
>>>>>>>>>>
>>>>>>>>> Have you tried running the test outside Oracle?
>>>>>>>>>
>>>>>>>>> Have you tried submitting the change to Mach5 as a non-Oracle developer? (i.e. using submit-repo)
>>>>>>>>>
>>>>>>>>> While I am glad to see these files removed from the repo, I hope people still have a chance to run the tests.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Max
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>
More information about the security-dev
mailing list