Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Improve proto rules #292

Merged

Conversation

coreywoodfield
Copy link
Contributor

I made the same changes as #217, #211, and #289, without realizing that it had already been done (and pulled the test from #217, thanks @bjchambers). Since merge checks are failing for two of those and none of them seem very active, I figured I'd give it a shot as well. Resolves #218. Tests pass locally.

Overview of changes:

  • Stop passing dependencies as sources to protoc. Previously, generating scala code for one.proto (in tests/proto) would also generate code for zero.proto, since one depends on zero. Since we're already also generating scala code for zero.proto by itself, the scala code for zero.proto gets generated twice, and each generated scala jar contains the scala code for all of its proto dependencies. With this change, generating scala code for one.proto doesn't generate code for zero.proto. Rather, the generated scala library for one has a dependency on the generated scala library for zero.
  • Pass proto_path arguments to protoc. Since we're no longer passing dependencies as sources to protoc, we pass the root directories of all dependencies to protoc so that it can find the proto files we depend on itself. This also allows third-party protos to be used (as mentioned in Add transitive path arguments when compiling protobuf #289) since protoc knows about all the directories where proto files are actually found, rather than just the directories where the protos being compiled presently are found.

coreywoodfield and others added 3 commits January 28, 2021 16:11
Also pass in transitive-paths so protoc can find the transitive sources
The test checks that the compilation of `one.proto` generates code only
for protos in one.proto, and not for protos in zero.proto.
Copy link
Collaborator

@SrodriguezO SrodriguezO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @coreywoodfield. CI looked good save for format; pushed up fix. Will merge once CI finishes. Travis is borked.. so here's the link.

Also thanks (and apologies for this falling from our radar) @bjchambers, @kerinin, and @tjarvstrand

@borkaehw borkaehw merged commit e9f7843 into higherkindness:master Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scala_proto_library contains code for all dependencies
4 participants