Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-21486] Fix plugin dependencies resolution #2172

Merged
merged 4 commits into from Apr 4, 2016

Conversation

7 participants
@Vlatombe
Copy link
Member

commented Mar 25, 2016

https://issues.jenkins-ci.org/browse/JENKINS-21486

Also fixes https://issues.jenkins-ci.org/browse/JENKINS-28955 which is a consequence.

  • Check core version. If the current core is older than required, the plugin isn't loaded.
  • Check that dependencies are enabled. A disabled optional dependency
    will not prevent a plugin from loading.
  • Check versions of dependencies declared by a plugin before loading it.
    If any dependency (even optional) is older than what is required,
    then the plugin isn't loaded.

This should prevent use cases where a plugin is loaded but one of its
dependencies is too old so that :

  • its @extension annotated classes cannot be loaded, causing the full
    Jenkins to blow up with crapload of exceptions which are tedious to
    investigate to understand the root cause.
  • NoSuchMethodError and the likes at runtime even though boot has
    completed.

Version check (for setups where version list is manually crafted but yet
works) can be disabled by starting Jenkins with

-Dhudson.PluginWrapper.dependenciesVersionCheck.enabled=true

Minor fixes done while implementing this change :

  • Fix version parsing in PluginWrapper.Dependency
  • Dynamic plugin load didn't check for disabled flag
@reviewbybees

This comment has been minimized.

Copy link

commented Mar 25, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2016

Will add a test shortly

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2016

@@ -29,6 +29,7 @@
import hudson.model.Api;
import hudson.model.ModelObject;
import jenkins.YesNoMaybe;
import jenkins.model.Configuration;

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 25, 2016

Member

That doesn't seem to be a great choice for configuration options outside Jenkins itself.

messageBuilder.append(plural ? "Dependencies " : "Dependency ")
.append(Util.join(obsoleteDependencies, ", "))
.append(" ").append(plural ? "are" : "is")
.append(" older than required.").append(" To disable the version check, use -D")

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 25, 2016

Member

Not sure this should be advertised so aggressively. We may well remove support for it at some point.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 25, 2016

Other than the choice of system property, seems 👍

@Vlatombe Vlatombe force-pushed the Vlatombe:JENKINS-28955 branch 2 times, most recently from 2cb5dc2 to 6cbcbc7 Mar 26, 2016

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2016

Updated and squashed.

@Vlatombe Vlatombe closed this Mar 26, 2016

@Vlatombe Vlatombe reopened this Mar 26, 2016

@batmat

This comment has been minimized.

Copy link
Member

commented Mar 26, 2016

👍

@@ -87,6 +87,7 @@
*/
@ExportedBean
public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
public static final boolean DISABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK = Boolean.getBoolean(PluginWrapper.class.getName()+"." + "disablePluginDependenciesVersionCheck");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 26, 2016

Member

NIT: Missing javadoc

This comment has been minimized.

Copy link
@jglick

jglick Mar 28, 2016

Member

Prefer to use positive, not negative, sense on options. Thus, a property which refers to enabling the version check, defaulting to true.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 26, 2016

I would definitely like to see a test case, which proves the loading does not blow up on optional dependencies. I'm also aware about bundling prebuilt HPI w/o sources.

Thanks for implementing it in any case! I think it makes sense to integrate it before 2.0

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

Does this supersede #2001? And probably useful to combine with #2098?

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

Long overdue, but I think you forgot the active check from #2001.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2016

@jglick : IMO both #2002 (supercedes #2001, although I don't really get what was the pushback on this one) and #2098 are complementary to this PR.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2016

Note: I didn't change the behaviour w.r.t optional dependencies. If an optional dependency is present but with an obsolete version, the plugin will be loaded anyway.

@Vlatombe Vlatombe closed this Mar 29, 2016

@Vlatombe Vlatombe reopened this Mar 29, 2016

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

That optional dependency behavior would be a bug. If the dependency is present, depending code will assume it can link against it, and will throw LinkageErrors otherwise.

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

So 🐛 for:

  • failing to verify that a dependency is not just installed, but enabled
  • failing to verify version of an enabled optional dependency
@jglick

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Would love to see this in 2.0 by the way.

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

BTW I think it would be better to link this to the original issue, JENKINS-21486.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

@jglick If this gets merged to master before the RC it will be in 2.0.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2016

I've added active check, and version check for optional dependencies, however it doesn't change anything regarding classloading : the classloader is created even before PluginWrapper instance is created, and it contains all dependencies including optional ones.

As I don't want to disable the plugin if an optional dependency is obsolete, then with the current state of code this optional dependency cannot be excluded from its classloader easily.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2016

PluginManager#installPluginWithObsoleteOptionalDependencyCannotAccessIt fails but would require rework of https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/ClassicPluginStrategy.java#L251 to work

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

I don't want to disable the plugin if an optional dependency is obsolete

Why not? You should! Consider plugin A 1.0

public abstract class X implements ExtensionPoint {
  public abstract void run();
}

and 1.1

public abstract class X implements ExtensionPoint {
  public abstract void run();
  protected void prepare() {}
}

and a plugin B declaring an optional dependency on A 1.1:

@Extension(optional=true) public class Y extends X {
  @Override public void run() {prepare();}
}

As I understand it, you would allow B to be loaded with A 1.0. This will cause Y to be included in the extension list, but Y.run will throw NoSuchMethodError.

I am not grasping for hypothetical examples. All sorts of plugins have optional dependencies which rely heavily on particular APIs introduced in particular versions of those dependencies. The “optional” part only refers to the fact that the dependee can cleanly turn off the entire block of functionality if the dependency is missing. But if it seems to be present, everything will be included, and it had better not throw errors.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2016

@jglick fine, in that case i'll just make it fail ;)

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

Oh, one important thing that has been totally forgotten here: parsing Jenkins-Version (or, if unset, Hudson-Version) from the manifest. If that is newer than Jenkins.getVersion(), resolvePluginDependencies should fail.

@Vlatombe Vlatombe force-pushed the Vlatombe:JENKINS-28955 branch from 03369d8 to 57e73a3 Apr 1, 2016

@Vlatombe Vlatombe closed this Apr 1, 2016

@Vlatombe Vlatombe reopened this Apr 1, 2016

@@ -534,6 +549,14 @@ public boolean hasLicensesXml() {
* thrown if one or several mandatory dependencies doesn't exists.
*/
/*package*/ void resolvePluginDependencies() throws IOException {
String requiredCoreVersion = getRequiredCoreVersion();
if (requiredCoreVersion == null) {
throw new IOException(shortName + " doesn't declare required core version.");

This comment has been minimized.

Copy link
@jglick

jglick Apr 1, 2016

Member

Hmm, not sure if this is even legal, but for the sake of compatibility perhaps this should be demoted to a warning?

if (requiredCoreVersion == null) {
throw new IOException(shortName + " doesn't declare required core version.");
} else {
if (Jenkins.getVersion().isOlderThan(new VersionNumber(requiredCoreVersion))) {

This comment has been minimized.

Copy link
@jglick

jglick Apr 1, 2016

Member

Should be made conditional on ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK I suppose.

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

Not sure what is going on with the CI build—looks like tests passed and then the JVM just got stuck.

@Vlatombe

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2016

Yes, happened twice; don't know why.

I'll fix your latest review items.

[JENKINS-21486] Check Jenkins version if the flag is set
And don't fail if no version can be found.
* 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"));

This comment has been minimized.

Copy link
@jglick

jglick Apr 1, 2016

Member

Could be private.

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

🐝

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 3, 2016

🐝

@jglick jglick merged commit f4e9cb4 into jenkinsci:master Apr 4, 2016

1 check passed

Jenkins This pull request looks good
Details

@batmat batmat assigned batmat and unassigned batmat Apr 4, 2016

jglick added a commit to jglick/workflow-cps-plugin that referenced this pull request Apr 6, 2016

Fix test-scoped deps.
Otherwise with jenkinsci/jenkins#2172 plugins will (correctly) be disabled due to version mismatches.

@jglick jglick referenced this pull request Apr 6, 2016

Merged

Fix test-scoped deps #1

daniel-beck added a commit to daniel-beck/jenkins that referenced this pull request Apr 6, 2016

Revert "[FIXED JENKINS-21486] Merged jenkinsci#2172: enforce plugin d…
…ependencies."

This reverts commit fa39668, reversing
changes made to be9bc0d.

daniel-beck added a commit to daniel-beck/jenkins that referenced this pull request Apr 20, 2016

Revert "[FIXED JENKINS-21486] Merged jenkinsci#2172: enforce plugin d…
…ependencies."

This reverts commit fa39668, reversing
changes made to be9bc0d.

kohsuke added a commit that referenced this pull request Apr 20, 2016

Merge pull request #2278 from daniel-beck/1.x-JENKINS-34073
Revert "[FIXED JENKINS-21486] Merged #2172: enforce plugin dependenci…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.