From dbaf0bf69dc70004eb1a6954f89b70e041d6cf52 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 3 Mar 2020 13:51:33 -0600 Subject: [PATCH 1/2] Issue #4628 - Ensuring checkEnabledModules is required dependency aware Signed-off-by: Joakim Erdfelt --- .../java/org/eclipse/jetty/start/Modules.java | 22 ++++++++++--------- .../org/eclipse/jetty/start/ModulesTest.java | 3 +++ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java index bc33e279828..4060d6cd6c3 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java @@ -487,17 +487,19 @@ public class Modules implements Iterable _modules.stream().filter(Module::isEnabled).forEach(m -> { // Check dependencies - m.getDepends().forEach(d -> - { - Set providers = getAvailableProviders(d); - if (providers.stream().filter(Module::isEnabled).count() == 0) + m.getDepends().stream() + .filter(Module::isRequiredDependency) + .forEach(d -> { - if (unsatisfied.length() > 0) - unsatisfied.append(','); - unsatisfied.append(m.getName()); - StartLog.error("Module %s requires a module providing %s from one of %s%n", m.getName(), d, providers); - } - }); + Set providers = getAvailableProviders(d); + if (providers.stream().noneMatch(Module::isEnabled)) + { + if (unsatisfied.length() > 0) + unsatisfied.append(','); + unsatisfied.append(m.getName()); + StartLog.error("Module %s requires a module providing %s from one of %s%n", m.getName(), d, providers); + } + }); }); if (unsatisfied.length() > 0) diff --git a/jetty-start/src/test/java/org/eclipse/jetty/start/ModulesTest.java b/jetty-start/src/test/java/org/eclipse/jetty/start/ModulesTest.java index 461351ceea5..68dbe4a3de0 100644 --- a/jetty-start/src/test/java/org/eclipse/jetty/start/ModulesTest.java +++ b/jetty-start/src/test/java/org/eclipse/jetty/start/ModulesTest.java @@ -234,6 +234,7 @@ public class ModulesTest // Collect active module list List active = modules.getEnabled(); + modules.checkEnabledModules(); // Assert names are correct, and in the right order List expectedNames = new ArrayList<>(); @@ -282,6 +283,7 @@ public class ModulesTest // Collect active module list List active = modules.getEnabled(); + modules.checkEnabledModules(); // Assert names are correct, and in the right order List expectedNames = new ArrayList<>(); @@ -331,6 +333,7 @@ public class ModulesTest // Collect active module list List active = modules.getEnabled(); + modules.checkEnabledModules(); // Assert names are correct, and in the right order List expectedNames = new ArrayList<>(); From 3fcc2edeea526919ee3dd2cc4b442a916b2366ad Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 19 Mar 2020 10:08:32 +0100 Subject: [PATCH 2/2] Issue #4628 - Add support for conditional module dependencies in jetty-start. Updated after review. Renamed isRequiredModule() -> isConditionalModule() and inverted expressions that were using the method. Signed-off-by: Simone Bordet --- .../java/org/eclipse/jetty/start/Module.java | 6 ++-- .../java/org/eclipse/jetty/start/Modules.java | 28 ++++++++----------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java index fbd4bfc39f1..0d7b8088117 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Module.java @@ -195,14 +195,14 @@ public class Module implements Comparable process(basehome); } - public static boolean isRequiredDependency(String depends) + public static boolean isConditionalDependency(String depends) { - return (depends != null) && (depends.charAt(0) != '?'); + return (depends != null) && (depends.charAt(0) == '?'); } public static String normalizeModuleName(String name) { - if (!isRequiredDependency(name)) + if (isConditionalDependency(name)) return name.substring(1); return name; } diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java index 4060d6cd6c3..7eb7fc802b5 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java @@ -91,8 +91,8 @@ public class Modules implements Iterable _modules.stream() .filter(m -> { - boolean included = all || m.getTags().stream().anyMatch(t -> include.contains(t)); - boolean excluded = m.getTags().stream().anyMatch(t -> exclude.contains(t)); + boolean included = all || m.getTags().stream().anyMatch(include::contains); + boolean excluded = m.getTags().stream().anyMatch(exclude::contains); return included && !excluded; }) .sorted() @@ -135,9 +135,9 @@ public class Modules implements Iterable { parent = Module.normalizeModuleName(parent); System.out.printf(label, parent); - if (!Module.isRequiredDependency(parent)) + if (Module.isConditionalDependency(parent)) { - System.out.print(" [not-required]"); + System.out.print(" [conditional]"); } label = ", %s"; } @@ -219,11 +219,7 @@ public class Modules implements Iterable Module module = new Module(_baseHome, file); _modules.add(module); _names.put(module.getName(), module); - module.getProvides().forEach(n -> - { - _provided.computeIfAbsent(n, k -> new HashSet()).add(module); - }); - + module.getProvides().forEach(n -> _provided.computeIfAbsent(n, k -> new HashSet<>()).add(module)); return module; } catch (Error | RuntimeException t) @@ -258,7 +254,7 @@ public class Modules implements Iterable public List getEnabled() { - List enabled = _modules.stream().filter(m -> m.isEnabled()).collect(Collectors.toList()); + List enabled = _modules.stream().filter(Module::isEnabled).collect(Collectors.toList()); TopologicalSort sort = new TopologicalSort<>(); for (Module module : enabled) @@ -360,7 +356,7 @@ public class Modules implements Iterable StartLog.debug("Enabled module %s depends on %s", module.getName(), module.getDepends()); for (String dependsOnRaw : module.getDepends()) { - boolean isRequired = Module.isRequiredDependency(dependsOnRaw); + boolean isConditional = Module.isConditionalDependency(dependsOnRaw); // Final to allow lambda's below to use name final String dependentModule = Module.normalizeModuleName(dependsOnRaw); @@ -376,7 +372,7 @@ public class Modules implements Iterable if (dependentModule.contains("/")) { Path file = _baseHome.getPath("modules/" + dependentModule + ".mod"); - if (isRequired || Files.exists(file)) + if (!isConditional || Files.exists(file)) { registerModule(file).expandDependencies(_args.getProperties()); providers = _provided.get(dependentModule); @@ -387,10 +383,10 @@ public class Modules implements Iterable continue; } } - // is this a non-required module - if (!isRequired) + // is this a conditional module + if (isConditional) { - StartLog.debug("Skipping non-required module [%s]: doesn't exist", dependentModule); + StartLog.debug("Skipping conditional module [%s]: it does not exist", dependentModule); continue; } // throw an exception (not a dynamic module and a required dependency) @@ -488,7 +484,7 @@ public class Modules implements Iterable { // Check dependencies m.getDepends().stream() - .filter(Module::isRequiredDependency) + .filter(depends -> !Module.isConditionalDependency(depends)) .forEach(d -> { Set providers = getAvailableProviders(d);