diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java index 7aa8a6202f36..9d1a50d27e46 100644 --- a/core/src/main/java/hudson/PluginManager.java +++ b/core/src/main/java/hudson/PluginManager.java @@ -489,7 +489,8 @@ public void dynamicLoad(File arc) throws IOException, InterruptedException, Rest // so existing plugins can't be depending on this newly deployed one. plugins.add(p); - activePlugins.add(p); + if (p.isActive()) + activePlugins.add(p); synchronized (((UberClassLoader) uberClassLoader).loaded) { ((UberClassLoader) uberClassLoader).loaded.clear(); } diff --git a/core/src/main/java/hudson/PluginWrapper.java b/core/src/main/java/hudson/PluginWrapper.java index 60eabf68cf50..71cdf14096b4 100644 --- a/core/src/main/java/hudson/PluginWrapper.java +++ b/core/src/main/java/hudson/PluginWrapper.java @@ -87,6 +87,12 @@ */ @ExportedBean public class PluginWrapper implements Comparable, ModelObject { + /** + * A plugin won't be loaded unless his declared dependencies are present and match the required minimal version. + * This can be set to false to disable the version check (legacy behaviour) + */ + public static final boolean ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK = Boolean.parseBoolean(System.getProperty(PluginWrapper.class.getName()+"." + "dependenciesVersionCheck.enabled", "true")); + /** * {@link PluginManager} to which this belongs to. */ @@ -217,10 +223,10 @@ public Dependency(String s) { if(idx==-1) throw new IllegalArgumentException("Illegal dependency specifier "+s); this.shortName = s.substring(0,idx); - this.version = s.substring(idx+1); - + String version = s.substring(idx+1); + boolean isOptional = false; - String[] osgiProperties = s.split(";"); + String[] osgiProperties = version.split("[;]"); for (int i = 1; i < osgiProperties.length; i++) { String osgiProperty = osgiProperties[i].trim(); if (osgiProperty.equalsIgnoreCase("resolution:=optional")) { @@ -228,11 +234,16 @@ public Dependency(String s) { } } this.optional = isOptional; + if (isOptional) { + this.version = osgiProperties[0]; + } else { + this.version = version; + } } @Override public String toString() { - return shortName + " (" + version + ")"; + return shortName + " (" + version + ") " + (optional ? "optional" : ""); } } @@ -524,18 +535,60 @@ public boolean hasLicensesXml() { */ /*package*/ void resolvePluginDependencies() throws IOException { List missingDependencies = new ArrayList(); + List obsoleteDependencies = new ArrayList(); + List disabledDependencies = new ArrayList(); // make sure dependencies exist for (Dependency d : dependencies) { - if (parent.getPlugin(d.shortName) == null) + PluginWrapper dependency = parent.getPlugin(d.shortName); + if (dependency == null) { missingDependencies.add(d.toString()); - } - if (!missingDependencies.isEmpty()) - throw new IOException("Dependency "+Util.join(missingDependencies, ", ")+" doesn't exist"); + } else { + if (dependency.isActive()) { + if (ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK && dependency.getVersionNumber().isOlderThan(new VersionNumber(d.version))) { + obsoleteDependencies.add(dependency.getShortName() + "(" + dependency.getVersion() + " < " + d.version + ")"); + } + } else { + disabledDependencies.add(d.toString()); + } + } + } // add the optional dependencies that exists for (Dependency d : optionalDependencies) { - if (parent.getPlugin(d.shortName) != null) - dependencies.add(d); + PluginWrapper dependency = parent.getPlugin(d.shortName); + if (dependency != null && dependency.isActive()) { + if (ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK && dependency.getVersionNumber().isOlderThan(new VersionNumber(d.version))) { + obsoleteDependencies.add(dependency.getShortName() + "(" + dependency.getVersion() + " < " + d.version + ")"); + } else { + dependencies.add(d); + } + } + } + StringBuilder messageBuilder = new StringBuilder(); + if (!missingDependencies.isEmpty()) { + boolean plural = missingDependencies.size() > 1; + messageBuilder.append(plural ? "Dependencies " : "Dependency ") + .append(Util.join(missingDependencies, ", ")) + .append(" ").append(plural ? "don't" : "doesn't") + .append(" exist. "); + } + if (!disabledDependencies.isEmpty()) { + boolean plural = disabledDependencies.size() > 1; + messageBuilder.append(plural ? "Dependencies " : "Dependency ") + .append(Util.join(missingDependencies, ", ")) + .append(" ").append(plural ? "are" : "is") + .append(" disabled. "); + } + if (!obsoleteDependencies.isEmpty()) { + boolean plural = obsoleteDependencies.size() > 1; + messageBuilder.append(plural ? "Dependencies " : "Dependency ") + .append(Util.join(obsoleteDependencies, ", ")) + .append(" ").append(plural ? "are" : "is") + .append(" older than required."); + } + String message = messageBuilder.toString(); + if (!message.isEmpty()) { + throw new IOException(message); } } diff --git a/test/src/test/java/hudson/PluginManagerTest.java b/test/src/test/java/hudson/PluginManagerTest.java index 282bc9f779b9..02e3e21bd28f 100644 --- a/test/src/test/java/hudson/PluginManagerTest.java +++ b/test/src/test/java/hudson/PluginManagerTest.java @@ -45,6 +45,7 @@ import org.apache.commons.io.FileUtils; import org.apache.tools.ant.filters.StringInputStream; import static org.junit.Assert.*; + import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -337,6 +338,75 @@ private String callDependerValue() throws Exception { assertTrue(r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint").isEmpty()); } + @Issue("JENKINS-21486") + @Test public void installPluginWithObsoleteDependencyFails() throws Exception { + // Load dependee 0.0.1. + { + dynamicLoad("dependee.hpi"); + } + + // Load mandatory-depender 0.0.2, depending on dependee 0.0.2 + try { + dynamicLoad("mandatory-depender-0.0.2.hpi"); + fail("Should not have worked"); + } catch (IOException e) { + // Expected + } + } + + @Issue("JENKINS-21486") + @Test public void installPluginWithDisabledOptionalDependencySucceeds() throws Exception { + // Load dependee 0.0.2. + { + dynamicLoadAndDisable("dependee-0.0.2.hpi"); + } + + // Load depender 0.0.2, depending optionally on dependee 0.0.2 + { + dynamicLoad("depender-0.0.2.hpi"); + } + + // dependee is not loaded so we cannot list any extension for it. + try { + r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint"); + fail(); + } catch( ClassNotFoundException _ ){ + } + } + + @Issue("JENKINS-21486") + @Test public void installPluginWithDisabledDependencyFails() throws Exception { + // Load dependee 0.0.2. + { + dynamicLoadAndDisable("dependee-0.0.2.hpi"); + } + + // Load mandatory-depender 0.0.2, depending on dependee 0.0.2 + try { + dynamicLoad("mandatory-depender-0.0.2.hpi"); + fail("Should not have worked"); + } catch (IOException e) { + // Expected + } + } + + + @Issue("JENKINS-21486") + @Test public void installPluginWithObsoleteOptionalDependencyFails() throws Exception { + // Load dependee 0.0.1. + { + dynamicLoad("dependee.hpi"); + } + + // Load depender 0.0.2, depending optionally on dependee 0.0.2 + try { + dynamicLoad("depender-0.0.2.hpi"); + fail("Should not have worked"); + } catch (IOException e) { + // Expected + } + } + @Issue("JENKINS-12753") @WithPlugin("tasks.jpi") @Test public void dynamicLoadRestartRequiredException() throws Exception { @@ -358,4 +428,8 @@ private String callDependerValue() throws Exception { private void dynamicLoad(String plugin) throws IOException, InterruptedException, RestartRequiredException { PluginManagerUtil.dynamicLoad(plugin, r.jenkins); } + + private void dynamicLoadAndDisable(String plugin) throws IOException, InterruptedException, RestartRequiredException { + PluginManagerUtil.dynamicLoad(plugin, r.jenkins, true); + } } diff --git a/test/src/test/java/hudson/PluginManagerUtil.java b/test/src/test/java/hudson/PluginManagerUtil.java index e67b9b31f9f3..2263622b4c44 100644 --- a/test/src/test/java/hudson/PluginManagerUtil.java +++ b/test/src/test/java/hudson/PluginManagerUtil.java @@ -48,9 +48,16 @@ public void before() throws Throwable { } public static void dynamicLoad(String plugin, Jenkins jenkins) throws IOException, InterruptedException, RestartRequiredException { + dynamicLoad(plugin, jenkins, false); + } + + public static void dynamicLoad(String plugin, Jenkins jenkins, boolean disable) throws IOException, InterruptedException, RestartRequiredException { URL src = PluginManagerTest.class.getClassLoader().getResource("plugins/" + plugin); File dest = new File(jenkins.getRootDir(), "plugins/" + plugin); FileUtils.copyURLToFile(src, dest); + if (disable) { + new File(dest.getPath() + ".disabled").createNewFile(); + } jenkins.pluginManager.dynamicLoad(dest); } } diff --git a/test/src/test/java/hudson/PluginWrapperTest.java b/test/src/test/java/hudson/PluginWrapperTest.java new file mode 100644 index 000000000000..7529c5319495 --- /dev/null +++ b/test/src/test/java/hudson/PluginWrapperTest.java @@ -0,0 +1,26 @@ +package hudson; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class PluginWrapperTest { + + @Test + public void dependencyTest() { + String version = "plugin:0.0.2"; + PluginWrapper.Dependency dependency = new PluginWrapper.Dependency(version); + assertEquals("plugin", dependency.shortName); + assertEquals("0.0.2", dependency.version); + assertEquals(false, dependency.optional); + } + + @Test + public void optionalDependencyTest() { + String version = "plugin:0.0.2;resolution:=optional"; + PluginWrapper.Dependency dependency = new PluginWrapper.Dependency(version); + assertEquals("plugin", dependency.shortName); + assertEquals("0.0.2", dependency.version); + assertEquals(true, dependency.optional); + } +} diff --git a/test/src/test/resources/plugins/dependee-0.0.2.hpi b/test/src/test/resources/plugins/dependee-0.0.2.hpi new file mode 100644 index 000000000000..79525b08464f Binary files /dev/null and b/test/src/test/resources/plugins/dependee-0.0.2.hpi differ diff --git a/test/src/test/resources/plugins/depender-0.0.2.hpi b/test/src/test/resources/plugins/depender-0.0.2.hpi new file mode 100644 index 000000000000..34826d3f7707 Binary files /dev/null and b/test/src/test/resources/plugins/depender-0.0.2.hpi differ diff --git a/test/src/test/resources/plugins/mandatory-depender-0.0.2.hpi b/test/src/test/resources/plugins/mandatory-depender-0.0.2.hpi new file mode 100644 index 000000000000..dedc65a62f99 Binary files /dev/null and b/test/src/test/resources/plugins/mandatory-depender-0.0.2.hpi differ