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?
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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
>
