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

Merged
merged 2 commits into from Jan 5, 2017

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Jan 2, 2017

Almost all the logic is now in a base class that thrift gen
for other languages can share.

To prove that this works, created a python thrift gen task
in only a handful of lines of code.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jan 3, 2017

Member

A few travis issues on this one. Will wait to review.

Member

stuhood commented Jan 3, 2017

A few travis issues on this one. Will wait to review.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Jan 3, 2017

Contributor

Yeah, it's not ready to go yet.

Contributor

benjyw commented Jan 3, 2017

Yeah, it's not ready to go yet.

Refactor the thrift codegen task.
Almost all the logic is now in a base class that thrift gefor other languages can share.

To prove that this works, created a python thrift gen task
in only a handful of lines of code.

@benjyw benjyw requested review from stuhood and wisechengyi Jan 3, 2017

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Jan 3, 2017

Contributor

OK, this is ready to be reviewed now. @wisechengyi - can you check that the change to the export test is OK? I'm assuming that the plugin (and anything else that consumes the export) doesn't expect the literal name gen/thrift/ and will be just as fine with gen/thrift-java/ ?

Contributor

benjyw commented Jan 3, 2017

OK, this is ready to be reviewed now. @wisechengyi - can you check that the change to the export test is OK? I'm assuming that the plugin (and anything else that consumes the export) doesn't expect the literal name gen/thrift/ and will be just as fine with gen/thrift-java/ ?

@stuhood

Thanks Benjy! This looks like a very nice improvement... looking forward to removal of the thrift builder code.

Just a few issues.

@@ -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

This comment has been minimized.

@stuhood

stuhood Jan 3, 2017

Member

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?

@stuhood

stuhood Jan 3, 2017

Member

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?

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

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.

@benjyw

benjyw Jan 4, 2017

Contributor

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.

@@ -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(

This comment has been minimized.

@stuhood

stuhood Jan 3, 2017

Member

This error could be more specific probably... the issue here is an empty component?

@stuhood

stuhood Jan 3, 2017

Member

This error could be more specific probably... the issue here is an empty component?

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

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 __init__.py issue (because the root-level __init__.py tickles this.

@benjyw

benjyw Jan 4, 2017

Contributor

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 __init__.py issue (because the root-level __init__.py tickles this.

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

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.

@benjyw

benjyw Jan 4, 2017

Contributor

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.

This comment has been minimized.

@stuhood

stuhood Jan 4, 2017

Member

Sounds good!

@stuhood

stuhood Jan 4, 2017

Member

Sounds good!

Show outdated Hide outdated src/python/pants/backend/codegen/thrift/java/apache_thrift_java_gen.py
@classmethod
def implementation_version(cls):
return super(ApacheThriftJavaGen, cls).implementation_version() + [('ApacheThriftGen', 2)]

This comment has been minimized.

@stuhood

stuhood Jan 3, 2017

Member

The redundancy of the the literal string here is annoying... but in this case I think this should be updated to ApacheThriftJavaGen

@stuhood

stuhood Jan 3, 2017

Member

The redundancy of the the literal string here is annoying... but in this case I think this should be updated to ApacheThriftJavaGen

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

Well, this is just a string that gets mixed into the fingerprint, right? It can be anything. If I change it, it'll invalidate the world. Well, the world of thrift, anyway. I guess that's not a big deal. Changed.

@benjyw

benjyw Jan 4, 2017

Contributor

Well, this is just a string that gets mixed into the fingerprint, right? It can be anything. If I change it, it'll invalidate the world. Well, the world of thrift, anyway. I guess that's not a big deal. Changed.

Show outdated Hide outdated src/python/pants/backend/codegen/thrift/lib/apache_thrift_gen_base.py
help='A list of specs pointing to dependencies of thrift generated java code.')
register('--service-deps', advanced=True, type=list,
help='A list of specs pointing to dependencies of thrift generated java service '
removal_version='1.5.0', removal_hint='Use --gen-options-map instead',

This comment has been minimized.

@stuhood

stuhood Jan 3, 2017

Member

For this deprecation and others, please see https://groups.google.com/d/msg/pants-committers/OrLkQOsUSDo/KYlpcrIaBAAJ ... sorry that I haven't published that anywhere yet.

In this case, I think that means targeting 1.5.0.dev0.

@stuhood

stuhood Jan 3, 2017

Member

For this deprecation and others, please see https://groups.google.com/d/msg/pants-committers/OrLkQOsUSDo/KYlpcrIaBAAJ ... sorry that I haven't published that anywhere yet.

In this case, I think that means targeting 1.5.0.dev0.

This comment has been minimized.

@stuhood

stuhood Jan 3, 2017

Member

...and if you have time to add a ban of non-devX-suffixed versions to the deprecation package, that would be amazing!

@stuhood

stuhood Jan 3, 2017

Member

...and if you have time to add a ban of non-devX-suffixed versions to the deprecation package, that would be amazing!

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

Done. Made an issue for the deprecation package fix: #4162

@benjyw

benjyw Jan 4, 2017

Contributor

Done. Made an issue for the deprecation package fix: #4162

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

BTW you mean 1.5.0dev0 not 1.5.0.dev0, ya?

@benjyw

benjyw Jan 4, 2017

Contributor

BTW you mean 1.5.0dev0 not 1.5.0.dev0, ya?

from pants.backend.python.targets.python_library import PythonLibrary
class ApacheThriftPyGen(ApacheThriftGenBase):

This comment has been minimized.

@stuhood

stuhood Jan 3, 2017

Member

Does this task (or a superclass...?) need a pydoc? IIRC, that's how ./pants help is generated.

@stuhood

stuhood Jan 3, 2017

Member

Does this task (or a superclass...?) need a pydoc? IIRC, that's how ./pants help is generated.

This comment has been minimized.

@benjyw

benjyw Jan 4, 2017

Contributor

True, although that's honored more in the breach than in the observance, if my spot-checking of other tasks is anything to go by... Will add something here, but we'll need to do a sweep through all the tasks.

@benjyw

benjyw Jan 4, 2017

Contributor

True, although that's honored more in the breach than in the observance, if my spot-checking of other tasks is anything to go by... Will add something here, but we'll need to do a sweep through all the tasks.

@wisechengyi

This comment has been minimized.

Show comment
Hide comment
Contributor

wisechengyi commented Jan 4, 2017

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Jan 4, 2017

Contributor

Pushed a commit that addresses the requested changes. PTAL.

Contributor

benjyw commented Jan 4, 2017

Pushed a commit that addresses the requested changes. PTAL.

@stuhood

stuhood approved these changes Jan 4, 2017

Thanks Benjy!

@benjyw benjyw merged commit e01cee8 into pantsbuild:master Jan 5, 2017

1 check passed

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

@benjyw benjyw deleted the benjyw:python_thrift branch Jan 5, 2017

JieGhost added a commit that referenced this pull request Jan 12, 2017

add various codegen packages to default backend packages (#4175)
### Problem

#4147 #4155 #4151 refactored codegen backend, and split it into multiple smaller packages.

We used to have codegen as one of the default backend packages, now we want to add these new packages to the default list instead.

### Solution

When registering "--backend-packages" option, add these new packages to the default list and remove codegen package from the list.

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

Refactor the thrift codegen task. (#4155)
Almost all the logic is now in a base class that thrift gen 
for other languages can share.

To prove that this works, created a python thrift gen task
in only a handful of lines of code.

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

add various codegen packages to default backend packages (#4175)
### Problem

#4147 #4155 #4151 refactored codegen backend, and split it into multiple smaller packages.

We used to have codegen as one of the default backend packages, now we want to add these new packages to the default list instead.

### Solution

When registering "--backend-packages" option, add these new packages to the default list and remove codegen package from the list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment