ACCUMULO-4618 Enforce dependency analysis during build #239
Conversation
mikewalch
commented
Apr 4, 2017
mikewalch
commented
- Build will fail if module has missing or unused dependency
- Plugin is conifigured to ignore certain depedencies in main pom.xml
- Fixed all missing or unused dependencies in build
|
Yay dep-convergence! Looks ok to me. |
pom.xml
Outdated
| <configuration> | ||
| <failOnWarning>true</failOnWarning> | ||
| <ignoredUsedUndeclaredDependencies> | ||
| <usedUndeclaredDependency>org.apache.curator:curator-client:jar:${curator.version}</usedUndeclaredDependency> |
There was a problem hiding this comment.
Is there anything you learned in the process of adding these that might be useful to share in a comment?
* Build will fail if module has missing or unused dependency * Plugin is configured to ignore certain dependencies in main pom.xml * Fixed all missing or unused dependencies in build
| @@ -35,10 +35,6 @@ | |||
| <artifactId>commons-lang</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>log4j</groupId> | |||
| <artifactId>log4j</artifactId> | |||
There was a problem hiding this comment.
I believe this is still a runtime dependency. Runtime scoped items are added to the test classpath, and I believe we still need an actual implementation declared for test logging. (Same issue in other modules.)
There was a problem hiding this comment.
Ok I will make sure modules have slfj4j-log4j12 with test scope.
| <artifactId>junit</artifactId> | ||
| <scope>test</scope> | ||
| <groupId>xml-apis</groupId> | ||
| <artifactId>xml-apis</artifactId> |
There was a problem hiding this comment.
Where does this one come from? These APIs are built-in to the JDK. At the very least, this should probably be marked "provided", but it can get a lot more complicated than that.
There was a problem hiding this comment.
In this module (maven-plugin) and the start module, the xml-apis jar is being marked as used but undeclared by dep analysis. It looks like this is a weird classloading issue where we either need to exclude xml-apis or mark it as provided as you mentioned. I will mark it as provided in both modules and see if that works.
| @@ -422,7 +423,7 @@ | |||
| <dependency> | |||
| <groupId>org.apache.httpcomponents</groupId> | |||
| <artifactId>httpclient</artifactId> | |||
| <version>4.3.1</version> | |||
| <version>${httpclient.version}</version> | |||
There was a problem hiding this comment.
It seems like a mistake for us to declare both org.apache.httpcomponents:httpclient and commons-httpclient:httpclient. Are we adding versions for dependency management within our transitive dependencies, or are we actually using both ourselves?
There was a problem hiding this comment.
We were actually using both. It looks org.apache.httpcomponents:httpclient is newer so I will see if we can only use that.
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-io</artifactId> | ||
| <scope>runtime</scope> |
There was a problem hiding this comment.
Pretty sure we still need these jetty libs at runtime for when trying to run tests in Eclipse, or during ITs, or just running the monitor manually out of your IDE for testing.
There was a problem hiding this comment.
They were being marked as declared but unused. They are pulled in by the jetty-server jar so I think this removal is fine.
[INFO] +- org.eclipse.jetty:jetty-server:jar:9.2.17.v20160517:compile
[INFO] | +- org.eclipse.jetty:jetty-http:jar:9.2.17.v20160517:compile
[INFO] | \- org.eclipse.jetty:jetty-io:jar:9.2.17.v20160517:compile
|
@joshelser just to be clear, this doesn't do dependency convergence enforcement; it does dependency declaration enforcement, which isn't quite the same thing. In fact, it can actually lead to dependency divergence. (Explicit version declarations mean greater potential for conflict with inherited transitive deps.) However, it does set us up to understand our real dependencies better, which could help in future, if we pursue convergence. And that's something to be excited about. |
|
+1 Late to comment here, sorry. I am in favor of convergence and started working it in https://issues.apache.org/jira/browse/ACCUMULO-762. Thanks for doing this Mike |
