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
Refactor the thrift codegen task. #4155
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,5 +25,5 @@ scandir==1.2 | |
setproctitle==1.1.10 | ||
setuptools==30.0.0 | ||
six>=1.9.0,<2 | ||
thrift==0.9.1 | ||
thrift>=0.9.1 | ||
wheel==0.29.0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,6 +104,9 @@ def _compile_target(self, target): | |
if isinstance(target, PythonBinary): | ||
source = 'entry_point {}'.format(target.entry_point) | ||
components = target.entry_point.rsplit(':', 1) | ||
if not all([x.strip() for x in components]): | ||
raise TaskError('Invalid entry point {} for target {}'.format( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error could be more specific probably... the issue here is an empty component? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the error is that you'll get "from blank import something" or "from something import blank" or even just "import blank". All of these will cause syntax errors when the pex is run, better to catch it now. I happened to notice this while debugging the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's clearly not a common or even likely error, or we'd have noticed it by now, so this is just a precaution, and the error message is probably fine. It would be laborious to explain in a comment, and I think whoever sees the actual broken entry_point string will immediately see why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! |
||
target.entry_point, target.address.spec)) | ||
module = components[0] | ||
if len(components) == 2: | ||
function = components[1] | ||
|
@@ -121,8 +124,9 @@ def _compile_target(self, target): | |
module_path, _ = os.path.splitext(path) | ||
source = 'file {}'.format(os.path.join(target.target_base, path)) | ||
module = module_path.replace(os.path.sep, '.') | ||
data = TemplateData(source=source, import_statement='import {}'.format(module)) | ||
modules.append(data) | ||
if module: | ||
data = TemplateData(source=source, import_statement='import {}'.format(module)) | ||
modules.append(data) | ||
|
||
if not modules: | ||
# Nothing to eval, so a trivial compile success. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
# coding=utf-8 | ||
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import (absolute_import, division, generators, nested_scopes, print_function, | ||
unicode_literals, with_statement) | ||
|
||
from pants.backend.codegen.thrift.java.java_thrift_library import JavaThriftLibrary | ||
from pants.backend.codegen.thrift.java.thrift_defaults import ThriftDefaults | ||
from pants.backend.codegen.thrift.lib.apache_thrift_gen_base import ApacheThriftGenBase | ||
from pants.backend.jvm.targets.java_library import JavaLibrary | ||
from pants.base.exceptions import TargetDefinitionException | ||
from pants.binaries.thrift_binary import ThriftBinary | ||
|
||
|
||
class ApacheThriftJavaGen(ApacheThriftGenBase): | ||
"""Generate Java source files from thrift IDL files.""" | ||
deprecated_options_scope = 'gen.thrift' # New scope is gen.thrift-java. | ||
deprecated_options_scope_removal_version = '1.5.0dev0' | ||
|
||
thrift_library_target_type = JavaThriftLibrary | ||
thrift_generator = 'java' | ||
|
||
_COMPILER = 'thrift' | ||
_RPC_STYLE = 'sync' | ||
|
||
@classmethod | ||
def subsystem_dependencies(cls): | ||
return (super(ApacheThriftJavaGen, cls).subsystem_dependencies() + | ||
(ThriftDefaults, ThriftBinary.Factory.scoped(cls))) | ||
|
||
@classmethod | ||
def implementation_version(cls): | ||
return super(ApacheThriftJavaGen, cls).implementation_version() + [('ApacheThriftJavaGen', 2)] | ||
|
||
def __init__(self, *args, **kwargs): | ||
super(ApacheThriftJavaGen, self).__init__(*args, **kwargs) | ||
self._thrift_defaults = ThriftDefaults.global_instance() | ||
|
||
def synthetic_target_type(self, target): | ||
return JavaLibrary | ||
|
||
def is_gentarget(self, target): | ||
return (super(ApacheThriftJavaGen, self).is_gentarget(target) and | ||
self._thrift_defaults.compiler(target) == self._COMPILER) | ||
|
||
def _validate(self, target): | ||
# TODO: Fix ThriftDefaults to only pertain to scrooge (see TODO there) and then | ||
# get rid of this spurious validation. | ||
if self._thrift_defaults.language(target) != self.thrift_generator: | ||
raise TargetDefinitionException( | ||
target, | ||
'Compiler {} supports only language={}.'.format(self._COMPILER, self.thrift_generator)) | ||
if self._thrift_defaults.rpc_style(target) != self._RPC_STYLE: | ||
raise TargetDefinitionException( | ||
target, | ||
'Compiler {} supports only rpc_style={}.'.format(self._COMPILER, self._RPC_STYLE)) | ||
|
||
def execute_codegen(self, target, target_workdir): | ||
self._validate(target) | ||
super(ApacheThriftJavaGen, self).execute_codegen(target, target_workdir) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
python_library( | ||
dependencies = [ | ||
'3rdparty/python/twitter/commons:twitter.common.collections', | ||
'src/python/pants/base:build_environment', | ||
'src/python/pants/base:exceptions', | ||
'src/python/pants/base:workunit', | ||
'src/python/pants/binaries:thrift_util', | ||
'src/python/pants/option', | ||
'src/python/pants/task', | ||
'src/python/pants/util:memo', | ||
], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the backwards-compatibility story of python thrift (changing the wire-format of floats, for example... cc @patricklaw ), an open-ended dep seems like a bad idea. Is there a reasonable bound for this? And if not, can you add a comment as to why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think <0.10 should be fine. I had to change this because it failed to resolve 0.9.1 (even though it is available on PyPi - so I'm guessing there was a version clash with some dep).
However I think the wire-format change happened between point releases, so all semver bets may be off.
I don't mind pinning this to, say, 0.9.3, or whatever it resolved to in practice, if you think that's preferable.