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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-52665] Treat plugin dependency mismatches involving snapshots as nonfatal #3551

Merged
merged 1 commit into from
Aug 11, 2018
Merged

[JENKINS-52665] Treat plugin dependency mismatches involving snapshots as nonfatal #3551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions core/src/main/java/hudson/PluginWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import hudson.model.UpdateCenter;
import hudson.model.UpdateSite;
import hudson.util.VersionNumber;
import org.jvnet.localizer.ResourceBundleHolder;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.accmod.restrictions.NoExternalUse;
Expand All @@ -54,7 +53,6 @@
import javax.annotation.Nonnull;
import java.io.Closeable;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.net.URL;
Expand All @@ -64,7 +62,6 @@
import java.util.Collections;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -600,7 +597,7 @@ public boolean hasLicensesXml() {
} else {
VersionNumber actualVersion = Jenkins.getVersion();
if (actualVersion.isOlderThan(new VersionNumber(requiredCoreVersion))) {
dependencyErrors.put(Messages.PluginWrapper_obsoleteCore(Jenkins.getVersion().toString(), requiredCoreVersion), false);
versionDependencyError(Messages.PluginWrapper_obsoleteCore(Jenkins.getVersion().toString(), requiredCoreVersion), Jenkins.getVersion().toString(), requiredCoreVersion);
}
}
}
Expand All @@ -618,11 +615,11 @@ public boolean hasLicensesXml() {
} else {
if (dependency.isActive()) {
if (isDependencyObsolete(d, dependency)) {
dependencyErrors.put(Messages.PluginWrapper_obsolete(dependency.getLongName(), dependency.getVersion(), d.version), false);
versionDependencyError(Messages.PluginWrapper_obsolete(dependency.getLongName(), dependency.getVersion(), d.version), dependency.getVersion(), d.version);
}
} else {
if (isDependencyObsolete(d, dependency)) {
dependencyErrors.put(Messages.PluginWrapper_disabledAndObsolete(dependency.getLongName(), dependency.getVersion(), d.version), false);
versionDependencyError(Messages.PluginWrapper_disabledAndObsolete(dependency.getLongName(), dependency.getVersion(), d.version), dependency.getVersion(), d.version);
} else {
dependencyErrors.put(Messages.PluginWrapper_disabled(dependency.getLongName()), false);
}
Expand All @@ -635,7 +632,7 @@ public boolean hasLicensesXml() {
PluginWrapper dependency = parent.getPlugin(d.shortName);
if (dependency != null && dependency.isActive()) {
if (isDependencyObsolete(d, dependency)) {
dependencyErrors.put(Messages.PluginWrapper_obsolete(dependency.getLongName(), dependency.getVersion(), d.version), false);
versionDependencyError(Messages.PluginWrapper_obsolete(dependency.getLongName(), dependency.getVersion(), d.version), dependency.getVersion(), d.version);
} else {
dependencies.add(d);
}
Expand All @@ -660,6 +657,26 @@ private boolean isDependencyObsolete(Dependency d, PluginWrapper dependency) {
return ENABLE_PLUGIN_DEPENDENCIES_VERSION_CHECK && dependency.getVersionNumber().isOlderThan(new VersionNumber(d.version));
}

/**
* Called when there appears to be a core or plugin version which is too old for a stated dependency.
* Normally records an error in {@link #dependencyErrors}.
* But if one or both versions {@link #isSnapshot}, just issue a warning (JENKINS-52665).
*/
private void versionDependencyError(String message, String actual, String minimum) {
if (isSnapshot(actual) || isSnapshot(minimum)) {
LOGGER.log(WARNING, "Suppressing dependency error in {0} v{1}: {2}", new Object[] {getLongName(), getVersion(), message});
} else {
dependencyErrors.put(message, false);
}
}

/**
* Similar to {@code org.apache.maven.artifact.ArtifactUtils.isSnapshot}.
*/
static boolean isSnapshot(@Nonnull String version) {
return version.contains("-SNAPSHOT") || version.matches(".+-[0-9]{8}.[0-9]{6}-[0-9]+");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how close https://updates.jenkins.io/download/plugins/nexus-jenkins-plugin/ comes, I'm not sure this is reality proof.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what build tool is producing those version numbers but they do not look like regular Maven snapshots to me. I cannot even find the source repo for this…? Anyway we can always expand the list later if there are proven needs; this pattern should cover plugins built using Maven & Incrementals in the normal way, which is a step forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm trying to point out here is that this disables a useful protection mechanism based on a heuristic I'm not sure is reliable -- the format of the version number.

I'm not quite sure what it takes for this to break for real users, but the use case seems somewhat underdefined here (I thought incrementals weren't snapshots).

Copy link
Member

@dwnusbaum dwnusbaum Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like just return version.contains("-SNAPSHOT"); covers the case explained in Jira, since if either the actual or minimum version is a Snapshot we suppress the error. Is the purpose of the second pattern to suppress errors if two incremental versions are being used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not a match for incremental versions, it is a match for timestamped snapshots like 3.19-20180719.153600-1 (~ 3.19-SNAPSHOT but a particular instance as deployed to a repository on that date).

Incremental versions would look something like 2.22-rc312.d41f02f66ac5, and these continue to be strictly compared by the lib-version-number library, so 311 < 312 < 313. Note that an incremental version that compares as greater than the requested dependency might have come from an unrelated branch and thus not actually be compatible, but it is up to the developer to deal with that (users of e.g. Essentials/Evergreen should never be running anything that has not been merged to master); what JEP-305 does guarantee (after jenkinsci/incrementals-tools#6 anyway) is that if the actual plugin was built from a commit which is a descendant (in the DAG ancestry sense) of the requested commit (and thus expected to be compatible with the dependency), the actual “revision” number will compare as greater, and so the plugin manager will be satisfied.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I totally forgot about timestamped snapshots, thanks for clarifying!

}

/**
* If the plugin has {@link #getUpdateInfo() an update},
* returns the {@link hudson.model.UpdateSite.Plugin} object.
Expand Down
17 changes: 14 additions & 3 deletions core/src/test/java/hudson/PluginWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@
import org.mockito.stubbing.Answer;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
import static org.junit.Assert.*;
import org.jvnet.hudson.test.Issue;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -180,4 +179,16 @@ private PluginWrapper build() {
);
}
}

@Issue("JENKINS-52665")
@Test
public void isSnapshot() {
assertFalse(PluginWrapper.isSnapshot("1.0"));
assertFalse(PluginWrapper.isSnapshot("1.0-alpha-1"));
assertFalse(PluginWrapper.isSnapshot("1.0-rc9999.abc123def456"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test related to Daniel's comment above:

diff --git a/core/src/test/java/hudson/PluginWrapperTest.java b/core/src/test/java/hudson/PluginWrapperTest.java
index e15c803dca..62027f8602 100644
--- a/core/src/test/java/hudson/PluginWrapperTest.java
+++ b/core/src/test/java/hudson/PluginWrapperTest.java
@@ -186,6 +186,7 @@ public class PluginWrapperTest {
         assertFalse(PluginWrapper.isSnapshot("1.0"));
         assertFalse(PluginWrapper.isSnapshot("1.0-alpha-1"));
         assertFalse(PluginWrapper.isSnapshot("1.0-rc9999.abc123def456"));
+        assertFalse(PluginWrapper.isSnapshot("1.2.20170404-163441.794de4c"));
         assertTrue(PluginWrapper.isSnapshot("1.0-SNAPSHOT"));
         assertTrue(PluginWrapper.isSnapshot("1.0-20180719.153600-1"));
         assertTrue(PluginWrapper.isSnapshot("1.0-SNAPSHOT (private-abcd1234-jqhacker)"));

And mvn clean test -Dtest=PluginWrapperTest still succeeds for me, so LGTM.

assertTrue(PluginWrapper.isSnapshot("1.0-SNAPSHOT"));
assertTrue(PluginWrapper.isSnapshot("1.0-20180719.153600-1"));
assertTrue(PluginWrapper.isSnapshot("1.0-SNAPSHOT (private-abcd1234-jqhacker)"));
}

}