ARROW-1379: [Java] adding maven-dependency-plugin and fixing all reported dependency errors #978

antonymayi · 2017-08-19T21:39:10Z

There is couple of dependency issues in the current maven config. This is then leaking into the integrating project which then needs to specify foreign dependencies just because arrow doesn't list them properly or is pulling unnecessary dependencies just because arrow lists them improperly.

arrow-format

[WARNING] Unused declared dependencies found:
[WARNING]    org.slf4j:slf4j-api:jar:1.7.25:compile
[WARNING]    com.vlkan:flatbuffers:jar:1.2.0-3f79e055:compile
[WARNING]    io.netty:netty-handler:jar:4.0.49.Final:compile
[WARNING]    com.google.guava:guava:jar:18.0:compile

arrow-memory

[WARNING] Used undeclared dependencies found:
[WARNING]    io.netty:netty-buffer:jar:4.0.49.Final:compile
[WARNING]    io.netty:netty-common:jar:4.0.49.Final:compile
[WARNING] Unused declared dependencies found:
[WARNING]    com.carrotsearch:hppc:jar:0.7.2:compile
[WARNING]    io.netty:netty-handler:jar:4.0.49.Final:compile

arrow-tools

[WARNING] Used undeclared dependencies found:
[WARNING]    com.fasterxml.jackson.core:jackson-databind:jar:2.7.9:compile
[WARNING]    com.fasterxml.jackson.core:jackson-core:jar:2.7.9:compile
[WARNING] Unused declared dependencies found:
[WARNING]    org.apache.commons:commons-lang3:jar:3.6:compile
[WARNING]    org.apache.arrow:arrow-format:jar:0.7.0-SNAPSHOT:compile
[WARNING]    io.netty:netty-handler:jar:4.0.49.Final:compile

arrow-vector

[WARNING] Used undeclared dependencies found:
[WARNING]    com.google.code.findbugs:jsr305:jar:3.0.2:compile
[WARNING]    com.vlkan:flatbuffers:jar:1.2.0-3f79e055:compile
[WARNING]    io.netty:netty-common:jar:4.0.49.Final:compile
[WARNING]    io.netty:netty-buffer:jar:4.0.49.Final:compile
[WARNING]    com.fasterxml.jackson.core:jackson-core:jar:2.7.9:compile
[WARNING] Unused declared dependencies found:
[WARNING]    org.apache.commons:commons-lang3:jar:3.6:compile
[WARNING]    io.netty:netty-handler:jar:4.0.49.Final:compile

I am proposing this PR to:

Add maven-dependency-plugin to enforce all dependencies are always listed corrctly
Fixing all the current dependency issues

antonymayi · 2017-08-19T22:07:33Z

I can see the build is failing but not because of my changes. Getting same error on master:

chmod: cannot access ‘/home/travis/build/apache/arrow/java/format/target/flatc-linux-x86_64-1.2.0-3f79e055.exe’: No such file or directory
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.4.0:exec (script-chmod) on project arrow-format: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]

wesm · 2017-08-20T04:05:33Z

One of @jacques-n, @StevenMPhillips, @siddharthteotia should take a look at this

jacques-n · 2017-08-20T21:07:30Z

@siddharthteotia, can you troubleshoot the master build for this?

antonymayi · 2017-08-20T21:30:01Z

apologies, master builds ok, I confused myself when checking (was too late hour for me). and this actually is problem of my PR as the dependency plugin collides with another instance in arrow-format which is using it to pull the flatc.exe.

please leave it with me to resolve and sorry again.

…n arrow-vector

antonymayi · 2017-08-20T23:59:00Z

Looks the maven-dependency-plugin is less useful for generated sources as in the arrow-vector module (it can analyze only direct sources, not the generated ones). I am using the ignoredUnusedDeclaredDependency tag for the genuine deps from generated sources to let it pass. This means any future extra dependencies coming from the generated sources would need to be whitelisted this way.

Current state of this PR:

all dependencies cleaned (added missing ones, redundant removed)
some of the repeating dependencies reorganized through dependencyManagement section in the parent pom
added maven-dependency-plugin to all modules (through pluginManagement in root pom) to enforce dependencies are declared properly
since this doesn't work for generated sources, the arrow-vector module has a whitelist of dependencies to be ignored for the unused-checks

jacques-n · 2017-08-21T00:54:19Z

java/format/pom.xml

    <flatc.download.skip>false</flatc.download.skip>
    <flatc.executable>${project.build.directory}/flatc-${os.detected.classifier}-${fbs.version}.exe</flatc.executable>
    <flatc.generated.files>${project.build.directory}/generated-sources/flatc</flatc.generated.files>
-    <maven-compiler-plugin.version>3.3</maven-compiler-plugin.version>


I don't see any references to this property anywhere so thought this is some leftover garbage?

jacques-n · 2017-08-21T00:54:39Z

java/format/pom.xml

    <flatc.executable>${project.build.directory}/flatc-${os.detected.classifier}-${fbs.version}.exe</flatc.executable>
    <flatc.generated.files>${project.build.directory}/generated-sources/flatc</flatc.generated.files>
-    <maven-compiler-plugin.version>3.3</maven-compiler-plugin.version>
-    <maven-dependency-plugin.version>2.10</maven-dependency-plugin.version>


same here. I think we added both of these previously since some versions of mvn 3 caused issues without these.

re the dependency plugin - I defined the dependency plugin globals in the pluginManagement in the parent pom, this way it gets propagate to all instances of this plugin in child poms.

re the compiler - see my separate answer above. if it is desirable to stick it to particular version I would recommend putting it into the pluginManagement section of the parent similarly to what I did with the dependency plugin.

I think it already is in pluginManagement (as I updated it for Java9 support) in the parent pom

👍 I knew I saw it somewhere

jacques-n · 2017-08-21T00:55:43Z

java/pom.xml

    <forkCount>2</forkCount>
-    <jackson.version>2.7.9</jackson.version>
+    <dep.jackson.version>2.7.9</dep.jackson.version>
    <hadoop.version>2.7.1</hadoop.version>


missed "dep." here

are you referring to the hadoop.version? I didn't touch that one actually. I was tempted to prepend it with dep. to make it consistent with the rest but thought it is out of scope of my PR... happy to do that though.

hadoop.version now renamed to dep.hadoop.version. this still leaves the fbs.version in its original form without the prefix, not sure if that should be renamed too but it is kind of special one so I am leaving it

jacques-n · 2017-08-21T00:58:11Z

java/pom.xml

            <execution>
-              <id>validate</id>
-              <phase>validate</phase>
+              <id>analyze</id>


please describe the purpose for all the checkstyle changes

actually no changes to the checkstyle plugin configuration. this is just the git diff making it to look very confusing as it is mixing lines from different part in the diff report... these changes actually belong to the dependency plugin, not the checkstyle. see the full pom.

I don't think there's any reason not to run the dependency plugin in the parent pom, so maybe the plugin could be configured in the parent (and inherited by children) instead of each child declaring the plugin?

that was actually my very first approach as well but the arrow-format is already using the maven-dependency-plugin and its goal copy to fetch the flatc.exe. if I run the dependency plugin in the parent pom (with the analyze-only goal) then it shadows the copy goal instance in the arrow-format child.

I was trying to configure it so that the arrow-format is just extending the configuration adding/merging the copy goal execution with the inherited analyze-only global execution but had no luck with any sensible approach (it is only possible to reduce/activate some executions in child pom by overriding its phase to/from none but not adding completely new one; so the only possible way would be defining the plugin in the root pom with both analyze-only execution (for global dependency enforcements as per tis subj) and also the copy execution (ie with phase being none so that it can be used in the arrow-format child (setting the phase only in that module to generate-source) - but specifying the copy-flatc execution in the parent pom would be ugly, hence my final approach with defining the analyze-only execution in pluginManagement and letting each module to instantiate (and override) it itself).

so if you know a way to move it to the parent while having arrow-format still pulling the flatc.exe correctly that would indeed be my preferred way also...

I suspect a maven bug here: when adding an execution for dependency plugin in the parent pom, the execution copy-flatc is moved after script-chmod. The merge is okay, but the order is wrong (Maven is supposed to use declaration order).

One way to workaround this would be move copy-flatc to an earlier phase like initialize.

I like this approach - having the analyze-only execution directly in the parent pom is much better and moving the copy-flatc to initialize looks almost like the right phase anyway.

so this is now in place in this PR.

laurentgo · 2017-08-21T20:22:15Z

java/vector/pom.xml

+        <executions>
+          <execution>
+            <id>analyze</id>
+            <configuration>


Instead, let's fix vector/src/main/codegen//includes/vv_imports.ftl to remove the unnecessary import

👍 nice catch, done.

laurentgo · 2017-08-21T20:23:35Z

java/tools/pom.xml

          <groupId>ch.qos.logback</groupId>
          <artifactId>logback-classic</artifactId>
          <version>1.2.3</version>
          <scope>run</scope>


can you fix the scope too? (it's runtime, not run)

parent pom

laurentgo · 2017-08-23T05:48:17Z

LGTM (non-binding)

wesm · 2017-08-23T14:28:35Z

cool, @jacques-n, last look? We will want to harden this in downstream projects before releasing 0.7.0 in case there are any problems

jacques-n · 2017-08-26T15:58:35Z

LGTM too

…rted dependency errors There is couple of dependency issues in the current maven config. This is then leaking into the integrating project which then needs to specify foreign dependencies just because arrow doesn't list them properly or is pulling unnecessary dependencies just because arrow lists them improperly. * ```arrow-format``` ``` [WARNING] Unused declared dependencies found: [WARNING] org.slf4j:slf4j-api:jar:1.7.25:compile [WARNING] com.vlkan:flatbuffers:jar:1.2.0-3f79e055:compile [WARNING] io.netty:netty-handler:jar:4.0.49.Final:compile [WARNING] com.google.guava:guava:jar:18.0:compile ``` * ```arrow-memory``` ``` [WARNING] Used undeclared dependencies found: [WARNING] io.netty:netty-buffer:jar:4.0.49.Final:compile [WARNING] io.netty:netty-common:jar:4.0.49.Final:compile [WARNING] Unused declared dependencies found: [WARNING] com.carrotsearch:hppc:jar:0.7.2:compile [WARNING] io.netty:netty-handler:jar:4.0.49.Final:compile ``` * ```arrow-tools``` ``` [WARNING] Used undeclared dependencies found: [WARNING] com.fasterxml.jackson.core:jackson-databind:jar:2.7.9:compile [WARNING] com.fasterxml.jackson.core:jackson-core:jar:2.7.9:compile [WARNING] Unused declared dependencies found: [WARNING] org.apache.commons:commons-lang3:jar:3.6:compile [WARNING] org.apache.arrow:arrow-format:jar:0.7.0-SNAPSHOT:compile [WARNING] io.netty:netty-handler:jar:4.0.49.Final:compile ``` * ```arrow-vector``` ``` [WARNING] Used undeclared dependencies found: [WARNING] com.google.code.findbugs:jsr305:jar:3.0.2:compile [WARNING] com.vlkan:flatbuffers:jar:1.2.0-3f79e055:compile [WARNING] io.netty:netty-common:jar:4.0.49.Final:compile [WARNING] io.netty:netty-buffer:jar:4.0.49.Final:compile [WARNING] com.fasterxml.jackson.core:jackson-core:jar:2.7.9:compile [WARNING] Unused declared dependencies found: [WARNING] org.apache.commons:commons-lang3:jar:3.6:compile [WARNING] io.netty:netty-handler:jar:4.0.49.Final:compile ``` I am proposing this PR to: 1. Add maven-dependency-plugin to enforce all dependencies are always listed corrctly 2. Fixing all the current dependency issues Author: Antony Mayi <[email protected]> Author: Stepan Kadlec <[email protected]> Closes apache#978 from antonymayi/master and squashes the following commits: d7f081e [Antony Mayi] moving `copy-flatc` to initialize phase and `analyze` execution to parent pom ec72717 [Antony Mayi] removing unused apache.commons.lang3, fixing pom 8cbfe5f [Antony Mayi] maven-dependency-plugin: ignoring dependencies of generated sources in arrow-vector dc833bb [Stepan Kadlec] adding maven-dependency-plugin and fixing all reported dependency errors

adding maven-dependency-plugin and fixing all reported dependency errors

dc833bb

maven-dependency-plugin: ignoring dependencies of generated sources i…

8cbfe5f

…n arrow-vector

jacques-n reviewed Aug 21, 2017

View reviewed changes

laurentgo reviewed Aug 21, 2017

View reviewed changes

antonymayi added 2 commits August 21, 2017 23:16

removing unused apache.commons.lang3, fixing pom

ec72717

moving copy-flatc to initialize phase and analyze execution to

d7f081e

parent pom

asfgit closed this in ea503a5 Aug 26, 2017

ARROW-1379: [Java] adding maven-dependency-plugin and fixing all reported dependency errors #978

ARROW-1379: [Java] adding maven-dependency-plugin and fixing all reported dependency errors #978

Uh oh!

Conversation

antonymayi commented Aug 19, 2017

Uh oh!

antonymayi commented Aug 19, 2017

Uh oh!

wesm commented Aug 20, 2017

Uh oh!

jacques-n commented Aug 20, 2017

Uh oh!

antonymayi commented Aug 20, 2017

Uh oh!

antonymayi commented Aug 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurentgo Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonymayi Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonymayi Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurentgo commented Aug 23, 2017

Uh oh!

wesm commented Aug 23, 2017

Uh oh!

jacques-n commented Aug 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laurentgo Aug 21, 2017 •

edited

Loading

antonymayi Aug 21, 2017 •

edited

Loading

antonymayi Aug 22, 2017 •

edited

Loading