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

Please add proto_library attrs to adjust its "virtual path" #3867

Closed
jmillikin-stripe opened this issue Oct 5, 2017 · 24 comments
Closed

Please add proto_library attrs to adjust its "virtual path" #3867

jmillikin-stripe opened this issue Oct 5, 2017 · 24 comments

Comments

@jmillikin-stripe
Copy link
Contributor

@jmillikin-stripe jmillikin-stripe commented Oct 5, 2017

When a .proto file contains import "foo/bar/baz.proto", that path doesn't actually have to exist as-is on the filesystem. It can be mapped to any arbitrary path by protoc by using an equals sign in --proto_path: protoc --proto_path=foo/bar/baz.proto=some/other/path.proto.

Please add attributes to proto_library() to adjust the virtual path used to locate its source proto, similar to how cc_library() allows changing the include path. Good attributes would be:

  • import_prefix to add a prefix to the virtual path.
  • strip_import_prefix to remove a prefix from the virtual path.

The main use case is cross-repo includes, where we want to use nice short names in the filesystem but URL-prefixed names in the import paths:

# ./external/base/proto/BUILD
proto_library(
  name = "build_info",
  srcs = ["build_info.proto"],
  import_prefix = "github.com/myorg/base",
)

# ./proto/BUILD
proto_library(
  name = "helloworld",
  srcs = ["helloworld.proto"],
  deps = ["@base//proto:build_info],
)

# CURRENT ./proto/helloworld.proto
import "proto/build_info.proto";

# DESIRED ./proto/helloworld.proto
import "github.com/myorg/base/proto/build_info.proto";
@nicolov
Copy link
Contributor

@nicolov nicolov commented Oct 6, 2017

I second this request: I've been using "@com_google_protobuf//:protobuf.bzl"as a workaround, since it allows setting a custom import path.

@aehlig
Copy link
Contributor

@aehlig aehlig commented Oct 6, 2017

@cgrushko for further triaging.

@cgrushko
Copy link
Contributor

@cgrushko cgrushko commented Oct 6, 2017

I passed the proto_library torch to @lberki , but -

  1. can you edit the OP and give an example of how it would look like without the new attribute?
  2. would automatic prefix based on the external repo name make sense?
@cgrushko cgrushko assigned lberki and unassigned cgrushko Oct 6, 2017
@jmillikin-stripe
Copy link
Contributor Author

@jmillikin-stripe jmillikin-stripe commented Oct 6, 2017

(1) Done. Without the proposed new attribute, all proto imports use the "relative" path of the .proto file within the original repository -- this has a high chance of name conflicts.

(2) My gut instinct is "no", because external dependency names are not required to be globally stable or consistent. It also ties the .proto file content very tightly with Bazel, which is undesirable for projects that need to support multiple build systems.

@kennknowles
Copy link

@kennknowles kennknowles commented Oct 26, 2017

I think this might also be relevant for multi-module maven projects that are being converted in place.

module-a/
    BUILD  # yay!
    pom.xml  # legacy, uses org.xolstice.maven.plugins:protobuf-maven-plugin
    src/main/proto/a.proto

module-b/
    BUILD  # yay!
    pom.xml  # legacy, depends on my-org:module-a
    src/main/proto/b.proto  # contains "import a.proto"

I know the above works because of details of how the protos are stuffed into jars and whatnot, but it is all default stuff.

I would love an easy solution to non-invasively building this tree with bazel just by dropping in BUILD files. This ticket looks about right, but I would be glad to be corrected. Here is what I imagine it would look like:

In module-a/BUILD:

proto_library(
  name = "a_proto",
  srcs = ["src/main/proto/a.proto"],
  strip_import_prefix = "src/main/proto",
)

In module-b/BUILD:

proto_library(
  name = "whatsit_proto",
  srcs = ["src/main/proto/b.proto"],
  strip_import_prefix = "src/main/proto",
  deps = ["//module-a:a_proto"],
)

What do you think? Have I missed something really easy here?

@tomwilkie
Copy link

@tomwilkie tomwilkie commented Nov 21, 2017

Hi there; just to chime in - this issue is preventing us vendoring proto libraries which depend on other proto libraries in go projects - see bazelbuild/rules_go#900. A good example is the go Kubernetes Client library. Is there any chance someone might work on this any time soon, or should I take a stab?

@jmhodges
Copy link
Contributor

@jmhodges jmhodges commented Mar 22, 2018

I just got hit by this in bazelbuild/bazel-gazelle#162 and have worked around it by forking k8s.io/api and k8s.io/apimachinery to my own account and deleting all of the proto files within them!

Trying to add client-go to my repo has been a source of a lot of pain over the last week while I tried to get other bazel-related blockers fixed for this one goal of getting client-go to build. I keep having to delete files from forks in order to move forward. (e.g. my workaround for kubernetes/publishing-bot#49 before it was done in the repo's that publishing-bot controls)

@jayconrod
Copy link
Collaborator

@jayconrod jayconrod commented May 4, 2018

Any chance this could be implemented soon?

This blocks Kubernetes and a number of related projects from using proto_library. They are having to pre-generate code with a separate system and check it in.

@RNabel
Copy link
Contributor

@RNabel RNabel commented May 22, 2018

Is there a timeline for this? This feature would save us a lot of pain as well.

@jayconrod
Copy link
Collaborator

@jayconrod jayconrod commented May 25, 2018

Bump, please fix soon.

Lots of people are running into this in the Go community. Currently, I'm advising people to check in generated .pb.go files and avoid using proto_library. The lack of flexibility in this rule erodes a major advantage Bazel has over other systems: integrated code generation.

@jin
Copy link
Member

@jin jin commented May 25, 2018

@lberki are you the current owner of proto rules?

cc @katre (current Bazel sheriff)

@tautomaton
Copy link

@tautomaton tautomaton commented Jun 21, 2018

Friendly ping! This issue seems to be affecting a number of projects.

@katre
Copy link
Member

@katre katre commented Jun 22, 2018

Pinging @lberki since hopefully he knows about proto rules.

@jmillikin-stripe
Copy link
Contributor Author

@jmillikin-stripe jmillikin-stripe commented Jul 7, 2018

I have a rough attempt at this feature in #5536

@lberki
Copy link
Contributor

@lberki lberki commented Nov 23, 2018

Update: I'm almost done with tidying up the implementation of proto_library for this to be feasible. Stay tuned, the functionality may even make it into 0.21!

@lberki
Copy link
Contributor

@lberki lberki commented Nov 26, 2018

Update: please take a look at the rough draft I have here:

https://github.com/lberki/bazel/
lberki@0b36536

The syntax is discernible from the test case -- it essentially adds a strip_import_prefix and an import_prefixattribute toproto_librarythat mirror what theinclude_prefix/strip_include_prefixattributes oncc_library` do.

We are cutting 0.21 sometime early next week, so please test it so that I know if I got something wrong! The code is still very rough and untested and contains some hacks, but the functionality should be fully there.

@jmillikin-stripe
Copy link
Contributor Author

@jmillikin-stripe jmillikin-stripe commented Nov 26, 2018

Just tested that branch, seems to be working in a few quick tests. @jayconrod will want to do tests with Gazelle to verify it can handle the Kubernetes case.

I found the syntax strip_prefix="/g/bad" to be really strange because it doesn't match the cc_library attribute of the same name. I expected strip_prefix="teststrip" to work, but instead got an error .proto file 'teststrip/file.proto' is not under the specific strip prefix 'teststrip/teststrip'. It seems like that attribute is interpreted relative to the current package, instead of relative to the repo.

@lberki
Copy link
Contributor

@lberki lberki commented Nov 27, 2018

@jmillikin-stripe , I entirely agree that this isn't very intuitive. However, cc_library does behave in the way you mentioned: relative strip_include_prefixes are relative to the package, absolute ones are relative to the repository root:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java#L895

@jmillikin-stripe
Copy link
Contributor Author

@jmillikin-stripe jmillikin-stripe commented Nov 27, 2018

Oh wow, you're right -- sorry! Must have some old cobwebs still sticking around.

@lberki
Copy link
Contributor

@lberki lberki commented Nov 28, 2018

/cc @haberman

@lberki
Copy link
Contributor

@lberki lberki commented Nov 28, 2018

Update: some concerns were raised that strip_import_prefix and import_prefix together are too powerful, so instead of submitting something quickly so that it makes it into 0.21, we'll take a bit more time. So 0.22 it is.

@jmillikin-stripe
Copy link
Contributor Author

@jmillikin-stripe jmillikin-stripe commented Nov 28, 2018

If a less powerful version of those two is merged, I'm going to open a new issue asking for the full-power version. Bazel should be useful for building code that depends on third-party dependencies, even if those dependencies are not compliant with Protobuf best practices regarding import paths.

Specifically: the custom Starlark go_proto_library I wrote to get this feature supports both, and I use all combinations of [add, strip, strip+add] to build some of our more complex tools, so the requested power is the minimum required to switch to the builtin proto_library rule.

@bazel-io bazel-io closed this in 8191c62 Dec 6, 2018
@lberki
Copy link
Contributor

@lberki lberki commented Dec 7, 2018

Update: the full version of this change (stripping + adding prefixes) was merged in the change above and should be in 0.22 . Build a Bazel at HEAD To give it a spin ahead of time!

pranj added a commit to yext/cloudprober that referenced this issue Apr 17, 2019
import prefix and strip import prefix needed to be set because of:
bazelbuild/bazel#3867

resolve directives are to work around bugs/limitations in gazelle and
were manually crafted from looking at package names in the proto files.

resolve directives require specifying both a source-lang and import-lang
despite it being the same because of another bug/limitation within
gazelle

Generate protos with:

gazelle update -repo_root `pwd` && \
find . -type f -name "BUILD.bazel" -print0 | xargs -0 sed -i '' -e 's#//github.com/yext/cloudprober/#//#g'
pranj added a commit to yext/cloudprober that referenced this issue May 21, 2019
import prefix and strip import prefix needed to be set because of:
bazelbuild/bazel#3867

resolve directives are to work around bugs/limitations in gazelle and
were manually crafted from looking at package names in the proto files.

resolve directives require specifying both a source-lang and import-lang
despite it being the same because of another bug/limitation within
gazelle

Generate protos with:

gazelle update -repo_root `pwd` && \
find . -type f -name "BUILD.bazel" -print0 | xargs -0 sed -i '' -e 's#//github.com/yext/cloudprober/#//#g'
@tristanz
Copy link

@tristanz tristanz commented Sep 20, 2019

The readme currently references this issue despite it apparently being fixed already.

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.