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

published incremental poms have dependencyManagement stripped causing incorrect dependency resolution #705

Open
jtnord opened this issue Mar 9, 2023 · 23 comments · May be fixed by #706
Open
Assignees
Labels

Comments

@jtnord
Copy link
Member

jtnord commented Mar 9, 2023

Jenkins and plugins versions report

Environment ci.jenkins.io - N/A

A plugin that is using incrementals/CD publishing has its pom mangled by the maven-flatten-plugin

this flattening removes the dependencyManagement entries as can be seen between this repository file and this published artifact

As the dependencies on kotlin are transitive they are not included in the flattened pom, but are included in the hpi.

when the plugin is depended on by another plugin the result is the dependency manamgemtn is gone so you get the transitive version of the dependencies, not the version that the plugin was built with and bundled.

This not only causes errors for consumers - it also makes the behaviour of a build different in your IDE than when deployed - as in the IDE with workspace resolution the dependencies would be correct, but once released or in CI they would be different.

Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 paths to dependency are:
+-org.jenkins-ci.plugins:github-api:1.303-999999-SNAPSHOT
ohttp-api-plugin --->    +-io.jenkins.plugins:okhttp-api:4.10.0-125.v3593b_a_f8c97b_
  +-com.squareup.okhttp3:logging-interceptor:4.10.0
    +-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10
and
+-org.jenkins-ci.plugins:github-api:1.303-999999-SNAPSHOT
ohttp-api-plugin --->  +-io.jenkins.plugins:okhttp-api:4.10.0-125.v3593b_a_f8c97b_
  +-com.squareup.okio:okio:3.3.0
    +-com.squareup.okio:okio-jvm:3.3.0
      +-org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.0
]

jenkinsci/github-api-plugin#200 (review)

What Operating System are you using (both controller, and any agents involved in the problem)?

N/A

Reproduction steps

jenkinsci/github-api-plugin#200 (review)

Expected Results

no enforcer error - dependency:tree would show the same versions of kotlin both in the okhttp-api plugin and in any other consumer of it that does not itself depend on kotlin

Actual Results

dependencyManagement is stripped, downstream consumers get incorrect library versions.

Anything else?

No response

@jtnord jtnord added the bug label Mar 9, 2023
@jtnord jtnord changed the title publish incremental poms have dependencyManagement stripped causing incorrect dependency resolution published incremental poms have dependencyManagement stripped causing incorrect dependency resolution Mar 9, 2023
@jtnord jtnord self-assigned this Mar 9, 2023
jtnord added a commit to jtnord/plugin-pom that referenced this issue Mar 9, 2023
dependencyManagement can be used to manage the transitive dependency
versions not only direct ones, and this impacts what can end up in the
resulting hpi file.

By not including the depMgmt section then any downtream consumer will
see the un managed versions of the dependencies which will differ from
what they should see
@jtnord jtnord linked a pull request Mar 9, 2023 that will close this issue
6 tasks
@jglick
Copy link
Member

jglick commented Mar 9, 2023

As the dependencies on kotlin are transitive they are not included in the flattened pom, but are included in the hpi.

So this seems wrong. Why are the dependencies not in the flattened POM?

My suspicious eye falls on gradle-module-metadata-maven-plugin but deleting that does not change anything.

@jtnord
Copy link
Member Author

jtnord commented Mar 9, 2023

why are the dependencies not in the flattened POM

because the flattened pom is not promoting them from transitive dependencies to direct dependencies.

you depend on say library X. you do not list all its dependencies - you just get them.

now the flatten plugin does have a way to do this - but this breaks the maven hierachy as mentioned in the PR - as the depth first wins changes as you promote something from Depth N to depth 1 and then in other plugins it will go from N+2 to 2.

(it will also promote lots of things that really should not be promoted too - as it promotes everything from plugins that you depend on!)

@jglick
Copy link
Member

jglick commented Mar 9, 2023

How widespread is the issue? This case is the first I can recall being a problem so I wondering what about this plugin is different. Can it simply declare all the Kotlin dependencies as direct in its POM? Since it is importing the BOM that would not really increase maintenance cost.

Or exclude kotlin-stdlib-jdk8 from logging-interceptor? Are excludes included in the flattened POM?

@jtnord
Copy link
Member Author

jtnord commented Mar 9, 2023

How widespread is the issue?

Anything that uses dependencyManagement to manage transitive dependencies. Can not answer - but upstream BOMs are getting more common - jackson2-api is /probably/ fine as jackson tends not to depend on anything 3rd party but I have not checked.

This case is the first I can recall being a problem so I wondering what about this plugin is different. Can it simply declare all the Kotlin dependencies as direct in its POM? Since it is importing the BOM that would not really increase maintenance cost.

it does in so much as if new transitive deps are added (or removed) then they can go unnoticed. (very few people checked the maven-hpi log for the WARNING bundling transitive dependency - less still when the bump if via dependabot!)

Or exclude kotlin-stdlib-jdk8 from logging-interceptor? Are excludes included in the flattened POM?

excludes can be in the flattened pom, and according to the documentation they are by default - but given we are here because the documentation lies....

@jglick
Copy link
Member

jglick commented Mar 9, 2023

very few people checked the maven-hpi log for the WARNING bundling transitive dependency

Unfortunately true, and I have thought of upgrading this to an error, though that would require a lot of adaptation (and, as you point out, leave behind stray deps when removed upstream).

@jtnord
Copy link
Member Author

jtnord commented Mar 9, 2023

very few people checked the maven-hpi log for the WARNING bundling transitive dependency

Unfortunately true, and I have thought of upgrading this to an error, though that would require a lot of adaptation (and, as you point out, leave behind stray deps when removed upstream).

I had in mind but had never done, adding an enforcer rule that checked the contents of WEB-INF/lib where you had to specify what you expected, or at least how many things you expected which fixes the issue with stray-deps but also would require migration (or at least some form of opt-in), but we are now off topic :)

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

this probably also needs duplicating to the main jenkins parent pom which is used by things like jenkins-test-harness which imports the jetty-bom

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

wwwait what... https://issues.apache.org/jira/browse/MNG-7003 (https://issues.apache.org/jira/browse/MNG-5761)

is the use of a bom in Maven useless when consuming a project that uses it for transitive dependencies 😱 so even if we did do this it would not work correctly

@jglick
Copy link
Member

jglick commented Mar 10, 2023

@jglick
Copy link
Member

jglick commented Mar 10, 2023

apache/maven#1000 is interesting but it would be more interesting if for a given packaging you could pick a dependency manager…especially one which behaves like what RequireUpperBoundsDeps enforces, rather than this crazy tracking of dependency depth.

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

So reconsider https://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#flattenDependencyMode ?

yup. I guess this should be safe even with a different dependency tree when having multiple plugins in your IDE vs consuming a plugin dep from the repository as you would (should) have a failure from RequireUpperBoundsDeps if there where any conflicts. and if there where no conflicts then you get the same version at any rate.

But I will need to check that it behaves correctly with plugin dependencies - I bet 💵💵 it won't which would be an issue. (all transitive dependencies from plugin X leak into plugin Y when it depends on X.

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

@jglick
Copy link
Member

jglick commented Mar 10, 2023

Interesting, adds <exclusions> automatically. Looks right to me. Again, hard to predict what the effects would be on corner cases in the future.

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

ass I thought - flattenMode all is no good as it promotes all transitive dependencies including those from other plugins which would completely mess with plugin BOM and hightly probably the PCT, as the plugins would be updated but you would get their old deps not the new ones from the transitive plugins

https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/github-api/1.303-419.vfca_9e79c9635/

note that github-api depends on several other plugins.

short of getting depedencyManagement fixed I think the only alternative is to not use flatten at all but replace it with an hpi aware alternative that adds the transitive dependencies that would get bundled into the hpi, but nothing that would come via another plugin.

@jglick
Copy link
Member

jglick commented Mar 10, 2023

Maybe just work around in okhttp-api for now (by declaring at least the offending lib dep directly) and sit on this unless and until it seems like a broader problem, or some clarity is reached regarding how Maven 4 will handle these issues? My understanding of the problem in this case is that it bundles an artifact which is available via two distinct dependency trails from other bundled artifacts, and which would be using conflicting versions were it not for the imported BOM. Certainly legal but seems like a relatively unusual case.

@timja
Copy link
Member

timja commented Mar 10, 2023

Couple more places it's popped up in:
jenkinsci/junit-plugin#505
jenkinsci/skip-notifications-trait-plugin#62

jtnord added a commit to jtnord/okhttp-api-plugin that referenced this issue Mar 10, 2023
Due to jenkinsci/plugin-pom#705 and
https://issues.apache.org/jira/browse/MNG-7003 using
dependencyManagement to manage the transitive dependencies is not a good
idea if you are going to be consumed by something else.

this keeps the dependencyManagement so we can manage the verions as one,
but where we use a transitive dependency that has had its version
managed we inline it into the dependencies
@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

Maybe just work around in okhttp-api for now (by declaring at least the offending lib dep directly) and sit on this unless and until it seems like a broader problem, or some clarity is reached regarding how Maven 4 will handle these issues? My understanding of the problem in this case is that it bundles an artifact which is available via two distinct dependency trails from other bundled artifacts, and which would be using conflicting versions were it not for the imported BOM. Certainly legal but seems like a relatively unusual case.

my understanding is different.

any plugin that bundles a transitive dependency (not declared inline) whose version is defined by an entry in dependencyManagement (either directly or via a bom) will when it is consumed / used by any other plugin not get the version that you would get from the plugin, but whatever version is defined without the bom.

There does not need to be a conflict or different paths to the dependency - which is the very scary part.

for example just add a dependency to okhttp from a plain project and compare the dependency:tree of them

e.g.

<?xml version="1.0" encoding="UTF-8"?>
<project>
  <modelVersion>4.0.0</modelVersion>
  <groupId>foo</groupId>
  <artifactId>bar</artifactId>
  <version>1.0-SNAPSHOT</version>
  <dependencies>
    <dependency>
      <groupId>io.jenkins.plugins</groupId>
      <artifactId>okhttp-api</artifactId>
      <version>4.10.0-125.v3593b_a_f8c97b_</version>
    </dependency>
  </dependencies>
</project>

produces

[INFO] foo:bar:jar:1.0-SNAPSHOT
[INFO] \- io.jenkins.plugins:okhttp-api:jar:4.10.0-125.v3593b_a_f8c97b_:compile
[INFO]    +- com.squareup.okio:okio:jar:3.3.0:compile
[INFO]    |  \- com.squareup.okio:okio-jvm:jar:3.3.0:compile
[INFO]    |     \- org.jetbrains.kotlin:kotlin-stdlib-common:jar:1.8.0:compile
[INFO]    +- com.squareup.okhttp3:okhttp:jar:4.10.0:compile
[INFO]    |  \- org.jetbrains.kotlin:kotlin-stdlib:jar:1.6.20:compile
[INFO]    |     \- org.jetbrains:annotations:jar:13.0:compile
[INFO]    \- com.squareup.okhttp3:logging-interceptor:jar:4.10.0:compile
[INFO]       \- org.jetbrains.kotlin:kotlin-stdlib-jdk8:jar:1.6.10:compile
[INFO]          \- org.jetbrains.kotlin:kotlin-stdlib-jdk7:jar:1.6.10:compile

notice the 1.6.10 versions of kotlin from logging-interceptor 4.10.0
also note that the cersion of the common-jar is 1.8.0 and not 1.8.10

this means even if there are no conflicts if you ran mvn test or compile you are getting the lower versions not what is shipped, and so you could be linking against some signatures that have disappeared in some incompatible way that works perfectly fine everywhere outside of a Jenkins instance.

@jtnord
Copy link
Member Author

jtnord commented Mar 10, 2023

so irrespective of the maven bug I believe it would be beneficial to publish the dependencyManagement section today (expanded).

This would give parity with anything produced by maven-release-plugin so having these entries would not leave us in a worse position - as we would still need to deal with any fallout should it arise.

My take on any fallout is that if it does arise whilst it could be surprise it will actually fix things and break workarounds that people have used rather than break something that was well setup and working as expected.

flattening dependencies would lead to tedious occurrences where you need to exclude a tree of dependencies (one exclude becomes N and fragile)

@jglick
Copy link
Member

jglick commented Mar 10, 2023

There does not need to be […] different paths to the dependency

No? Is there a realistic example of a case where there is only a single path? I think you could construct one but it would be artificial—the normal reason to bother importing a BOM when not needed to align versions of direct dependencies would be that there are different paths to some transitive dep, with different versions.

@jtnord
Copy link
Member Author

jtnord commented Mar 15, 2023

there are probably lots we do not know about - as upperBounds will only tells you when there is a conflict, not that a plugin version was "downgraded"

realistic example (you do not need a BOM - just a regular dep inside a dependencyManagement section) (this is constructed - but happens - esp when a transitive dep has a vulnerability)

plugin A depends on library B
library B is rarely released but relies on library C (old version 2.03)
plugin A uses dependencyManagement to bundle newer version of C to 2.76 (does not declare direct dependency - which is correct)

plugin X depends on plugin A.

inside Jenkins plugin X will see Library C at the newer version 2.76 when building and running tests it will see 2.03

There is a single path to the dependency and as dependencyManagement is ignored plugin X will get the dependency version of C that is declared in library B's pom.

@jglick
Copy link
Member

jglick commented Jan 8, 2024

apache/maven#1357 👀

@jtnord
Copy link
Member Author

jtnord commented Feb 8, 2024

https://issues.apache.org/jira/browse/MNG-7982 released in maven 4.0.0-alpha12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants