Skip to content
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

Add PB.importRoot setting and implementation #346

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jessekempf-vsco
Copy link

We've got a protobuf repo at work we need to pull in called protobufs, and the general structure of it looks like:

protobufs/
  - android/
  - ios/
  - protos/
    - shared
      - foo.proto
      - bar.proto
      - baz.proto
    - servicename1/
      - svc1_schema.proto
      - .
      - .
      - .
    - servicename2/
      - svc2_schema.proto
        - .
        - .
        - .

And the important thing to note, here, is that this has been arranged for the convenience of Bazel. Our microservices are all written in Go with Bazel as a build tool, while our data engineering department uses Scala. We've been using Bazel and Scala so far, but I'm working on migrating away from Bazel to SBT for JVM builds.

protos/servicename2/svc2_schema.proto will import things this way:

import protos/shared/foo.proto
import protos/shared/bar.proto
import protos/servicename1/svc1_schema.proto

and so forth. The already-existing sbt-protoc approach produced only protobuf errors about enum values already being defined, so I gave myself a way to directly control where to look for imports.

@jessekempf-vsco
Copy link
Author

jessekempf-vsco commented Sep 11, 2023

@thesamet: Hello! What do I need to do to get this reviewed and merged?

@thesamet
Copy link
Owner

Trying to understand why the new setting is necessary - wouldn't it be possible to override PB.includePaths within your project?

@jessekempf-vsco
Copy link
Author

@thesamet: Yes, it's possible for me to replace PB.includePaths, but the problem is that because I'm replacing it, I need to duplicate the contents of sbt-protoc lines 314-319 into my config. This is generally a pretty big warning sign that I'm doing something wrong. It'll also be harder for whoever comes after me to maintain.

On the other hand, adding it as a new setting sidesteps those problems.

@thesamet
Copy link
Owner

Sorry for the delay. Can you add to the PR a test under sbt-test/settings that demonstrates the usage of the new setting? I'd also like to validate the concern about duplicating code from sbt-protoc. If indeed duplication (or non-trivial sbt trickery) is necessary to get it to work, then I'll accept the PR which introduces this new setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants