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

Missing dependency hides circular dependency #4138

Closed
peiyuwang opened this Issue Dec 13, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@peiyuwang
Contributor

peiyuwang commented Dec 13, 2016

While working on #4136 I noticed the following circular dependency:

Exception message: Cycle detected:
	src/python/pants/backend/jvm/targets:jvm ->
	src/python/pants/backend/jvm/subsystems:junit ->
	src/python/pants/backend/jvm/subsystems:shader ->
	src/python/pants/backend/jvm/targets:jvm

The corresponding class level cycle is JUnitTests -> Junit -> Shader -> JarDependency

It didn't break is only because the targets:jvm to subsystems:junit dependency is currently missing, which actually exists (junit_tests.py needs JUnit). As soon as we add the missing dependency, it will break.

diff --git a/src/python/pants/backend/jvm/targets/BUILD b/src/python/pants/backend/jvm/targets/BUILD
index eee27ff..e7bbe16 100644
--- a/src/python/pants/backend/jvm/targets/BUILD
+++ b/src/python/pants/backend/jvm/targets/BUILD
@@ -33,6 +33,7 @@ python_library(
     '3rdparty/python/twitter/commons:twitter.common.dirutil',
     '3rdparty/python:six',
     'src/python/pants/backend/jvm/subsystems:java',
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/backend/jvm/subsystems:jvm_platform',
     'src/python/pants/backend/jvm:jar_dependency_utils',
     'src/python/pants/base:build_environment',

We might need to break src/python/pants/backend/jvm/targets:jvm into smaller ones.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 18, 2017

Contributor

Turns out there's an even more direct cycle:

JUnitTests -> Junit -> JarLibrary

Contributor

benjyw commented Mar 18, 2017

Turns out there's an even more direct cycle:

JUnitTests -> Junit -> JarLibrary

benjyw added a commit to benjyw/pants that referenced this issue Mar 18, 2017

Fix missing and circular deps.
The main fixes are:

- Move jar_dependency*.py and exclude.py out of the JVM backend
  and into core (speifically, the already existing pants.java.jar
  package, which seems like a logical place).

- Move a utility function from classpath_util.py to classpath_products.py
  (and change the corresponding tests appropriately).

- A few random missing deps noticed along the way.

These changes, while intrusive, are necessary to fix the dep cycles
mentioned in the issue below.  Fortunately they are also quite logical:
In particular, since JarDependency is not a target, it makes sense for
it to live outside pants.backend.jvm.targets.  And moving it into core
paves the way for using JVM-based tools in non-JVM backends without
having to depend on the entire JVM backend (e.g., python ANTLR codegen
needs to run a JVM, but doesn't care about JVM source code in the repo).

Addresses pantsbuild#4138.
@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 18, 2017

Contributor

Addressed in #4345

Contributor

benjyw commented Mar 18, 2017

Addressed in #4345

benjyw added a commit to benjyw/pants that referenced this issue Mar 18, 2017

Fix missing and circular deps.
The main fixes are:

- Move jar_dependency*.py and exclude.py out of the JVM backend
  and into core (speifically, the already existing pants.java.jar
  package, which seems like a logical place).

- Move a utility function from classpath_util.py to classpath_products.py
  (and change the corresponding tests appropriately).

- A few random missing deps noticed along the way.

These changes, while intrusive, are necessary to fix the dep cycles
mentioned in the issue below.  Fortunately they are also quite logical:
In particular, since JarDependency is not a target, it makes sense for
it to live outside pants.backend.jvm.targets.  And moving it into core
paves the way for using JVM-based tools in non-JVM backends without
having to depend on the entire JVM backend (e.g., python ANTLR codegen
needs to run a JVM, but doesn't care about JVM source code in the repo).

Addresses pantsbuild#4138.

benjyw added a commit that referenced this issue Mar 18, 2017

Fix missing and circular deps. (#4345)
The main fixes are:

- Move jar_dependency*.py and exclude.py out of the JVM backend
  and into core (specifically, the already existing pants.java.jar
  package, which seems like a logical place).

- Move a utility function from classpath_util.py to classpath_products.py
  (and change the corresponding tests appropriately).

- A few random missing deps noticed along the way.

These changes, while intrusive, are necessary to fix the dep cycles
mentioned in the issue below.  Fortunately they are also quite logical:
In particular, since JarDependency is not a target, it makes sense for
it to live outside pants.backend.jvm.targets.  And moving it into core
paves the way for using JVM-based tools in non-JVM backends without
having to depend on the entire JVM backend (e.g., python ANTLR codegen
needs to run a JVM, but doesn't care about JVM source code in the repo).

Addresses #4138.
@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 18, 2017

Contributor

Fixed in #4345.

Contributor

benjyw commented Mar 18, 2017

Fixed in #4345.

@benjyw benjyw closed this Mar 18, 2017

lenucksi added a commit to lenucksi/pants that referenced this issue Apr 25, 2017

Fix missing and circular deps. (#4345)
The main fixes are:

- Move jar_dependency*.py and exclude.py out of the JVM backend
  and into core (specifically, the already existing pants.java.jar
  package, which seems like a logical place).

- Move a utility function from classpath_util.py to classpath_products.py
  (and change the corresponding tests appropriately).

- A few random missing deps noticed along the way.

These changes, while intrusive, are necessary to fix the dep cycles
mentioned in the issue below.  Fortunately they are also quite logical:
In particular, since JarDependency is not a target, it makes sense for
it to live outside pants.backend.jvm.targets.  And moving it into core
paves the way for using JVM-based tools in non-JVM backends without
having to depend on the entire JVM backend (e.g., python ANTLR codegen
needs to run a JVM, but doesn't care about JVM source code in the repo).

Addresses pantsbuild#4138.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment