Skip to content

Conversation

proski
Copy link
Contributor

@proski proski commented Aug 7, 2019

While "provided" makes no difference to maven in presence of "optional", it would reassure plugin writers that they too can depend on "provided" slf4j-api and rely on it being present on Jenkins.

While "provided" makes no difference to maven in presence of "optional",
it would reassure plugin writers that they too can depend on "provided"
slf4j-api and rely on it being present on Jenkins.
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.

+1

@oleg-nenashev oleg-nenashev changed the title Use "provided" scope for slf4j-api dependency Use "provided" scope for slf4j-api dependency (instead of compile) Aug 12, 2019
@oleg-nenashev oleg-nenashev requested a review from jglick August 14, 2019 09:48
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Probably OK, but we should be enhancing the IT for a simple plugin to verify that WEB-INF/lib/*.jar contains only the plugin JAR: this would fail if you accidentally used compile without optional, for example.

@@ -286,7 +286,7 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>${slf4jVersion}</version>
<scope>compile</scope>
<scope>provided</scope>
<!-- mark the API as optional so it is not packaged in the HPI but available during compile -->
<optional>true</optional>
Copy link
Member

Choose a reason for hiding this comment

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

In this case we could delete optional I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually see some uses of optional with provided. Please see spotbugs-annotations and jcip-annotations. While it's not very useful, I don't want to break the pre-existing pattern.

@jglick
Copy link
Member

jglick commented Sep 10, 2019

Note: conflicts with #229.

@proski
Copy link
Contributor Author

proski commented Sep 10, 2019

Note: conflicts with #229.

No worries, I would not mind discarding this PR is there is a better approach to declaring dependencies.

@jglick
Copy link
Member

jglick commented Sep 10, 2019

I meant “conflict” in the technical sense of Git merges. #229 does not make this change, so we could do both.

@oleg-nenashev
Copy link
Member

Let's go ahead with this PR for now, BOM is still in review

@oleg-nenashev oleg-nenashev merged commit b2b1124 into jenkinsci:master Oct 22, 2019
@oleg-nenashev
Copy link
Member

Thanks @proski !

@proski proski deleted the slf4j-provided branch November 15, 2019 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants