add various codegen packages to default backend packages #4175

Merged
merged 5 commits into from Jan 12, 2017

Conversation

Projects
None yet
4 participants
@JieGhost
Contributor

JieGhost commented Jan 10, 2017

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 as well.

Solution

When registering "--backend-packages" option, add these new packages to the default list.

@JieGhost JieGhost requested review from stuhood and benjyw Jan 10, 2017

@@ -65,6 +65,13 @@ def register_bootstrap_options(cls, register):
'pants.backend.python',
'pants.backend.jvm',
'pants.backend.codegen',

This comment has been minimized.

@benjyw

benjyw Jan 10, 2017

Contributor

I think we don't need this backend now? In fact I'm surprised we don't get errors for re-registering on the same task names?

@benjyw

benjyw Jan 10, 2017

Contributor

I think we don't need this backend now? In fact I'm surprised we don't get errors for re-registering on the same task names?

This comment has been minimized.

@JieGhost

JieGhost Jan 10, 2017

Contributor

This comment has been minimized.

@JieGhost

JieGhost Jan 10, 2017

Contributor

I mean, for code that still does
"from pants.backend.codegen.targets.java_thrift_library import JavaThriftLibrary",
it is an error now?

@JieGhost

JieGhost Jan 10, 2017

Contributor

I mean, for code that still does
"from pants.backend.codegen.targets.java_thrift_library import JavaThriftLibrary",
it is an error now?

This comment has been minimized.

@JieGhost

JieGhost Jan 10, 2017

Contributor

Also, it seems "re-registering" won't cause errors, probably later registering can override previous ones?

@JieGhost

JieGhost Jan 10, 2017

Contributor

Also, it seems "re-registering" won't cause errors, probably later registering can override previous ones?

This comment has been minimized.

@benjyw

benjyw Jan 11, 2017

Contributor

No, it won't make it an error, it'll become an error on that version.

Pants itself is already not using those stubs, those deprecation warnings are for any custom pants tasks that might be using them.

@benjyw

benjyw Jan 11, 2017

Contributor

No, it won't make it an error, it'll become an error on that version.

Pants itself is already not using those stubs, those deprecation warnings are for any custom pants tasks that might be using them.

This comment has been minimized.

@JieGhost

JieGhost Jan 11, 2017

Contributor

At first I was concerned about "module not found" kind of error, as "pants.backend.codegen" is not loaded in this case.

But I have tried and it seems no such error is generated. I think it is already in the pythonpath and load_backend just registers goals/tasks/options.

@JieGhost

JieGhost Jan 11, 2017

Contributor

At first I was concerned about "module not found" kind of error, as "pants.backend.codegen" is not loaded in this case.

But I have tried and it seems no such error is generated. I think it is already in the pythonpath and load_backend just registers goals/tasks/options.

@benjyw

I think we don't need to register the original codegen backend now? In fact I'm surprised we don't get errors for re-registering on the same task names?

@stuhood

It's unclear why this wasn't caught in tests... the backend was loaded implicitly for some reason...?

@JieGhost

This comment has been minimized.

Show comment
Hide comment
@JieGhost

JieGhost Jan 11, 2017

Contributor

@stuhood
My guess is that, for unit tests, backend packages are irrelevant and for Integration tests, since Benjy's change is backward compatible, just loading "pants.backend.codegen" is enough to get tests pass.

Contributor

JieGhost commented Jan 11, 2017

@stuhood
My guess is that, for unit tests, backend packages are irrelevant and for Integration tests, since Benjy's change is backward compatible, just loading "pants.backend.codegen" is enough to get tests pass.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Jan 12, 2017

Contributor

@stuhood why what wasn't caught? I don't think this fixes an error, it just calls the registration via different paths.

Contributor

benjyw commented Jan 12, 2017

@stuhood why what wasn't caught? I don't think this fixes an error, it just calls the registration via different paths.

@benjyw

benjyw approved these changes Jan 12, 2017

@JieGhost JieGhost merged commit 5ed4c29 into pantsbuild:master Jan 12, 2017

1 check passed

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

@JieGhost JieGhost deleted the JieGhost:yujieproject/add_codegen_thrift_backend_package branch Jan 12, 2017

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jan 12, 2017

Member

@benjyw : These backends weren't loaded in pantsbuild via backend_packages in pants.ini, so it's not clear to me why they were being loaded at all.

Member

stuhood commented Jan 12, 2017

@benjyw : These backends weren't loaded in pantsbuild via backend_packages in pants.ini, so it's not clear to me why they were being loaded at all.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Jan 12, 2017

Contributor

The old codegen backend continued to register all the same target types and tasks as before, they were just split up into new backends so it's now also possible to register them via the new backends, which is what this change did.

Contributor

benjyw commented Jan 12, 2017

The old codegen backend continued to register all the same target types and tasks as before, they were just split up into new backends so it's now also possible to register them via the new backends, which is what this change did.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood Jan 13, 2017

Member

Ahh.

Member

stuhood commented Jan 13, 2017

Ahh.

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