Define slf4jVersion unconditionally

[proski]

*  Define slf4jVersion unconditionally
   
   Add a comment that slf4jVersion is only used in the profile that doesn't
   use Jenkins BOM.
   
   Conditionally defined slf4jVersion cannot be used in plugins. Even though
   ${slf4jVersion} expands during "validate" goal, it fails at the "flatten"
   goal.

[proski]

I don't know if it's the right solution, but it fixes an issue with Stash Pull Request Builder plugin, which can use plugin-pom 3.53 but not 3.54.

The plugin has a dependency:

    <dependency>
      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>
      <version>${slf4jVersion}</version>
      <scope>provided</scope>
    </dependency>

That's how it fails to build:

$ mvn clean verify
[INFO] Scanning for projects...
[INFO] 
[INFO] ----------< org.jenkins-ci.plugins:stash-pullrequest-builder >----------
[INFO] Building Stash Pullrequest Builder Plugin 1.16-SNAPSHOT
[INFO] --------------------------------[ hpi ]---------------------------------
[INFO] 
[INFO] --- maven-clean-plugin:3.1.0:clean (default-clean) @ stash-pullrequest-builder ---
[INFO] Deleting /home/proski/src/jenkins/plugins/stash-pullrequest-builder-plugin/target
[INFO] 
[INFO] --- maven-hpi-plugin:3.10:validate (default-validate) @ stash-pullrequest-builder ---
[INFO] 
[INFO] --- maven-enforcer-plugin:3.0.0-M3:display-info (display-info) @ stash-pullrequest-builder ---
[INFO] Maven Version: 3.5.4
[INFO] JDK Version: 1.8.0_232 normalized as: 1.8.0-232
[INFO] OS Info: Arch: amd64 Family: unix Name: linux Version: 5.3.12-300.fc31.x86_64
[INFO] 
[INFO] --- maven-enforcer-plugin:3.0.0-M3:enforce (display-info) @ stash-pullrequest-builder ---
[INFO] Adding ignore: module-info
[INFO] Ignoring requireUpperBoundDeps in com.google.guava:guava
[INFO] 
[INFO] --- sortpom-maven-plugin:2.10.0:sort (sortpom) @ stash-pullrequest-builder ---
[INFO] Sorting file /home/proski/src/jenkins/plugins/stash-pullrequest-builder-plugin/pom.xml
[INFO] Pom file is already sorted, exiting
[INFO] 
[INFO] --- maven-localizer-plugin:1.26:generate (default) @ stash-pullrequest-builder ---
[INFO] 
[INFO] --- fmt-maven-plugin:2.9:format (default) @ stash-pullrequest-builder ---
[INFO] Processed 33 files (0 reformatted).
[INFO] 
[INFO] --- maven-resources-plugin:3.1.0:resources (default-resources) @ stash-pullrequest-builder ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 27 resources
[INFO] 
[INFO] --- flatten-maven-plugin:1.1.0:flatten (flatten) @ stash-pullrequest-builder ---
[INFO] Generating flattened POM of project org.jenkins-ci.plugins:stash-pullrequest-builder:hpi:1.16-SNAPSHOT...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

Note that a reference to an invalid variable would fail much earlier.

I can remove the dependency, but I believe it's important to declare dependencies of the code. The code uses org.slf4j.helpers.MessageFormatter to format strings.

I can use 1.7.25 instead of ${slf4jVersion}, but I want to use the version that Jenkins is guaranteed to provide to the plugin.

Moving the slf4jVersion definition to the top level with an appropriate comment resolves the issue.

[jtnord]

The plugin has a dependency:

<dependency>
  <groupId>org.slf4j</groupId>
  <artifactId>slf4j-api</artifactId>
  <version>${slf4jVersion}</version>
  <scope>provided</scope>
</dependency>

it should not have a version defined - it is in dependencyManagement

Now the flatten-maven plugin has a bug - that can not handle this case.

whilst I generally agree that you should declare what you use, the way that the current Jenkins pom works is completely broken in this regard.

The way to correctly handle this is to use the jenkins-bom - but this support is also currently broken in the plugin-pom.

[jtnord]

temporary workaround is ok, but comment needs to be fixed. we do not want to encourage plugins to use this.

[jtnord]

it is not supposed to be usable by plugins, so this comment is wrong.
the dependencies for slf4j are in dependencyManagement or the bom so you should omit the version entirely.

sadly the flatten maven plugin can not cope with this, but the comment should really be addressed that this should not be used and is only a workaround for plugins producing incrementals and also declaring a dependency on and slf4j artifact.

[proski]

OK, I've rephrased the comment. I hope I got it right.

I don't insist on this change. Stash Pull Request Builder plugin can use plugin-pom 3.53 for now and switch to BOM once it becomes more reasonable to require a newer Jenkins.

[proski]

Speaking of the newer Jenkins - README.md is this repository implies that Jenkins 2.195 should be required, which is newer than the latest stable Jenkins version - way too new for plugin developers.
However, the bom package provides retroactive dependencies for stable branches starting with 2.138.x and its README.md recommends setting jenkins.version to 2.138.4, which is entirely reasonable.
bitbucket-branch-source-plugin has already switched to BOM and it requires Jenkins 2.138.4. I'm going to explore a similar change in Stash Pull Request Builder.
I think README.md in this repository should be clarified.

[jetersen]

jenkins-bom and plugin-bom is not the same @proski

This is mostly for aligning plugins: https://github.com/jenkinsci/bom
This is jenkins bom: https://github.com/jenkinsci/jenkins/blob/master/bom/pom.xml

[jtnord]

!!Do not use the BOM profile!!

[jetersen]

@jtnord too late? plenty of plugins use it.
https://github.com/search?q=org%3Ajenkinsci+%22io.jenkins.tools.bom%22+extension%3Axml&type=Code

[jtnord]

wrong bom I'm talking about the profile so more bom confusion :)

[jtnord]

should be fixed in #269

[oleg-nenashev]

Closing it after https://github.com/jenkinsci/plugin-pom/releases/tag/plugin-4.0-beta-1 . BOM should address the root cause issue. Thanks @proski for your pull request anyway!