Good. I have no other comment. Thanks Max
> On Aug 15, 2018, at 3:22 PM, [email protected] 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, [email protected] 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, [email protected] 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, [email protected] 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, [email protected] 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, [email protected] >>>>>>>>>>> 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 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >> >
