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-33296] - plugin dependency issues during install #2122

Merged
merged 5 commits into from Mar 16, 2016

Conversation

6 participants
@kzantow
Copy link
Contributor

commented Mar 14, 2016

This addresses a few issues with dependent plugins:

  • previously "implied" plugin dependencies are not available without explicit dependency references
  • maven-plugin was being explicitly excluded as a dependency, causing many failures
  • mailer had explicit dependencies in the main codebase for functionality, including saving the 'email' address field for Jenkins user accounts
  • dependencies were included in client-side search text in the installer

This addresses:
https://issues.jenkins-ci.org/browse/JENKINS-33296
https://issues.jenkins-ci.org/browse/JENKINS-33308

@jenkinsci/code-reviewers

public boolean isMailerPluginPresent() {
try {
// mail support has moved to a separate plugin
return null != Jenkins.getInstance().pluginManager.uberClassLoader.loadClass("hudson.tasks.Mailer$UserProperty");

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 14, 2016

Member

Wouldn't it be easier to query the plugin manager for mailer? But probably safer this way…

This comment has been minimized.

Copy link
@slide

slide Mar 14, 2016

Member

I wish I had never moved this UserProperty out of core...

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

🐜 loadClass cannot ever return null, so you need not and should not inspect the return value.

try {
    Jenkins.getInstance().pluginManager.uberClassLoader.loadClass("hudson.tasks.Mailer$UserProperty");
    return true;
} catch (ClassNotFoundException e) {
    return false;
}

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

I wish I had never moved this UserProperty out of core...

Creating files like Mailer$UserProperty.java is technically legal, but pretty messy! Anyway might cause linkage errors now (mismatch in outer class code attribute; cannot remember if this applies only to inner classes or also to nested classes).

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

@jglick right, most of the changes here were just from refactoring

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 14, 2016

For reference, while this fixes Maven Plugin related issues, it does not take care of JENKINS-33308 type dependency problems.

@kzantow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2016

@daniel-beck right, it's only a part of the issue

JENKINS-33308 - automatically install previously bundled dependencies
for plugins that depend on old Jenkins versions
// TODO: Someone that understands what the following logic is about, please add a comment.
// There could be cases like:
// 'plugin.ambiguous.updatesite' where both
// 'plugin' @ 'ambigiuous.updatesite' and 'plugin.ambiguous' @ 'updatesite' resolve to valid plugins

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

Unrelated, but no need for a TODO here

This comment has been minimized.

Copy link
@kwhetstone

kwhetstone Mar 15, 2016

Hey, adding doc is always welcome.

@kzantow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 15, 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.

@kohsuke kohsuke changed the title JENKINS-33296 - plugin dependency issues during install [JENKINS-33296] - plugin dependency issues during install Mar 15, 2016

@@ -19,7 +19,7 @@ exports.recommendedPlugins = [
"gradle",
"ldap",
"mailer",
// "matrix-auth",
"matrix-auth",

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

Effectively reverting 7f87486 I suppose.

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

Yes, since the issues are 'fixed'

/**
* Returns all the bundled plugin dependencies for a particular Jenkins version
*/
public static List<PluginWrapper.Dependency> getPreviouslyBundledDependencies(String jenkinsVersion) {

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

🐜 the naming is misleading. Whether these were bundled in 1.x or not is irrelevant—only whether they were detached from core. (There are some bundled plugins which were never detached, and we could have unbundled detached plugins though as a matter of policy I think we never did.)

List<PluginWrapper.Dependency> out = new ArrayList<>();
for (DetachedPlugin detached : DETACHED_LIST) {
if (jenkinsVersion == null || jenkinsVersion.equals("null") || new VersionNumber(jenkinsVersion).compareTo(detached.splitWhen) <= 0) {
out.add(new PluginWrapper.Dependency(detached.shortName + ':' + detached.requireVersion));

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

🐛 reuse DetachedPlugin.fix, not only to avoid duplication but so you can honor BREAK_CYCLES.

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

(probably after refactoring that method to accept String yourName, String jenkinsVersion rather than Attributes atts)

if (p == null) {
throw new Failure("No such plugin: " + n);
}

// JENKINS-33308 - automatically install implied/previously bundled dependencies for older plugins that may need them
for(PluginWrapper.Dependency previouslyBundledDependency : ClassicPluginStrategy.getPreviouslyBundledDependencies(p.requiredCore)) {

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

🐛 This smells like the wrong place to be doing this. The detached plugin mapping should be logically applied to incoming metadata, whether from UC JSON or from a plugin manifest. You seem to be adding in these dependencies at the last minute, whereas they should been considered dependencies from the start.

I suspect that the difference is tangible: if you go into the setup wizard and check the box next to some ancient plugin, the box for the detached dependency like matrix-project should get checked then too (which of course also means that we will wind up installing it, but without any special handling here).

@@ -634,10 +634,8 @@ public Plugin(String sourceId, JSONObject o) {
this.categories = o.has("labels") ? (String[])o.getJSONArray("labels").toArray(new String[0]) : null;
for(Object jo : o.getJSONArray("dependencies")) {
JSONObject depObj = (JSONObject) jo;
// Make sure there's a name attribute, that that name isn't maven-plugin - we ignore that one -

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

WTF! From ad172c7 by @abayer FTR.

throw e;
} catch (Exception e) {
LOGGER.log(Level.WARNING, "Failed to set the e-mail address",e);
if(isMailerPluginPresent()) {

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

Simpler to just catch, and ignore, ClassNotFoundException right here.

}
user.save();
return user;
}

public boolean isMailerPluginPresent() {

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

🐛 should not be an API:

@Restricted(NoExternalUse.class) // just here for Jelly
if(localMatches.length > 0) {
matches = matches.concat(localMatches);
}
});

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

Is this related?

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

Related to issues with dependencies, insomuch as they were erroneously being included in search results in the installer, and especially now that these are included in the metadata, without these changes, searching for 'maven' or 'mailer' doesn't prove very useful

@@ -358,7 +358,7 @@ private User createAccount(StaplerRequest req, StaplerResponse rsp, boolean self
if(si.fullname==null || si.fullname.length()==0)
si.fullname = si.username;

if(si.email==null || !si.email.contains("@"))
if(isMailerPluginPresent() && (si.email==null || !si.email.contains("@")))

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

This is really crying out for an extension point—a way for UserPropertyDescriptor to indicate that it wishes to be configured during account creation, for security realms supporting such a concept.

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

Agree, but given the timing and existing code, I think handling of this is probably sufficient: this at least hides the fields that would not save properly, which would be very confusing and buggy to the user

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

Yes I think this is good enough for now, just mentioning it for the record.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 15, 2016

Member

This is really crying out for an extension point

Agreed, would be great, but out of scope for this PR and 2.0 I think. 2.1 maybe…

kzantow added some commits Mar 15, 2016

Address @jglick's comments: refactor ClassicPluginStrategy to use
BREAK_CYCLE, use the same getImpliedDependency method for multiple code
paths, @restricted isMailerPresent
Implement @jglick's idea: move implicit dependency modifications to
UpdateSite, when reading plugin metadata
@@ -612,7 +623,7 @@ public Api getApi() {
public final String[] categories;

/**
* Dependencies of this plugin.
* Dependencies of this plugin, a name -&gt; version mapping.

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

BTW requires no escape. :-)

@@ -260,6 +260,40 @@ private static Manifest loadLinkedManifest(File archive) throws IOException {
createClassLoader(paths, dependencyLoader, atts), disableFile, dependencies, optionalDependencies);
}

private static void fix(Attributes atts, List<PluginWrapper.Dependency> optionalDependencies) {
// don't fix the dependency for yourself, or else we'll have a cycle

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

This comment might make more sense inside getImpliedDependencies where the pluginName is first used.


/**
* Returns all the plugin dependencies that are implicit based on a particular Jenkins version
*/

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

@since TODO

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

(assuming you expect this to be public)

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

(BTW for plugin-compat-tester we would actually need the list to be present in a static resource. CC @kwhetstone.)

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

Is it fine to leave it literally "@SInCE TODO" should I guess at the upcoming version? Sorry, just don't know what to do here, specifically.

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

As a rule in PRs we just leave it as TODO, with the expectation that the person merging the PR should replace that with the actual version then in the POM. Of course in this case you expect the version to be 2.0 (or some uninteresting alpha variant thereof).

if(!implicitDeps.isEmpty()) {
for(PluginWrapper.Dependency dep : implicitDeps) {
if(!p.dependencies.containsKey(dep.shortName)) {
p.dependencies.put(dep.shortName, dep.version);

This comment has been minimized.

Copy link
@jglick

jglick Mar 15, 2016

Member

FWIW in createPluginWrapper these are added as optional dependencies—I am not really sure why, and that is perhaps part of the problem in JENKINS-33308.

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

Yes, they're added as optional dependencies, but this is too late in the process, I believe this just makes the classes available at runtime?

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 15, 2016

🐝 but please check behavior w.r.t. disabling implied dependencies which I suspect is still broken. (For example with the Flexible Publish → Matrix Project dep.)

@kzantow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

I did test the update center attempting to remove an implied dependency, it treats them as normal, required dependencies and doesn't let you. @jglick

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 15, 2016

disabling implied dependencies which I suspect is still broken

Yes…so you have fixed the UI portion of the code to consider these, and did so as mandatory dependencies. I suspect that the mistake in PluginManager (i.e., the non-UI part) treating them as optional will not become visible until we have something like #2001. You would expect for example that

touch $JENKINS_HOME/work/plugins/matrix-project.jpi.disabled

and restarting would cause Flexible Publish to be disabled, too. Even if there were such a defense in core—today there is not—the inexplicable use of optionalDependencies for detached dependencies means that it would still not work for these cases.

Thus not a regression in your PR, but rather an apparent bug in code your PR refactored.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 15, 2016

From reading the code, this looks like 👍 to me. Now on to the testing…

user.addProperty((UserProperty)c.newInstance(si.email));
} catch (ReflectiveOperationException e) {
throw new RuntimeException(e);
}

This comment has been minimized.

Copy link
@kwhetstone

kwhetstone Mar 15, 2016

Maybe log a message here with the attempted action.

This comment has been minimized.

Copy link
@kzantow

kzantow Mar 15, 2016

Author Contributor

It's not attempting anything, it doesn't proceed into this block unless isMailerPluginPresent() is true, in which case the class check is already done and instead of silently failing as it was doing, it will throw an exception.

@kzantow kzantow force-pushed the kzantow:2.0 branch from db2f7d5 to e7ad694 Mar 16, 2016

@kwhetstone

This comment has been minimized.

Copy link

commented Mar 16, 2016

🐝

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 16, 2016

Seems pretty solid in testing. It's annoying as hell, but that's how implied dependencies work…

daniel-beck added a commit that referenced this pull request Mar 16, 2016

Merge pull request #2122 from kzantow/2.0
[JENKINS-33296] - plugin dependency issues during install

@daniel-beck daniel-beck merged commit 449719c into jenkinsci:2.0 Mar 16, 2016

1 check passed

Jenkins This pull request looks good
Details
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.