Support "exports" for thrift targets #4564

Merged
merged 12 commits into from May 10, 2017

Conversation

Projects
None yet
3 participants
@JieGhost
Contributor

JieGhost commented May 8, 2017

Problem

#4423

Currently, with "exports" feature, if exported targets of a dependency are thrift targets, then they are directly added to strict dependencies list of the dependee. However, only library targets will be added to classpaths during compilation, thus we need to do a conversion from thrift target to its corresponding synthetic target and then add to dependee's strict dependencies list.

Solution

If a target A has dependency on a thrift target B, then the synthetic target of B will be injected to A's dependency list as well. Thus we can iterate over A's dependency list to find the synthetic target for B.

When iterating over a target's exports list, if an exports target is a thrift target, the above logic will be executed to yield a synthetic target as exports target.

All the logic is in 'src/python/pants/backend/jvm/tasks/jvm_compile/compile_context.py'.

@JieGhost JieGhost changed the title from Copy "exports" from java_thrift_library target to synthetic target in scrooge_gen task to Copy "exports" from thrift target to synthetic target in scrooge_gen task May 8, 2017

@JieGhost JieGhost requested review from stuhood and baroquebobcat May 8, 2017

@@ -77,7 +77,7 @@ def excludes(self):
return self.payload.excludes
@property
- def exports(self):
+ def exports_targets(self):

This comment has been minimized.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

I don't think you can just remove this since it's public.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

I don't think you can just remove this since it's public.

This comment has been minimized.

@JieGhost

JieGhost May 8, 2017

Contributor

hmm, this wasn't added very long ago (about a month ago), I don't think anyone is using it yet, but I can also keep it and make it "return None" I guess?

@JieGhost

JieGhost May 8, 2017

Contributor

hmm, this wasn't added very long ago (about a month ago), I don't think anyone is using it yet, but I can also keep it and make it "return None" I guess?

This comment has been minimized.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

Hm. It might be ok. If we can be pretty sure no one is using it, and won't adopt it in 1.3.0 then we could do it. Actually, if we merged the update into 1.3.0 before the release, then it's moot because it was never part of a non-dev release.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

Hm. It might be ok. If we can be pretty sure no one is using it, and won't adopt it in 1.3.0 then we could do it. Actually, if we merged the update into 1.3.0 before the release, then it's moot because it was never part of a non-dev release.

This comment has been minimized.

@stuhood

stuhood May 8, 2017

Member

Yea, I think that is a reasonable stretching of the :API: public definition.

@stuhood

stuhood May 8, 2017

Member

Yea, I think that is a reasonable stretching of the :API: public definition.

+)
+
+java_thrift_library(
+ name='C1',

This comment has been minimized.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

It'd be nice if the names for these targets included something about their config. C-with-exporting-dependency or something would be clearer when they are referenced in tests.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

It'd be nice if the names for these targets included something about their config. C-with-exporting-dependency or something would be clearer when they are referenced in tests.

for spec in self.payload.exports:
addr = Address.parse(spec, relative_to=self.address.spec_path)
target = self._build_graph.get_target(addr)
+ if target.is_thrift:

This comment has been minimized.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

It feels like this ought to be more general. In theory, it could apply to any gen target that's exported. I'm not sure whether that makes sense to do in this review though.

I'd also like to see some unit tests for this method that exercise the new functionality.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

It feels like this ought to be more general. In theory, it could apply to any gen target that's exported. I'm not sure whether that makes sense to do in this review though.

I'd also like to see some unit tests for this method that exercise the new functionality.

@@ -84,7 +84,6 @@ def default_java_namespace(self):
def include_paths(self):
return self._include_paths
- # TODO(Eric Ayers) As of 2/5/2015 this call is DEPRECATED and should be removed soon
@property

This comment has been minimized.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

Perhaps instead of adding a new call for is_thrift, this could move to a different pattern. Maybe it could look for the class instead via isinstance. It looks like the deprecation plan for the associated is_* methods involved types via marker mixins.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

Perhaps instead of adding a new call for is_thrift, this could move to a different pattern. Maybe it could look for the class instead via isinstance. It looks like the deprecation plan for the associated is_* methods involved types via marker mixins.

This comment has been minimized.

@JieGhost

JieGhost May 8, 2017

Contributor

what is marker mixins? What do you mean by "look for the class"? I tried to use isintance in JvmTarget, but since it requires import of JavaThriftTarget, it leads to circular import I think.

@JieGhost

JieGhost May 8, 2017

Contributor

what is marker mixins? What do you mean by "look for the class"? I tried to use isintance in JvmTarget, but since it requires import of JavaThriftTarget, it leads to circular import I think.

This comment has been minimized.

@baroquebobcat

baroquebobcat May 8, 2017

Contributor

I see what you mean. The usecase in jvm_target can't work unless the cycle were to be broken.

Marker mixins are classes that have no behavior, but mark a class as being a particular type. It's a python multiple inheritance thing. eg

class ThriftMixin(object):
  pass

class Foo(ThriftMixin, Bar):
  pass
@baroquebobcat

baroquebobcat May 8, 2017

Contributor

I see what you mean. The usecase in jvm_target can't work unless the cycle were to be broken.

Marker mixins are classes that have no behavior, but mark a class as being a particular type. It's a python multiple inheritance thing. eg

class ThriftMixin(object):
  pass

class Foo(ThriftMixin, Bar):
  pass
@@ -278,3 +288,7 @@ def excludes(self):
@property
def services(self):
return self._services
+
+ @property
+ def is_thrift(self):

This comment has been minimized.

@stuhood

stuhood May 8, 2017

Member

I really don't like the is_thrift solution... it was deprecated for a reason, and should stay that way. And moreover, JvmTarget should not have awareness of other target types.

@stuhood

stuhood May 8, 2017

Member

I really don't like the is_thrift solution... it was deprecated for a reason, and should stay that way. And moreover, JvmTarget should not have awareness of other target types.

for spec in self.payload.exports:
addr = Address.parse(spec, relative_to=self.address.spec_path)
target = self._build_graph.get_target(addr)
+ if target.is_thrift:
+ for dep in self.dependencies:
+ if dep != target and dep.is_synthetic and dep.derived_from == target:

This comment has been minimized.

@stuhood

stuhood May 8, 2017

Member

It's hard to tell what is going on here, because there isn't a comment. But it seems like this might reduce to checking Target.concrete_derived_from?

More generally, it seems like this code should move off of Target, and into the task. The direction that we're going to try and head in over time is that awareness of Task-specific dependencies (like exports), is a Task-level concern. So jvm_compile needs to know what exports and strict_deps mean, but no other Tasks need to.

@stuhood

stuhood May 8, 2017

Member

It's hard to tell what is going on here, because there isn't a comment. But it seems like this might reduce to checking Target.concrete_derived_from?

More generally, it seems like this code should move off of Target, and into the task. The direction that we're going to try and head in over time is that awareness of Task-specific dependencies (like exports), is a Task-level concern. So jvm_compile needs to know what exports and strict_deps mean, but no other Tasks need to.

This comment has been minimized.

@JieGhost

JieGhost May 8, 2017

Contributor

I don't think "concrete_derived_from" works better than "derived_from" here. The issue is that we can only walk "JavaLibrary/ScalaLibrary" -> "JavaThriftLibrary" path, but not the other way around. What the above code does is to walk "JavaThriftLibrary" -> "JavaLibrary/ScalaLibrary" path. I should have left a comment here, sorry!

I think your suggestion about moving the logic into task makes sense, and I will make the change.

@JieGhost

JieGhost May 8, 2017

Contributor

I don't think "concrete_derived_from" works better than "derived_from" here. The issue is that we can only walk "JavaLibrary/ScalaLibrary" -> "JavaThriftLibrary" path, but not the other way around. What the above code does is to walk "JavaThriftLibrary" -> "JavaLibrary/ScalaLibrary" path. I should have left a comment here, sorry!

I think your suggestion about moving the logic into task makes sense, and I will make the change.

@stuhood stuhood added this to the 1.3.0 milestone May 9, 2017

@JieGhost JieGhost changed the title from Copy "exports" from thrift target to synthetic target in scrooge_gen task to Handle the case where exported target is a thrift target May 9, 2017

@JieGhost JieGhost changed the title from Handle the case where exported target is a thrift target to Support "exports" for thrift targets May 9, 2017

@stuhood

Thanks Yujie... this looks much better.

+ if not isinstance(export, Target):
+ addr = Address.parse(export, relative_to=target.address.spec_path)
+ export = target._build_graph.get_target(addr)
+ if export is None:

This comment has been minimized.

@stuhood

stuhood May 10, 2017

Member

I think export is None and export not in target.dependencies should both be treated as the same error: if it isn't in the graph, it definitely wasn't in your dependencies list.

@stuhood

stuhood May 10, 2017

Member

I think export is None and export not in target.dependencies should both be treated as the same error: if it isn't in the graph, it definitely wasn't in your dependencies list.

@stuhood

stuhood approved these changes May 10, 2017 edited

Thanks Yujie. Feel free to merge whenever you're satisfied.

@JieGhost JieGhost merged commit c9ea2fc into pantsbuild:master May 10, 2017

1 check passed

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

@JieGhost JieGhost deleted the JieGhost:fix_strict_deps branch May 10, 2017

stuhood added a commit that referenced this pull request May 12, 2017

Support "exports" for thrift targets (#4564)
### Problem

#4423 

Currently, with "exports" feature, if exported targets of a dependency are thrift targets, then they are directly added to strict dependencies list of the dependee. However, only library targets will be added to classpaths during compilation, thus we need to do a conversion from thrift target to its corresponding synthetic target and then add to dependee's strict dependencies list.

### Solution

If a target A has dependency on a thrift target B, then the synthetic target of B will be injected to A's dependency list as well. Thus we can iterate over A's dependency list to find the synthetic target for B.

When iterating over a target's exports list, if an exports target is a thrift target, the above logic will be executed to yield a synthetic target as exports target.

All the logic is in 'src/python/pants/backend/jvm/tasks/jvm_compile/compile_context.py'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment