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

Moe Sync #486

Closed
wants to merge 2 commits into from
Closed

Moe Sync #486

wants to merge 2 commits into from

Conversation

ronshapiro
Copy link
Contributor

This code has been reviewed and submitted internally. Feel free to discuss on the PR and we can submit follow-up changes as necessary.

Commits:

Exclude Compile-Testing's circular dependency on Truth.

Maven already largely ignores this, but the circularity does cause Maven a couple problems:

  • It causes errors under dependencyConvergence (which I'm not 100% sure we want to turn on but which I've been experimenting with).
  • It makes Maven print out the dependency tree in a confusing way, with Truth listed as if it were a direct dependency of itself (rather than a dependency through Compile-Testing) and Compile-Testing's other dependencies missing entirely.

Here's the diff in Maven's dependency:tree output produced by this change:

$ ( filter() { grep -v -e SUCCESS -e Downloaded -e 'Total time' -e 'Finished at' $@; }; diff <(filter /tmp/pre) <(filter /tmp/post) )
--- /dev/fd/63 2018-08-14 13:58:21.938426152 -0400
+++ /dev/fd/62 2018-08-14 13:58:21.942426128 -0400
@@ -68,9 +68,13 @@
[INFO] | +- (com.google.j2objc:j2objc-annotations:jar:1.1:test - omitted for duplicate)
[INFO] | +- (com.google.guava:guava:jar:25.1-android:test - version managed from 25.1-jre; omitted for duplicate)
[INFO] | - (junit:junit:jar:4.12:test - version managed from 4.11; omitted for duplicate)
-[INFO] +- (com.google.truth:truth:jar:0.37:test - omitted for cycle)
[INFO] +- com.google.testing.compile:compile-testing:jar:0.15:test
-[INFO] | - (junit:junit:jar:4.12:test - version managed from 4.11; omitted for duplicate)
+[INFO] | +- (junit:junit:jar:4.12:test - version managed from 4.11; omitted for duplicate)
+[INFO] | +- (com.google.guava:guava:jar:25.1-android:test - version managed from 23.5-jre; omitted for duplicate)
+[INFO] | +- (com.google.auto.value:auto-value:jar:1.6.2:test - version managed from 1.5.3; omitted for duplicate)
+[INFO] | +- com.google.auto:auto-common:jar:0.9:test
+[INFO] | | - (com.google.guava:guava:jar:25.1-android:test - version managed from 23.5-jre; omitted for duplicate)
+[INFO] | - com.sun:tools:jar:1.8.0_151-google-v7:system
[INFO] - com.google.errorprone:error_prone_annotations:jar:2.3.1:compile
[INFO]
[INFO] ------------------------------------------------------------------------

ff1a5f0


Enable requireUpperBoundDeps and dependencyConvergence, and update Guava to 26.0.

The Guava update doesn't solve any dependency conflicts, but, as I've noted in a comment in pom.xml, it helps requireUpperBoundDeps actually work. (But other such problems might still go undetected.)

requireUpperBoundDeps has to exclude Guava, so it wouldn't have caught the problem in #473, but dependencyConvergence would have.

e8c1902

Maven already largely ignores this, but the circularity does cause Maven a couple problems:

- It causes errors under dependencyConvergence (which I'm not 100% sure we want to turn on but which I've been experimenting with).
- It makes Maven print out the dependency tree in a confusing way, with Truth listed as if it were a direct dependency of itself (rather than a dependency through Compile-Testing) and Compile-Testing's other dependencies missing entirely.

Here's the diff in Maven's dependency:tree output produced by this change:

$ ( filter() { grep -v -e SUCCESS -e Downloaded -e 'Total time' -e 'Finished at' $@; }; diff <(filter /tmp/pre) <(filter /tmp/post) )
--- /dev/fd/63  2018-08-14 13:58:21.938426152 -0400
+++ /dev/fd/62  2018-08-14 13:58:21.942426128 -0400
@@ -68,9 +68,13 @@
 [INFO] |  +- (com.google.j2objc:j2objc-annotations:jar:1.1:test - omitted for duplicate)
 [INFO] |  +- (com.google.guava:guava:jar:25.1-android:test - version managed from 25.1-jre; omitted for duplicate)
 [INFO] |  \- (junit:junit:jar:4.12:test - version managed from 4.11; omitted for duplicate)
-[INFO] +- (com.google.truth:truth:jar:0.37:test - omitted for cycle)
 [INFO] +- com.google.testing.compile:compile-testing:jar:0.15:test
-[INFO] |  \- (junit:junit:jar:4.12:test - version managed from 4.11; omitted for duplicate)
+[INFO] |  +- (junit:junit:jar:4.12:test - version managed from 4.11; omitted for duplicate)
+[INFO] |  +- (com.google.guava:guava:jar:25.1-android:test - version managed from 23.5-jre; omitted for duplicate)
+[INFO] |  +- (com.google.auto.value:auto-value:jar:1.6.2:test - version managed from 1.5.3; omitted for duplicate)
+[INFO] |  +- com.google.auto:auto-common:jar:0.9:test
+[INFO] |  |  \- (com.google.guava:guava:jar:25.1-android:test - version managed from 23.5-jre; omitted for duplicate)
+[INFO] |  \- com.sun:tools:jar:1.8.0_151-google-v7:system
 [INFO] \- com.google.errorprone:error_prone_annotations:jar:2.3.1:compile
 [INFO]
 [INFO] ------------------------------------------------------------------------

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=208682723
…ava to 26.0.

The Guava update doesn't solve any dependency conflicts, but, as I've noted in a comment in pom.xml, it helps requireUpperBoundDeps actually work. (But other such problems might still go undetected.)

requireUpperBoundDeps has to exclude Guava, so it wouldn't have caught the problem in #473, but dependencyConvergence would have.

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=208710143
@jbduncan
Copy link

@ronshapiro I understand that @cpovirk already pushed these commits to master, so I think this PR/sync is redundant. 🤔

@ronshapiro ronshapiro closed this Aug 15, 2018
@ronshapiro ronshapiro deleted the sync-master-2018/08/14 branch August 15, 2018 21:02
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.

4 participants