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

Make jar_library target export all its dependencies #4395

Merged
merged 7 commits into from Mar 29, 2017

Conversation

Projects
None yet
3 participants
@JieGhost
Contributor

JieGhost commented Mar 28, 2017

Problem

Some JarLibrary targets have dependencies on other targets (both 3rdparty or non 3rdparty).
Those dependencies are not directly used by dependees of jar_library, but has to be included in compile time classpath.

Solution

Added a new property "exports" which is same as "dependencies" to JarLibrary. The "exports" property will be picked up by "exports resolve" logic.

Also enhance the "exports resolve" logic to handle exported target being a "target" alias. In that case, we just use strict_dependencies of the target alias.

@JieGhost JieGhost changed the title from use jar_library transitive deps to use jar_library's transitive deps even when strict_deps=True Mar 28, 2017

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/compile_context.py
@@ -8,18 +8,20 @@
import zipfile
from contextlib import contextmanager
from pants.backend.jvm.targets.jar_library import JarLibrary
from pants.build_graph.aliased_target import AliasTarget
from pants.build_graph.target import Target
from pants.util.contextutil import open_zip
def _resolve_strict_dependencies(target):

This comment has been minimized.

@stuhood

stuhood Mar 28, 2017

Member

I'd like to see a comment that discusses the reasoning behind this. In particular, an alternative would have been to require exports on jar_library targets in order for them to expose their deps. In the long run, that might better align with exports being supported for more target types.

It's possible that having strict_deps not apply to JarLibrary is something that we should put behind an option, and not do by default.

@stuhood

stuhood Mar 28, 2017

Member

I'd like to see a comment that discusses the reasoning behind this. In particular, an alternative would have been to require exports on jar_library targets in order for them to expose their deps. In the long run, that might better align with exports being supported for more target types.

It's possible that having strict_deps not apply to JarLibrary is something that we should put behind an option, and not do by default.

This comment has been minimized.

@JieGhost

JieGhost Mar 28, 2017

Contributor

If I implement exports in jar_library as well, do we want to exports all dependencies for a jar_library target? I feel exporting all dependencies of a jar_library should be a default behavior because otherwise there will be no dependencies for a jar_library target in the first place.

@JieGhost

JieGhost Mar 28, 2017

Contributor

If I implement exports in jar_library as well, do we want to exports all dependencies for a jar_library target? I feel exporting all dependencies of a jar_library should be a default behavior because otherwise there will be no dependencies for a jar_library target in the first place.

This comment has been minimized.

@JieGhost

JieGhost Mar 28, 2017

Contributor

It seems JarLibrary does not have "strict_deps" field. Is it intentional? If so, then does that mean JarLibrary is default to be transitive? @stuhood

@JieGhost

JieGhost Mar 28, 2017

Contributor

It seems JarLibrary does not have "strict_deps" field. Is it intentional? If so, then does that mean JarLibrary is default to be transitive? @stuhood

This comment has been minimized.

@stuhood

stuhood Mar 28, 2017

Member

Both strict_deps and exports are currently jvm *_library-target-only concepts. The question is whether they should work with more target types in the future.

@stuhood

stuhood Mar 28, 2017

Member

Both strict_deps and exports are currently jvm *_library-target-only concepts. The question is whether they should work with more target types in the future.

This comment has been minimized.

@JieGhost

JieGhost Mar 28, 2017

Contributor

I think the question I have here is whether all dependencies of JarLibrary should be exported by default. I suppose a jar_library target has dependencies only because the compilation will fail without it, even in the pre-strict_deps era. Is there use cases where NOT all dependencies of a jar_library are needed for its dependees to compile?

@JieGhost

JieGhost Mar 28, 2017

Contributor

I think the question I have here is whether all dependencies of JarLibrary should be exported by default. I suppose a jar_library target has dependencies only because the compilation will fail without it, even in the pre-strict_deps era. Is there use cases where NOT all dependencies of a jar_library are needed for its dependees to compile?

This comment has been minimized.

@JieGhost

JieGhost Mar 28, 2017

Contributor

And how do you like Nick's suggestion below as a quick tackle for this? I can create subsequent ticket if we want to implement strict_deps and exports for jar_library (which I am still not sure if that's necessary)

@JieGhost

JieGhost Mar 28, 2017

Contributor

And how do you like Nick's suggestion below as a quick tackle for this? I can create subsequent ticket if we want to implement strict_deps and exports for jar_library (which I am still not sure if that's necessary)

This comment has been minimized.

@stuhood

stuhood Mar 28, 2017

Member

Fine with that.

@stuhood

stuhood Mar 28, 2017

Member

Fine with that.

Show outdated Hide outdated src/python/pants/backend/jvm/tasks/jvm_compile/compile_context.py
# The transitive dependencies of JarLibrary targets have
# to be included regardless of strict_deps setting,
# because they are always required for compilation.
if type(declared) in (AliasTarget, JarLibrary, Target):

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

The funny thing about implementing it this way is that it changes the ordering of the resulting classpath entries.

I think a more intention revealing way to implement this would be to add an exports property to JarLibrary that's aliased to its dependencies.

@baroquebobcat

baroquebobcat Mar 28, 2017

Contributor

The funny thing about implementing it this way is that it changes the ordering of the resulting classpath entries.

I think a more intention revealing way to implement this would be to add an exports property to JarLibrary that's aliased to its dependencies.

This comment has been minimized.

@JieGhost

JieGhost Mar 28, 2017

Contributor

That sounds like a good idea. But why does ordering matter?

@JieGhost

JieGhost Mar 28, 2017

Contributor

That sounds like a good idea. But why does ordering matter?

@JieGhost JieGhost changed the title from use jar_library's transitive deps even when strict_deps=True to Make jar_library target export all its dependencies Mar 29, 2017

@@ -25,3 +25,23 @@ scala_library(
dependencies=[':C'],
strict_deps=True,
)
target(

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

Is there a test case that exercises this target here?

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

Is there a test case that exercises this target here?

This comment has been minimized.

@JieGhost

JieGhost Mar 29, 2017

Contributor

Yeah it's in tests/python/pants_test/backend/jvm/tasks/jvm_compile/test_dep_exports_integration.py.

The test method does "list ::" in this dir, thus the new targets are automatically covered.

@JieGhost

JieGhost Mar 29, 2017

Contributor

Yeah it's in tests/python/pants_test/backend/jvm/tasks/jvm_compile/test_dep_exports_integration.py.

The test method does "list ::" in this dir, thus the new targets are automatically covered.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

ah. I see.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

ah. I see.

yield export
if type(export) in (AliasTarget, Target):
# If exported target is an alias, expand its dependencies.
for dep in _resolve_strict_dependencies(export):

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

Could you add a test case for this bit of mutual recursion? It does make sense that it would work this way, but I'd like to see some examples.

@baroquebobcat

baroquebobcat Mar 29, 2017

Contributor

Could you add a test case for this bit of mutual recursion? It does make sense that it would work this way, but I'd like to see some examples.

This comment has been minimized.

@JieGhost

JieGhost Mar 29, 2017

Contributor

Yeah, sure. I will add some unit tests.

@JieGhost

JieGhost Mar 29, 2017

Contributor

Yeah, sure. I will add some unit tests.

This comment has been minimized.

@JieGhost

JieGhost Mar 29, 2017

Contributor

added.

@JieGhost

JieGhost Mar 29, 2017

Contributor

added.

@stuhood

Thanks.

@JieGhost JieGhost merged commit c684511 into pantsbuild:master Mar 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JieGhost JieGhost deleted the JieGhost:yujieproject/jar_library_transitive branch Mar 29, 2017

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

Make jar_library target export all its dependencies (#4395)
### Problem

Some JarLibrary targets have dependencies on other targets (both 3rdparty or non 3rdparty).
Those dependencies are not directly used by dependees of jar_library, but has to be included in compile time classpath.

### Solution

Added a new property "exports" which is same as "dependencies" to JarLibrary. The "exports" property will be picked up by "exports resolve" logic.

Also enhance the "exports resolve" logic to handle exported target being a "target" alias. In that case, we just use strict_dependencies of the target alias.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment