Good. I have no other comment.
Thanks
Max
> On Aug 15, 2018, at 3:22 PM, sha.ji...@oracle.com wrote:
>
> 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.ji...@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.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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>
>
