JENKINS-70032 : Make maven-plugin an optional dependency

[aah9]

This change makes maven-plugin an optional dependency, for the reasons explained in JENKINS-70032.

The copy artifact plugin already does this, so I just copied its plugin checks:

https://github.com/jenkinsci/copyartifact-plugin/blob/master/src/main/java/hudson/plugins/copyartifact/CopyArtifact.java#L525

https://github.com/jenkinsci/copyartifact-plugin/blob/master/src/main/java/hudson/plugins/copyartifact/CopyArtifact.java#L848

cc @alecharp since you last committed to the repo.

[aah9]

@alecharp - could you review this or point me to someone that could review this, please?

[alecharp]

I'm not sure the test is valid.

If the maven-plugin is not installed, the object cannot be an instance of MavenModuleSetBuild. However, the class could not exists in that case.
How the plugin would behave it that case? The class couldn't be loaded and still the code would try to use it for the test.

Could you test that?

[alecharp]

If the plugin is not installed, can the src be an instance of MavenModelSetBuild? This seems redundant, no?

[alecharp]

dito.

[aah9]

I'm not sure the test is valid.

If the maven-plugin is not installed, the object cannot be an instance of MavenModuleSetBuild. However, the class could not exists in that case. How the plugin would behave it that case? The class couldn't be loaded and still the code would try to use it for the test.

Could you test that?

Sure, I'll retest it and confirm that the s3CopyArtifact steps works without the maven plugin.

My understanding of how this works is ...

The plugin compiles without errors because maven-plugin is still listed as a dependency, just with an additional property, optional = true.

At runtime, if the plugin is not installed, it'll exit the conditional statement before running src instanceof MavenModuleSetBuild, preventing a ClassNotFoundException.

[akrapfl]

This article confirms the intent here.

This class can execute properly without the MavenModuleSetBuild, and the extra check appropriately short-circuits the condition BEFORE the exception.

I'd love this PR to be approved, as I don't want to bundle the maven-plugin anymore, but I'd like to continue to support the s3-plugin.

[aah9]

Confirmed, the copy artifact from s3 step works:

• in a freestyle job without maven plugin installed
• in a maven job (with maven plugin installed, ofc)
• in a freestyle job with maven plugin installed

[alecharp]

Sorry, I was out. My question about the isMavenPluginInstalled method is that I'm not sure it is necessary to have this test. If the plugin is not installed, the src object cannot be of type MavenModuleSetBuild, yes?

[aah9]

Sorry, I was out. My question about the isMavenPluginInstalled method is that I'm not sure it is necessary to have this test. If the plugin is not installed, the src object cannot be of type MavenModuleSetBuild, yes?

Correct, src cannot be of type MavenModuleSetBuild, but without the test, java will still try and check whether src is an instance of MavenModuleSetBuild, and since MavenModuleSetBuild does not exist, it will throw a ClassNotFoundException:

[alecharp]

This makes sense. Thank you for taking the time to point this and test it.

[alecharp]

@aah9 which JVM have you used to do your tests?

[aah9]

@alecharp - My test jenkins instances were running jdk 11.