8168149: Examine the behavior of jmod command-line options - repeating vs last one wins
Reviewed-by: mchung
--- a/jdk/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Wed Jan 11 19:36:11 2017 -0800
+++ b/jdk/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java Thu Jan 12 11:41:51 2017 +0000
@@ -1088,34 +1088,58 @@
}
}
- static class ClassPathConverter implements ValueConverter<Path> {
- static final ValueConverter<Path> INSTANCE = new ClassPathConverter();
+ /**
+ * An abstract converter that given a string representing a list of paths,
+ * separated by the File.pathSeparator, returns a List of java.nio.Path's.
+ * Specific subclasses should do whatever validation is required on the
+ * individual path elements, if any.
+ */
+ static abstract class AbstractPathConverter implements ValueConverter<List<Path>> {
+ @Override
+ public List<Path> convert(String value) {
+ List<Path> paths = new ArrayList<>();
+ String[] pathElements = value.split(File.pathSeparator);
+ for (String pathElement : pathElements) {
+ paths.add(toPath(pathElement));
+ }
+ return paths;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public Class<List<Path>> valueType() {
+ return (Class<List<Path>>)(Object)List.class;
+ }
+
+ @Override public String valuePattern() { return "path"; }
+
+ abstract Path toPath(String path);
+ }
+
+ static class ClassPathConverter extends AbstractPathConverter {
+ static final ValueConverter<List<Path>> INSTANCE = new ClassPathConverter();
@Override
- public Path convert(String value) {
+ public Path toPath(String value) {
try {
Path path = CWD.resolve(value);
if (Files.notExists(path))
throw new CommandException("err.path.not.found", path);
- if (! (Files.isDirectory(path) ||
- (Files.isRegularFile(path) && path.toString().endsWith(".jar"))))
+ if (!(Files.isDirectory(path) ||
+ (Files.isRegularFile(path) && path.toString().endsWith(".jar"))))
throw new CommandException("err.invalid.class.path.entry", path);
return path;
} catch (InvalidPathException x) {
throw new CommandException("err.path.not.valid", value);
}
}
-
- @Override public Class<Path> valueType() { return Path.class; }
-
- @Override public String valuePattern() { return "path"; }
}
- static class DirPathConverter implements ValueConverter<Path> {
- static final ValueConverter<Path> INSTANCE = new DirPathConverter();
+ static class DirPathConverter extends AbstractPathConverter {
+ static final ValueConverter<List<Path>> INSTANCE = new DirPathConverter();
@Override
- public Path convert(String value) {
+ public Path toPath(String value) {
try {
Path path = CWD.resolve(value);
if (Files.notExists(path))
@@ -1127,10 +1151,6 @@
throw new CommandException("err.path.not.valid", value);
}
}
-
- @Override public Class<Path> valueType() { return Path.class; }
-
- @Override public String valuePattern() { return "path"; }
}
static class ExtractDirPathConverter implements ValueConverter<Path> {
@@ -1142,12 +1162,6 @@
if (Files.exists(path)) {
if (!Files.isDirectory(path))
throw new CommandException("err.cannot.create.dir", path);
- } else {
- try {
- Files.createDirectories(path);
- } catch (IOException ioe) {
- throw new CommandException("err.cannot.create.dir", path);
- }
}
return path;
} catch (InvalidPathException x) {
@@ -1316,22 +1330,19 @@
options = new Options();
parser.formatHelpWith(new JmodHelpFormatter(options));
- OptionSpec<Path> classPath
+ OptionSpec<List<Path>> classPath
= parser.accepts("class-path", getMessage("main.opt.class-path"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(ClassPathConverter.INSTANCE);
- OptionSpec<Path> cmds
+ OptionSpec<List<Path>> cmds
= parser.accepts("cmds", getMessage("main.opt.cmds"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
- OptionSpec<Path> config
+ OptionSpec<List<Path>> config
= parser.accepts("config", getMessage("main.opt.config"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
OptionSpec<Path> dir
@@ -1359,22 +1370,19 @@
OptionSpec<Void> helpExtra
= parser.accepts("help-extra", getMessage("main.opt.help-extra"));
- OptionSpec<Path> headerFiles
+ OptionSpec<List<Path>> headerFiles
= parser.accepts("header-files", getMessage("main.opt.header-files"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
- OptionSpec<Path> libs
+ OptionSpec<List<Path>> libs
= parser.accepts("libs", getMessage("main.opt.libs"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
- OptionSpec<Path> legalNotices
+ OptionSpec<List<Path>> legalNotices
= parser.accepts("legal-notices", getMessage("main.opt.legal-notices"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
@@ -1383,17 +1391,15 @@
.withRequiredArg()
.describedAs(getMessage("main.opt.main-class.arg"));
- OptionSpec<Path> manPages
+ OptionSpec<List<Path>> manPages
= parser.accepts("man-pages", getMessage("main.opt.man-pages"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
- OptionSpec<Path> modulePath
+ OptionSpec<List<Path>> modulePath
= parser.acceptsAll(Set.of("p", "module-path"),
getMessage("main.opt.module-path"))
.withRequiredArg()
- .withValuesSeparatedBy(File.pathSeparatorChar)
.withValuesConvertedBy(DirPathConverter.INSTANCE);
OptionSpec<Version> moduleVersion
@@ -1452,48 +1458,48 @@
}
if (opts.has(classPath))
- options.classpath = opts.valuesOf(classPath);
+ options.classpath = getLastElement(opts.valuesOf(classPath));
if (opts.has(cmds))
- options.cmds = opts.valuesOf(cmds);
+ options.cmds = getLastElement(opts.valuesOf(cmds));
if (opts.has(config))
- options.configs = opts.valuesOf(config);
+ options.configs = getLastElement(opts.valuesOf(config));
if (opts.has(dir))
- options.extractDir = opts.valueOf(dir);
+ options.extractDir = getLastElement(opts.valuesOf(dir));
if (opts.has(dryrun))
options.dryrun = true;
if (opts.has(excludes))
- options.excludes = opts.valuesOf(excludes);
+ options.excludes = opts.valuesOf(excludes); // excludes is repeatable
if (opts.has(libs))
- options.libs = opts.valuesOf(libs);
+ options.libs = getLastElement(opts.valuesOf(libs));
if (opts.has(headerFiles))
- options.headerFiles = opts.valuesOf(headerFiles);
+ options.headerFiles = getLastElement(opts.valuesOf(headerFiles));
if (opts.has(manPages))
- options.manPages = opts.valuesOf(manPages);
+ options.manPages = getLastElement(opts.valuesOf(manPages));
if (opts.has(legalNotices))
- options.legalNotices = opts.valuesOf(legalNotices);
+ options.legalNotices = getLastElement(opts.valuesOf(legalNotices));
if (opts.has(modulePath)) {
- Path[] dirs = opts.valuesOf(modulePath).toArray(new Path[0]);
+ Path[] dirs = getLastElement(opts.valuesOf(modulePath)).toArray(new Path[0]);
options.moduleFinder = new ModulePath(Runtime.version(), true, dirs);
}
if (opts.has(moduleVersion))
- options.moduleVersion = opts.valueOf(moduleVersion);
+ options.moduleVersion = getLastElement(opts.valuesOf(moduleVersion));
if (opts.has(mainClass))
- options.mainClass = opts.valueOf(mainClass);
+ options.mainClass = getLastElement(opts.valuesOf(mainClass));
if (opts.has(osName))
- options.osName = opts.valueOf(osName);
+ options.osName = getLastElement(opts.valuesOf(osName));
if (opts.has(osArch))
- options.osArch = opts.valueOf(osArch);
+ options.osArch = getLastElement(opts.valuesOf(osArch));
if (opts.has(osVersion))
- options.osVersion = opts.valueOf(osVersion);
+ options.osVersion = getLastElement(opts.valuesOf(osVersion));
if (opts.has(warnIfResolved))
- options.moduleResolution = opts.valueOf(warnIfResolved);
+ options.moduleResolution = getLastElement(opts.valuesOf(warnIfResolved));
if (opts.has(doNotResolveByDefault)) {
if (options.moduleResolution == null)
options.moduleResolution = ModuleResolution.empty();
options.moduleResolution = options.moduleResolution.withDoNotResolveByDefault();
}
if (opts.has(hashModules)) {
- options.modulesToHash = opts.valueOf(hashModules);
+ options.modulesToHash = getLastElement(opts.valuesOf(hashModules));
// if storing hashes then the module path is required
if (options.moduleFinder == null)
throw new CommandException("err.modulepath.must.be.specified")
@@ -1531,6 +1537,13 @@
throw new CommandException("err.classpath.must.be.specified").showUsage(true);
if (options.mainClass != null && !isValidJavaIdentifier(options.mainClass))
throw new CommandException("err.invalid.main-class", options.mainClass);
+ if (options.mode.equals(Mode.EXTRACT) && options.extractDir != null) {
+ try {
+ Files.createDirectories(options.extractDir);
+ } catch (IOException ioe) {
+ throw new CommandException("err.cannot.create.dir", options.extractDir);
+ }
+ }
} catch (OptionException e) {
throw new CommandException(e.getMessage());
}
@@ -1558,6 +1571,12 @@
return true;
}
+ static <E> E getLastElement(List<E> list) {
+ if (list.size() == 0)
+ throw new InternalError("Unexpected 0 list size");
+ return list.get(list.size() - 1);
+ }
+
private void reportError(String message) {
out.println(getMessage("error.prefix") + " " + message);
}
--- a/jdk/test/tools/jmod/JmodTest.java Wed Jan 11 19:36:11 2017 -0800
+++ b/jdk/test/tools/jmod/JmodTest.java Thu Jan 12 11:41:51 2017 +0000
@@ -23,7 +23,7 @@
/*
* @test
- * @bug 8142968 8166568 8166286 8170618
+ * @bug 8142968 8166568 8166286 8170618 8168149
* @summary Basic test for jmod
* @library /lib/testlibrary
* @modules jdk.compiler
@@ -459,6 +459,76 @@
}
@Test
+ public void testLastOneWins() throws IOException {
+ Path workDir = Paths.get("lastOneWins");
+ if (Files.exists(workDir))
+ FileUtils.deleteFileTreeWithRetry(workDir);
+ Files.createDirectory(workDir);
+ Path jmod = MODS_DIR.resolve("lastOneWins.jmod");
+ FileUtils.deleteFileIfExistsWithRetry(jmod);
+ Path cp = EXPLODED_DIR.resolve("foo").resolve("classes");
+ Path bp = EXPLODED_DIR.resolve("foo").resolve("bin");
+ Path lp = EXPLODED_DIR.resolve("foo").resolve("lib");
+ Path cf = EXPLODED_DIR.resolve("foo").resolve("conf");
+
+ Path shouldNotBeAdded = workDir.resolve("shouldNotBeAdded");
+ Files.createDirectory(shouldNotBeAdded);
+ Files.write(shouldNotBeAdded.resolve("aFile"), "hello".getBytes(UTF_8));
+
+ // Pairs of options. For options with required arguments the last one
+ // should win ( first should be effectively ignored, but may still be
+ // validated ).
+ jmod("create",
+ "--conf", shouldNotBeAdded.toString(),
+ "--conf", cf.toString(),
+ "--cmds", shouldNotBeAdded.toString(),
+ "--cmds", bp.toString(),
+ "--libs", shouldNotBeAdded.toString(),
+ "--libs", lp.toString(),
+ "--class-path", shouldNotBeAdded.toString(),
+ "--class-path", cp.toString(),
+ "--main-class", "does.NotExist",
+ "--main-class", "jdk.test.foo.Foo",
+ "--module-version", "00001",
+ "--module-version", "5.4.3",
+ "--do-not-resolve-by-default",
+ "--do-not-resolve-by-default",
+ "--warn-if-resolved=incubating",
+ "--warn-if-resolved=deprecated",
+ MODS_DIR.resolve("lastOneWins.jmod").toString())
+ .assertSuccess()
+ .resultChecker(r -> {
+ ModuleDescriptor md = getModuleDescriptor(jmod);
+ Optional<String> omc = md.mainClass();
+ assertTrue(omc.isPresent());
+ assertEquals(omc.get(), "jdk.test.foo.Foo");
+ Optional<Version> ov = md.version();
+ assertTrue(ov.isPresent());
+ assertEquals(ov.get().toString(), "5.4.3");
+
+ try (Stream<String> s1 = findFiles(lp).map(p -> LIBS_PREFIX + p);
+ Stream<String> s2 = findFiles(cp).map(p -> CLASSES_PREFIX + p);
+ Stream<String> s3 = findFiles(bp).map(p -> CMDS_PREFIX + p);
+ Stream<String> s4 = findFiles(cf).map(p -> CONFIGS_PREFIX + p)) {
+ Set<String> expectedFilenames = Stream.concat(Stream.concat(s1,s2),
+ Stream.concat(s3, s4))
+ .collect(toSet());
+ assertJmodContent(jmod, expectedFilenames);
+ }
+ });
+
+ jmod("extract",
+ "--dir", "blah",
+ "--dir", "lastOneWinsExtractDir",
+ jmod.toString())
+ .assertSuccess()
+ .resultChecker(r -> {
+ assertTrue(Files.exists(Paths.get("lastOneWinsExtractDir")));
+ assertTrue(Files.notExists(Paths.get("blah")));
+ });
+ }
+
+ @Test
public void testPackagesAttribute() throws IOException {
Path jmod = MODS_DIR.resolve("foo.jmod");
FileUtils.deleteFileIfExistsWithRetry(jmod);