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

EOL dependency on Maven Integration #5156

Merged
merged 6 commits into from
Jan 11, 2021
Merged

EOL dependency on Maven Integration #5156

merged 6 commits into from
Jan 11, 2021

Conversation

basil
Copy link
Member

@basil basil commented Jan 8, 2021

Background

See jenkinsci/jenkins#5118 (comment) and jenkinsci/maven-plugin#155. The tests in Jenkins core depend on an old version of Maven Integration (i.e., maven-plugin). This pollutes the dependency tree for the tests in Jenkins core, causing RequireUpperBoundDeps issues and complicating maintenance.

Problem

The tests in Jenkins core depend on an old version of maven-plugin for three reasons:

  1. A number of tests in Jenkins core depend on hudson.maven.MavenModuleSet or hudson.maven.MavenModuleSetBuild. These classes were previously in Jenkins core before they were extracted to maven-plugin. However, the corresponding tests were never moved to maven-plugin. As a result, the tests are no longer in the same repository as the code under test.
  2. hudson.CustomPluginManagerTest, hudson.CustomPluginManagerTest, and hudson.util.FormFieldValidatorTest load a checked-in tasks.jpi plugin using @WithPlugin("tasks.jpi"), and tasks.jpi depends on maven-plugin (which is currently provided via test/pom.xml). Note that the plugin page for Task Scanner reads: "The Task Scanner Plug-in reached end-of-life. All functionality has been integrated into the Warnings Next Generation Plugin." The dependency on the deprecated Task Scanner plugin is largely unnecessary, as is explained further below.
  3. jenkins.slaves.OldRemotingAgentTest imports a class from a transitive dependency of maven-plugin; namely, org.codehaus.plexus.util.FileUtils. This is probably accidental; the programmer almost certainly meant to import org.apache.commons.io.FileUtils instead.

Solution

To address these maintenance problems, we propose the following solution:

  1. Move the tests in Jenkins core that depend on hudson.maven.MavenModuleSet or hudson.maven.MavenModuleSetBuild to maven-plugin.
  2. Refactor hudson.CustomPluginManagerTest, hudson.CustomPluginManagerTest, and hudson.util.FormFieldValidatorTest such that they no longer depend on a checked-in tasks.jpi plugin (and therefore no longer depend, transitively, on maven-plugin). This eliminates two old and unmaintained plugins from the dependency tree, easing maintenance efforts.
  3. Import org.apache.commons.io.FileUtils rather than org.codehaus.plexus.util.FileUtils in jenkins.slaves.OldRemotingAgentTest, as was probably intended in the first place. This eliminates a dependency on Plexus (and therefore, on maven-plugin which provides Plexus).

Implementation

We implement these changes with four separate commits:

  1. First, we move any tests that depend on MavenModuleSet or MavenModuleSetBuild to maven-plugin in Move tests that depend on MavenModuleSet and MavenModuleSetBuild from Jenkins core to Maven Integration maven-plugin#155. A handful of these tests are already disabled and have been for several years; we do not make any effort to revive these or move them. Rather we only move the tests that are still running regularly. Then we delete any references to MavenModuleSet or MavenModuleSetBuild from the tests in Jenkins core.
  2. We delete tasks.jpi from the source tree and remove any references to it. The reference in hudson.util.FormFieldValidatorTest appears to be unused; the test passes without it and I cannot see any reason why it would be needed for the test case. The remaining references in hudson.CustomPluginManagerTest and hudson.PluginManagerTest are for test cases that require any JPI, not tasks.jpi in particular. We replace these with references to htmlpublisher.jpi, another JPI file that is already checked in. This should serve the same purpose. The only exception is hudson.PluginManagerTest, which deals with tasks.jpi specifically in order to confirm that it has an optional dependency on maven-plugin. This seems pointless, and the test has this comment: "This actually doesn't really test what we need, though." In our judgment this test is not valuable. We deleted this pointless test.
  3. We fix the erroneous import in jenkins.slaves.OldRemotingAgentTest.
  4. Finally, we remove all references to maven-plugin from test/pom.xml. This cascades into further deletion of some requireUpperBoundDeps exclusions, which were added specifically to deal with maven-plugin.

@jvz
Copy link
Member

jvz commented Jan 8, 2021

Very nice to see this!

@oleg-nenashev oleg-nenashev added internal skip-changelog Should not be shown in the changelog labels Jan 9, 2021
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Less code to maintain

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 9, 2021
@timja
Copy link
Member

timja commented Jan 9, 2021

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@timja timja merged commit 1b2ca8a into jenkinsci:master Jan 11, 2021
@jglick
Copy link
Member

jglick commented Jan 11, 2021

Long overdue. Thanks!

@basil basil deleted the maven branch January 11, 2021 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants