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
