You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by mikewalch <gi...@git.apache.org> on 2017/04/04 17:44:05 UTC
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
GitHub user mikewalch opened a pull request:
https://github.com/apache/accumulo/pull/239
ACCUMULO-4618 Enforce dependency analysis during build
* 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
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikewalch/accumulo dep-analyze
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/239.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #239
----
commit 29f4010d108d0e5d6164b50642b9c3a8ff0fad42
Author: Mike Walch <mw...@apache.org>
Date: 2017-04-04T17:11:41Z
ACCUMULO-4618 Enforce dependency analysis during build
* 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
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109942704
--- Diff: fate/pom.xml ---
@@ -35,10 +35,6 @@
<artifactId>commons-lang</artifactId>
</dependency>
<dependency>
- <groupId>log4j</groupId>
- <artifactId>log4j</artifactId>
--- End diff --
Ok I will make sure modules have slfj4j-log4j12 with test scope.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109754583
--- Diff: pom.xml ---
@@ -840,6 +851,38 @@
</pluginManagement>
<plugins>
<plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>analyze</id>
+ <goals>
+ <goal>analyze-only</goal>
+ </goals>
+ <configuration>
+ <failOnWarning>true</failOnWarning>
+ <ignoredUsedUndeclaredDependencies>
+ <usedUndeclaredDependency>org.apache.curator:curator-client:jar:${curator.version}</usedUndeclaredDependency>
--- End diff --
Is there anything you learned in the process of adding these that might be useful to share in a comment?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #239: ACCUMULO-4618 Enforce dependency analysis during build
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/239
Yay dep-convergence! Looks ok to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #239: ACCUMULO-4618 Enforce dependency analysis during build
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on the issue:
https://github.com/apache/accumulo/pull/239
+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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/239
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109792804
--- Diff: fate/pom.xml ---
@@ -35,10 +35,6 @@
<artifactId>commons-lang</artifactId>
</dependency>
<dependency>
- <groupId>log4j</groupId>
- <artifactId>log4j</artifactId>
--- End diff --
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.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109939194
--- Diff: server/monitor/pom.xml ---
@@ -105,23 +113,13 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-http</artifactId>
- <scope>runtime</scope>
- </dependency>
- <dependency>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-io</artifactId>
- <scope>runtime</scope>
--- End diff --
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
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109794980
--- Diff: server/monitor/pom.xml ---
@@ -105,23 +113,13 @@
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-http</artifactId>
- <scope>runtime</scope>
- </dependency>
- <dependency>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-io</artifactId>
- <scope>runtime</scope>
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109929494
--- Diff: maven-plugin/pom.xml ---
@@ -66,9 +66,8 @@
<artifactId>maven-plugin-annotations</artifactId>
</dependency>
<dependency>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <scope>test</scope>
+ <groupId>xml-apis</groupId>
+ <artifactId>xml-apis</artifactId>
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109938253
--- Diff: pom.xml ---
@@ -422,7 +423,7 @@
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
- <version>4.3.1</version>
+ <version>${httpclient.version}</version>
--- End diff --
We were actually using both. It looks `org.apache.httpcomponents:httpclient` is newer so I will see if we can only use that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109794175
--- Diff: pom.xml ---
@@ -422,7 +423,7 @@
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
- <version>4.3.1</version>
+ <version>${httpclient.version}</version>
--- End diff --
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #239: ACCUMULO-4618 Enforce dependency analysis during build
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/239
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #239: ACCUMULO-4618 Enforce dependency analysis during...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/239#discussion_r109793616
--- Diff: maven-plugin/pom.xml ---
@@ -66,9 +66,8 @@
<artifactId>maven-plugin-annotations</artifactId>
</dependency>
<dependency>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
- <scope>test</scope>
+ <groupId>xml-apis</groupId>
+ <artifactId>xml-apis</artifactId>
--- End diff --
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][1].
[1]: http://stackoverflow.com/questions/11677572/dealing-with-xerces-hell-in-java-maven
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---