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

Thrifty support for pants #5531

Merged
merged 5 commits into from Mar 6, 2018

Conversation

Projects
None yet
2 participants
@traviscrawford
Copy link
Member

traviscrawford commented Feb 28, 2018

Similar to how Wire generates Android-optimized Java code from protobufs, Microsoft Thrifty generates Android-optimized Java code from thrift IDL files.

Here we add a new java_thrifty_library target type to integrate Thrifty with Pants.

@stuhood
Copy link
Member

stuhood left a comment

Thanks Travis! Looks good.

@@ -0,0 +1 @@
python_library(sources=globs('*.py'))

This comment has been minimized.

from pants.contrib.thrifty.java_thrifty_library import JavaThriftyLibrary


logger = logging.getLogger(__name__)

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

A task should use self.context.log rather than a static logger.

'thrifty-compiler',
classpath=[thrifty_jar(name='thrifty-compiler')])

def __init__(self, *args, **kwargs):

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

No need to declare an empty __init__: can remove.

return deps

def synthetic_target_extra_exports(self, target, target_workdir):
return self.synthetic_target_extra_dependencies(target, target_workdir)

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

indent is odd.

But also, generally it's not a good idea to export everything that you depend on. Not a huge deal, but would recommend starting conservatively and exporting only the "required/default" dependency (the thrifty-runtime). Not a blocker, but relatively hard to fix later, so good to figure out early.

This comment has been minimized.

@traviscrawford

traviscrawford Mar 1, 2018

Member

This is probably a gap in my understanding about what should be exported. In the case where java_thrifty_library targets depend on other java_thrifty_library targets, do we need to export that IDL target chain, or just the thrifty-runtime dep?

This comment has been minimized.

@stuhood

stuhood Mar 1, 2018

Member

It depends what the generated code looks like, but the answer is "sometimes".

If you look at java_thrift_library, it takes an exports argument that allows users to say "I depend on 10 things, but only export 4 of them", similar to what you would do for non-generated code.

This is only relevant when you have strict_deps enabled for your targets, but if you don't care either way, I'd recommend going conservative and not exporting much: a dependent can always just add an explicit dependency, and it's much easier to loosen the constraints later than it is to tighten them.

def synthetic_target_extra_exports(self, target, target_workdir):
return self.synthetic_target_extra_dependencies(target, target_workdir)

def format_args_for_target(self, target, target_workdir):

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

Recommend starting private methods with _ ie.: _format_args_for_target

def _compute_include_paths(self, target):
"""Computes the set of paths that thrifty uses to lookup imports.
The IDL files under these paths are not compiled, but they are required to to compile

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

require to to

paths.add(os.path.join(get_buildroot(), self.context.source_roots.find(target).path))

def collect_paths(dep):
if not dep.has_sources():

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

has_sources takes an extension argument, so you can filter to only targets with thrift sources.

:return: an ordered set of directories to pass along to thrifty.
"""
paths = OrderedSet()
paths.add(os.path.join(get_buildroot(), self.context.source_roots.find(target).path))

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

Rather than self.context.source_roots.find(target), you can call target.target_base.

return
for source in dep.sources_relative_to_buildroot():
if source.endswith('.thrift'):
root = self.context.source_roots.find_by_path(source)

This comment has been minimized.

@stuhood

stuhood Feb 28, 2018

Member

Ditto dep.target_base

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Feb 28, 2018

Also, see contrib/README.md for more information on how to get this plugin set up to actually publish as part of the release process.

@traviscrawford traviscrawford force-pushed the traviscrawford:travis/contrib-thrifty branch from ec1a702 to 0ee3596 Mar 1, 2018

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 1, 2018

Thanks for the thoughtful review! 🙇 I made the suggested changes, and ran ./build-support/bin/release.sh -n to verify the release integration is setup.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 1, 2018

Pushed one more commit fixing the target subclass to fix publishing.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 1, 2018

CI failed due to an external connection timeout. Says Stack Overflow, I should close/open this PR to trigger a rebuild without pushing a commit.

Travis Crawford added some commits Feb 26, 2018

Travis Crawford
Thrifty support for pants
Similar to how Wire generates Android-optimized Java code from
protobufs, [Microsoft Thrifty](https://github.com/Microsoft/thrifty)
generates Android-optimized Java code from thrift IDL files. Here we add
a new `java_thrifty_library` target type to integrate Thrifty with
Pants.
Travis Crawford

@traviscrawford traviscrawford force-pushed the traviscrawford:travis/contrib-thrifty branch from bc0e6bd to 8ca2a0e Mar 1, 2018

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 1, 2018

Pushed a change that limits what deps are exported - it's now just thrifty-runtime. I verified that a published POM does indeed have all the deps needed, so consumers from maven will get the right stuff, along with folks in the repo itself.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 2, 2018

@stuhood I think this is ready to go, please LMK if you have any additional questions/comments.

Once this is merged to master, do you mind if I backport to the 1.4.x branch?

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 5, 2018

Bump :) Also, any objection to backporting into the 1.4.x branch?

@stuhood

stuhood approved these changes Mar 6, 2018

Copy link
Member

stuhood left a comment

Ack, sorry for the delay.

@stuhood stuhood merged commit 3f9c130 into pantsbuild:master Mar 6, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

stuhood commented Mar 6, 2018

I just landed the notes for 1.4.0rc3, but I hadn't yet cherry-picked them. Will make one more tweak.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 6, 2018

🙏

@stuhood stuhood added this to the 1.4.x milestone Mar 6, 2018

stuhood added a commit that referenced this pull request Mar 6, 2018

Thrifty support for pants (#5531)
Similar to how Wire generates Android-optimized Java code from protobufs, [Microsoft Thrifty](https://github.com/Microsoft/thrifty) generates Android-optimized Java code from thrift IDL files.

Here we add a new `java_thrifty_library` target type to integrate Thrifty with Pants.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment