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?
Thanks
Max
> On Aug 15, 2018, at 2:45 PM, sha.ji...@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.ji...@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.ji...@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.ji...@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
>>>>>>
>>>>>>
>>>>>>
>>
>
