A flag to add the buildroot to protoc's import path. #4122

Merged
merged 3 commits into from Dec 7, 2016

Conversation

Projects
None yet
3 participants
@benjyw
Contributor

benjyw commented Dec 6, 2016

A flag to add the buildroot to protoc's import path.

This allows use of Pants in repos adhering to the protobuf
documentation's recommendation of importing relative to the buildroot,
rather than relative to the .proto file's source root (e.g., src/proto).

A flag to add the buildroot to protoc's import path.
This allows use of Pants in repos adhering to the protobuf
documentation's recommendation of importing from the buildroot,
rather than from the proto source root (e.g., src/proto).
@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Dec 6, 2016

Contributor

Note that this is separate from the source roots change. That change was occasioned by my noticing, in the course of poking at this proto import problem, that we don't support the buildroot being a source root. However that is merely an aesthetic/completeness fix. It doesn't actually solve this problem: the buildroot is likely not the "source root" (whatever that means for .proto files), but we still need to be able to import relative to it. Hence this change.

Contributor

benjyw commented Dec 6, 2016

Note that this is separate from the source roots change. That change was occasioned by my noticing, in the course of poking at this proto import problem, that we don't support the buildroot being a source root. However that is merely an aesthetic/completeness fix. It doesn't actually solve this problem: the buildroot is likely not the "source root" (whatever that means for .proto files), but we still need to be able to import relative to it. Hence this change.

+ ['gen.protoc', '--import-from-root',
+ 'testprojects/src/protobuf/org/pantsbuild/testproject/import_from_buildroot/bar'])
+ self.assert_success(pants_run)
+

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 6, 2016

Contributor

Could you add a test that asserts that if there's a import for a proto file that is not owned by one of a target's dependencies that it fails? I didn't see one of those when I looked.

Maybe we can't ensure that right now, because of how source_roots work with targets.

@baroquebobcat

baroquebobcat Dec 6, 2016

Contributor

Could you add a test that asserts that if there's a import for a proto file that is not owned by one of a target's dependencies that it fails? I didn't see one of those when I looked.

Maybe we can't ensure that right now, because of how source_roots work with targets.

This comment has been minimized.

@benjyw

benjyw Dec 6, 2016

Contributor

It wouldn't fail in that case, as far as I can tell. And even before, you were free to import any proto file in any source root that was in your other dependencies' bases (or your own), even if you didn't depend on it.

This is true in all codegen, I think, and in many other tasks. But I'd rather we put energy into moving in the other direction - namely implying dependencies when we can instead of requiring them to be explicit... :)

@benjyw

benjyw Dec 6, 2016

Contributor

It wouldn't fail in that case, as far as I can tell. And even before, you were free to import any proto file in any source root that was in your other dependencies' bases (or your own), even if you didn't depend on it.

This is true in all codegen, I think, and in many other tasks. But I'd rather we put energy into moving in the other direction - namely implying dependencies when we can instead of requiring them to be explicit... :)

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 6, 2016

Contributor

I see what you mean. We've run into the source root import 'feature' a few times too. Another approach to resolving it is working towards isolation based on the build graph. That also won't require specific per task error handling, which is good.

I just get a bit nervous when I see a test suite that doesn't have any tests for failure / error cases.

@baroquebobcat

baroquebobcat Dec 6, 2016

Contributor

I see what you mean. We've run into the source root import 'feature' a few times too. Another approach to resolving it is working towards isolation based on the build graph. That also won't require specific per task error handling, which is good.

I just get a bit nervous when I see a test suite that doesn't have any tests for failure / error cases.

This comment has been minimized.

@benjyw

benjyw Dec 6, 2016

Contributor

Agreed, the isolation should come from the engine. Does the new engine admit anything like it easily?

@benjyw

benjyw Dec 6, 2016

Contributor

Agreed, the isolation should come from the engine. Does the new engine admit anything like it easily?

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

I think it could. The snapshot execution work was a proof of concept for this.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

I think it could. The snapshot execution work was a proof of concept for this.

@benjyw benjyw changed the title from [wip] A flag to add the buildroot to protoc's import path. to A flag to add the buildroot to protoc's import path. Dec 6, 2016

@baroquebobcat

Looks good to me.

+ register('--import-from-root', type=bool, advanced=True,
+ help='If set, add the buildroot to the path protoc searches for imports. '
+ 'This facilitates import paths in .proto files that are relative to the '
+ 'build root, as recommended by the protoc documentation.')

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

The second sentence feels a little awkward, maybe something like

This enables using import paths relative to the build root in .proto files, as recommended by the protoc documentation.

would be better

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

The second sentence feels a little awkward, maybe something like

This enables using import paths relative to the build root in .proto files, as recommended by the protoc documentation.

would be better

This comment has been minimized.

@benjyw

benjyw Dec 7, 2016

Contributor

That does scan better. Changed.

@benjyw

benjyw Dec 7, 2016

Contributor

That does scan better. Changed.

@benjyw benjyw merged commit 06918be into pantsbuild:master Dec 7, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@benjyw benjyw deleted the benjyw:import_proto_from_root branch Dec 7, 2016

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Dec 7, 2016

Contributor

Thanks for the review!

Contributor

benjyw commented Dec 7, 2016

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment