From 7e558876aee9090947036e47ca99f98be8808ba3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 19 Dec 2024 03:23:04 -0500 Subject: [PATCH] Expand robustness of `isPlugin` against unbuildable models (#699) * Expand robustness of `isPlugin` against unbuildable models * Javadoc mistake * IT for `isPlugin` --- pom.xml | 3 + .../invoker.properties | 1 + .../pom.xml | 38 +++++++++ .../src/main/resources/index.jelly | 2 + .../src/test/java/sample/SampleTest.java | 16 ++++ src/it/somedep-1/invoker.properties | 1 + src/it/somedep-1/pom.xml | 36 ++++++++ .../java/io/jenkins/plugins/sample/Util.java | 13 +++ .../somedep-1/src/main/resources/index.jelly | 4 + src/it/somedep-2/invoker.properties | 1 + src/it/somedep-2/pom.xml | 36 ++++++++ .../java/io/jenkins/plugins/sample/Util.java | 13 +++ .../somedep-2/src/main/resources/index.jelly | 4 + .../maven/plugins/hpi/AbstractHpiMojo.java | 2 +- .../hpi/AbstractJenkinsManifestMojo.java | 4 +- .../plugins/hpi/AssembleDependenciesMojo.java | 10 +-- .../jenkinsci/maven/plugins/hpi/HplMojo.java | 2 +- .../hpi/ListPluginDependenciesMojo.java | 2 +- .../maven/plugins/hpi/MavenArtifact.java | 39 ++++++--- .../jenkinsci/maven/plugins/hpi/RunMojo.java | 2 +- .../maven/plugins/hpi/TestDependencyMojo.java | 2 +- .../maven/plugins/hpi/ValidateHpiMojo.java | 2 +- .../jenkinsci/maven/plugins/hpi/WarMojo.java | 83 +++++++++---------- 23 files changed, 244 insertions(+), 72 deletions(-) create mode 100644 src/it/override-test-dependencies-split-plugin/invoker.properties create mode 100644 src/it/override-test-dependencies-split-plugin/pom.xml create mode 100644 src/it/override-test-dependencies-split-plugin/src/main/resources/index.jelly create mode 100644 src/it/override-test-dependencies-split-plugin/src/test/java/sample/SampleTest.java create mode 100644 src/it/somedep-1/invoker.properties create mode 100644 src/it/somedep-1/pom.xml create mode 100644 src/it/somedep-1/src/main/java/io/jenkins/plugins/sample/Util.java create mode 100644 src/it/somedep-1/src/main/resources/index.jelly create mode 100644 src/it/somedep-2/invoker.properties create mode 100644 src/it/somedep-2/pom.xml create mode 100644 src/it/somedep-2/src/main/java/io/jenkins/plugins/sample/Util.java create mode 100644 src/it/somedep-2/src/main/resources/index.jelly diff --git a/pom.xml b/pom.xml index b348265619..77db16ec50 100644 --- a/pom.xml +++ b/pom.xml @@ -343,6 +343,9 @@ + + somedep-*/pom.xml + diff --git a/src/it/override-test-dependencies-split-plugin/invoker.properties b/src/it/override-test-dependencies-split-plugin/invoker.properties new file mode 100644 index 0000000000..90ea8f6118 --- /dev/null +++ b/src/it/override-test-dependencies-split-plugin/invoker.properties @@ -0,0 +1 @@ +invoker.goals=-ntp -DoverrideVersions=io.jenkins.plugins:somedep:2 -Djenkins.version=2.489 test diff --git a/src/it/override-test-dependencies-split-plugin/pom.xml b/src/it/override-test-dependencies-split-plugin/pom.xml new file mode 100644 index 0000000000..9ee1d523bf --- /dev/null +++ b/src/it/override-test-dependencies-split-plugin/pom.xml @@ -0,0 +1,38 @@ + + + 4.0.0 + + org.jenkins-ci.plugins + plugin + 5.3 + + + org.jenkins-ci.tools.hpi.its + override-test-dependencies-smokes + 1.0-SNAPSHOT + hpi + + 2.479.2 + @project.version@ + false + + + + io.jenkins.plugins + somedep + 1 + + + + + repo.jenkins-ci.org + https://repo.jenkins-ci.org/public/ + + + + + repo.jenkins-ci.org + https://repo.jenkins-ci.org/public/ + + + diff --git a/src/it/override-test-dependencies-split-plugin/src/main/resources/index.jelly b/src/it/override-test-dependencies-split-plugin/src/main/resources/index.jelly new file mode 100644 index 0000000000..2f655e510a --- /dev/null +++ b/src/it/override-test-dependencies-split-plugin/src/main/resources/index.jelly @@ -0,0 +1,2 @@ + +
diff --git a/src/it/override-test-dependencies-split-plugin/src/test/java/sample/SampleTest.java b/src/it/override-test-dependencies-split-plugin/src/test/java/sample/SampleTest.java new file mode 100644 index 0000000000..32cea78f18 --- /dev/null +++ b/src/it/override-test-dependencies-split-plugin/src/test/java/sample/SampleTest.java @@ -0,0 +1,16 @@ +package sample; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +import io.jenkins.plugins.sample.Util; +import org.junit.Test; + +public class SampleTest { + + @Test + public void smokes() throws Exception { + // just load some org.apache.commons.compress types: + assertThat(Util.x(), is(259L)); + } +} diff --git a/src/it/somedep-1/invoker.properties b/src/it/somedep-1/invoker.properties new file mode 100644 index 0000000000..6530ff60fc --- /dev/null +++ b/src/it/somedep-1/invoker.properties @@ -0,0 +1 @@ +invoker.goals=-ntp -P-might-produce-incrementals -Pquick-build install diff --git a/src/it/somedep-1/pom.xml b/src/it/somedep-1/pom.xml new file mode 100644 index 0000000000..d5d6eef9ce --- /dev/null +++ b/src/it/somedep-1/pom.xml @@ -0,0 +1,36 @@ + + + 4.0.0 + + org.jenkins-ci.plugins + plugin + 5.3 + + + io.jenkins.plugins + somedep + 1 + hpi + + 2.479.2 + + + + org.apache.commons + commons-compress + provided + + + + + repo.jenkins-ci.org + https://repo.jenkins-ci.org/public/ + + + + + repo.jenkins-ci.org + https://repo.jenkins-ci.org/public/ + + + diff --git a/src/it/somedep-1/src/main/java/io/jenkins/plugins/sample/Util.java b/src/it/somedep-1/src/main/java/io/jenkins/plugins/sample/Util.java new file mode 100644 index 0000000000..e3791c0151 --- /dev/null +++ b/src/it/somedep-1/src/main/java/io/jenkins/plugins/sample/Util.java @@ -0,0 +1,13 @@ +package io.jenkins.plugins.sample; + +import org.apache.commons.compress.utils.ByteUtils; + +public class Util { + + public static long x() { + return ByteUtils.fromLittleEndian(new byte[] {3, 1}); // ⇒ 259 + } + + private Util() {} + +} diff --git a/src/it/somedep-1/src/main/resources/index.jelly b/src/it/somedep-1/src/main/resources/index.jelly new file mode 100644 index 0000000000..35f37a7f2d --- /dev/null +++ b/src/it/somedep-1/src/main/resources/index.jelly @@ -0,0 +1,4 @@ + +
+ TODO +
diff --git a/src/it/somedep-2/invoker.properties b/src/it/somedep-2/invoker.properties new file mode 100644 index 0000000000..6530ff60fc --- /dev/null +++ b/src/it/somedep-2/invoker.properties @@ -0,0 +1 @@ +invoker.goals=-ntp -P-might-produce-incrementals -Pquick-build install diff --git a/src/it/somedep-2/pom.xml b/src/it/somedep-2/pom.xml new file mode 100644 index 0000000000..8a2b9d57c6 --- /dev/null +++ b/src/it/somedep-2/pom.xml @@ -0,0 +1,36 @@ + + + 4.0.0 + + org.jenkins-ci.plugins + plugin + 5.3 + + + io.jenkins.plugins + somedep + 2 + hpi + + 2.479.2 + + + + io.jenkins.plugins + commons-compress-api + 1.26.1-2 + + + + + repo.jenkins-ci.org + https://repo.jenkins-ci.org/public/ + + + + + repo.jenkins-ci.org + https://repo.jenkins-ci.org/public/ + + + diff --git a/src/it/somedep-2/src/main/java/io/jenkins/plugins/sample/Util.java b/src/it/somedep-2/src/main/java/io/jenkins/plugins/sample/Util.java new file mode 100644 index 0000000000..e3791c0151 --- /dev/null +++ b/src/it/somedep-2/src/main/java/io/jenkins/plugins/sample/Util.java @@ -0,0 +1,13 @@ +package io.jenkins.plugins.sample; + +import org.apache.commons.compress.utils.ByteUtils; + +public class Util { + + public static long x() { + return ByteUtils.fromLittleEndian(new byte[] {3, 1}); // ⇒ 259 + } + + private Util() {} + +} diff --git a/src/it/somedep-2/src/main/resources/index.jelly b/src/it/somedep-2/src/main/resources/index.jelly new file mode 100644 index 0000000000..35f37a7f2d --- /dev/null +++ b/src/it/somedep-2/src/main/resources/index.jelly @@ -0,0 +1,4 @@ + +
+ TODO +
diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java index f51e631aef..d5ca4ec345 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java @@ -458,7 +458,7 @@ public void buildWebapp(MavenProject project, File webappDirectory) throws MojoE Set jenkinsPlugins = new HashSet<>(); Set excludedArtifacts = new HashSet<>(); for (MavenArtifact artifact : Utils.unionOf(artifacts, dependencyArtifacts)) { - if (artifact.isPluginBestEffort(getLog())) { + if (artifact.isPlugin(getLog())) { jenkinsPlugins.add(artifact.getId()); } // Exclude dependency if it comes from test or provided trail. diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractJenkinsManifestMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractJenkinsManifestMojo.java index 690e88a67c..58238aaf2d 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractJenkinsManifestMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractJenkinsManifestMojo.java @@ -263,7 +263,7 @@ protected void setAttributes(Manifest.ExistingSection mainSection) private String findDependencyPlugins() throws IOException, MojoExecutionException { StringBuilder buf = new StringBuilder(); for (MavenArtifact a : getDirectDependencyArtfacts()) { - if (a.isPlugin() && scopeFilter.include(a.artifact) && !a.hasSameGAAs(project)) { + if (a.isPlugin(getLog()) && scopeFilter.include(a.artifact) && !a.hasSameGAAs(project)) { if (buf.length() > 0) { buf.append(','); } @@ -280,7 +280,7 @@ private String findDependencyPlugins() throws IOException, MojoExecutionExceptio // see // http://jenkins-ci.361315.n4.nabble.com/Classloading-problem-when-referencing-classes-from-another-plugin-during-the-initialization-phase-of-td394967.html for (Artifact a : project.getDependencyArtifacts()) { - if ("provided".equals(a.getScope()) && wrap(a).isPlugin()) { + if ("provided".equals(a.getScope()) && wrap(a).isPlugin(getLog())) { throw new MojoExecutionException( a.getId() + " is marked as 'provided' scope dependency, but it should be the 'compile' scope."); } diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/AssembleDependenciesMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/AssembleDependenciesMojo.java index cb362a6f7e..b61c471ec0 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/AssembleDependenciesMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/AssembleDependenciesMojo.java @@ -68,13 +68,9 @@ protected boolean accept(DependencyNode g) { return false; // cut off optional dependencies } - try { - if (!a.isPlugin()) { - // only traverse chains of direct plugin dependencies, unless it's from the root - return g.getParent() == null; - } - } catch (IOException e) { - getLog().warn("Failed to process " + a, e); + if (!a.isPlugin(getLog())) { + // only traverse chains of direct plugin dependencies, unless it's from the root + return g.getParent() == null; } MavenArtifact v = hpis.get(a.getArtifactId()); diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/HplMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/HplMojo.java index dc8e33208a..e8fa8f76c4 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/HplMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/HplMojo.java @@ -119,7 +119,7 @@ private void buildLibraries(List paths) throws IOException, MojoExecutio // List up IDs of Jenkins plugin dependencies Set jenkinsPlugins = new HashSet<>(); for (MavenArtifact artifact : artifacts) { - if (artifact.isPluginBestEffort(getLog())) { + if (artifact.isPlugin(getLog())) { jenkinsPlugins.add(artifact.getId()); } } diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/ListPluginDependenciesMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/ListPluginDependenciesMojo.java index 7ed091f8d7..782b3f8bc4 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/ListPluginDependenciesMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/ListPluginDependenciesMojo.java @@ -36,7 +36,7 @@ public void execute() throws MojoExecutionException, MojoFailureException { ? new NullWriter() : new OutputStreamWriter(new FileOutputStream(outputFile), StandardCharsets.UTF_8)) { for (MavenArtifact a : getDirectDependencyArtfacts()) { - if (!a.isPlugin()) { + if (!a.isPlugin(getLog())) { continue; } diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/MavenArtifact.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/MavenArtifact.java index 3e289a17ac..53d401001e 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/MavenArtifact.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/MavenArtifact.java @@ -72,27 +72,40 @@ public MavenProject resolvePom() throws ProjectBuildingException { /** * Is this a Jenkins plugin? + * The preferred check is for {@code hpi} or {@code jpi} packaging. + * If the project model cannot be resolved + * (for example when an indirect dependency has a bogus {@code systemPath} that is only rejected in some environments) + * then the fallback logic is to look for a JAR manifest with entries typical of plugins. */ - public boolean isPlugin() throws IOException { - String type = getResolvedType(); - return type.equals("hpi") || type.equals("jpi"); - } - - /** - * Like {@link #isPlugin} but will not throw an exception if the project model cannot be resolved. - * Helpful for example when an indirect dependency has a bogus {@code systemPath} that is only rejected in some environments. - */ - public boolean isPluginBestEffort(Log log) { + public boolean isPlugin(Log log) { try { - return isPlugin(); + String type = getResolvedType(); + return type.equals("hpi") || type.equals("jpi"); } catch (IOException x) { if (log.isDebugEnabled()) { log.debug(x); } else { - log.warn(x.getCause().getMessage()); + log.warn("While inspecting " + artifact + ": " + x.getCause().getMessage()); } - return false; } + var f = artifact.getFile(); + if (f.getName().endsWith(".jar") && f.isFile()) { + try (var jf = new JarFile(f)) { + var mani = jf.getManifest(); + if (mani != null) { + var attr = mani.getMainAttributes(); + return attr.getValue("Jenkins-Version") != null && attr.getValue("Plugin-Version") != null; + } + } catch (IOException x) { + if (log.isDebugEnabled()) { + log.debug(x); + } else { + log.warn( + "While inspecting " + artifact + ": " + x.getCause().getMessage()); + } + } + } + return false; } public String getId() { diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/RunMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/RunMojo.java index c644e3596b..0107cd85e8 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/RunMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/RunMojo.java @@ -373,7 +373,7 @@ protected ClassLoader configureClassLoader(ClassLoader loader) { // copy other dependency Jenkins plugins try { for (MavenArtifact a : getProjectArtifacts()) { - if (!a.isPluginBestEffort(getLog())) { + if (!a.isPlugin(getLog())) { continue; } diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java index 1e5a77a4a8..129e98effc 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java @@ -411,7 +411,7 @@ private void copyTestDependencies(Set mavenArtifacts) throws Mojo List artifactRequests = new ArrayList<>(); for (MavenArtifact mavenArtifact : mavenArtifacts) { - if (!mavenArtifact.isPluginBestEffort(getLog())) { + if (!mavenArtifact.isPlugin(getLog())) { continue; } diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateHpiMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateHpiMojo.java index a0dbc84d87..33b8a9ebbe 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateHpiMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/ValidateHpiMojo.java @@ -29,7 +29,7 @@ public void execute() throws MojoExecutionException, MojoFailureException { for (MavenArtifact artifact : Utils.unionOf(getProjectArtfacts(), getDirectDependencyArtfacts())) { try { - if (artifact.isPluginBestEffort(getLog())) { + if (artifact.isPlugin(getLog())) { VersionNumber dependencyCoreVersion = getDependencyCoreVersion(artifact); if (dependencyCoreVersion.compareTo(maxCoreVersion) > 0) { maxCoreVersionArtifact = artifact; diff --git a/src/main/java/org/jenkinsci/maven/plugins/hpi/WarMojo.java b/src/main/java/org/jenkinsci/maven/plugins/hpi/WarMojo.java index 15fe620b9d..0bee7a0d6d 100644 --- a/src/main/java/org/jenkinsci/maven/plugins/hpi/WarMojo.java +++ b/src/main/java/org/jenkinsci/maven/plugins/hpi/WarMojo.java @@ -1,7 +1,6 @@ package org.jenkinsci.maven.plugins.hpi; import java.io.File; -import java.io.IOException; import java.util.LinkedHashSet; import java.util.Optional; import java.util.Set; @@ -54,58 +53,54 @@ public class WarMojo extends RunMojo { */ @Override public void execute() throws MojoExecutionException { - try { - if (outputFile == null) { - outputFile = new File( - getProject().getBasedir(), "target/" + getProject().getArtifactId() + ".war"); - } + if (outputFile == null) { + outputFile = + new File(getProject().getBasedir(), "target/" + getProject().getArtifactId() + ".war"); + } - File war = getJenkinsWarArtifact().getFile(); + File war = getJenkinsWarArtifact().getFile(); - Zip rezip = new Zip(); - rezip.setDestFile(outputFile); - rezip.setProject(new Project()); - ZipFileSet z = new ZipFileSet(); - z.setSrc(war); - rezip.addZipfileset(z); + Zip rezip = new Zip(); + rezip.setDestFile(outputFile); + rezip.setProject(new Project()); + ZipFileSet z = new ZipFileSet(); + z.setSrc(war); + rezip.addZipfileset(z); - getProject().setArtifacts(resolveDependencies(dependencyResolution)); + getProject().setArtifacts(resolveDependencies(dependencyResolution)); - Set projectArtifacts = new LinkedHashSet<>(getProjectArtifacts()); - if (getProject().getPackaging().equals("hpi") && addThisPluginToCustomWar) { - Optional.ofNullable(getProject()).map(MavenProject::getArtifact).map(a -> { - projectArtifacts.add(wrap(a)); // side effect - getLog().debug("This plugin " + a + "to be added to custom war"); - return projectArtifacts; // have to return something from multiline lambda inside map() - }); + Set projectArtifacts = new LinkedHashSet<>(getProjectArtifacts()); + if (getProject().getPackaging().equals("hpi") && addThisPluginToCustomWar) { + Optional.ofNullable(getProject()).map(MavenProject::getArtifact).map(a -> { + projectArtifacts.add(wrap(a)); // side effect + getLog().debug("This plugin " + a + "to be added to custom war"); + return projectArtifacts; // have to return something from multiline lambda inside map() + }); + } + for (MavenArtifact a : projectArtifacts) { + if (!a.isPlugin(getLog())) { + continue; } - for (MavenArtifact a : projectArtifacts) { - if (!a.isPlugin()) { - continue; - } - - // find corresponding .hpi file - Artifact hpi = - artifactFactory.createArtifact(a.getGroupId(), a.getArtifactId(), a.getVersion(), null, "hpi"); - hpi = MavenArtifact.resolveArtifact(hpi, project, session, repositorySystem); - if (hpi.getFile().isDirectory()) { - throw new UnsupportedOperationException( - hpi.getFile() + " is a directory and not packaged yet. this isn't supported"); - } + // find corresponding .hpi file + Artifact hpi = + artifactFactory.createArtifact(a.getGroupId(), a.getArtifactId(), a.getVersion(), null, "hpi"); + hpi = MavenArtifact.resolveArtifact(hpi, project, session, repositorySystem); - z = new ZipFileSet(); - z.setFile(hpi.getFile()); - z.setFullpath("WEB-INF/plugins/" + hpi.getArtifactId() + ".hpi"); - rezip.addZipfileset(z); + if (hpi.getFile().isDirectory()) { + throw new UnsupportedOperationException( + hpi.getFile() + " is a directory and not packaged yet. this isn't supported"); } - rezip.execute(); - getLog().info("Generated " + outputFile); - - projectHelper.attachArtifact(getProject(), "war", outputFile); - } catch (IOException e) { - throw new MojoExecutionException("Failed to package war", e); + z = new ZipFileSet(); + z.setFile(hpi.getFile()); + z.setFullpath("WEB-INF/plugins/" + hpi.getArtifactId() + ".hpi"); + rezip.addZipfileset(z); } + + rezip.execute(); + getLog().info("Generated " + outputFile); + + projectHelper.attachArtifact(getProject(), "war", outputFile); } }