-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Always strip /external/<repo_name> from proto import path #8683
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hlopko Thanks for working on this!
I've tried this, and it seems to fix the issue with proto_library
.
However, it looks like cc_proto_library
, and possibly ProtoInfo#proto_source_root
, also need to be fixed. java_proto_library
already works with this change.
Below is an example for cc_proto_library
. I'll have a look whether there's a bug in my starlark rule that uses ProtoInfo#proto_source_root
or if that needs to be fixed Bazel-side.
proto_library(
name = "some_proto",
srcs = ["some.proto"],
import_prefix = "add_prefix",
)
cc_proto_library(
name = "some_cc_proto",
deps = [":some_proto"],
)
$ bazel build @private_repo//redacted:some_cc_proto
...
in BazelCcProtoAspect aspect on proto_library rule @private_repo//redacted:some_proto: header 'bazel-out/darwin-fastbuild/bin/external/private_repo/redacted/_virtual_imports/some_proto/add_prefix/redacted/some.pb.h' is not under the specified strip prefix 'external/private_repo/external/private_repo/redacted/_virtual_imports/some_proto'
I'll look into I don't plan to fix |
I was talking of this proto_source_root which, IIUC, isn't part of the deprecation in #7153. |
proto_library.proto_source_root is still broken for external workspaces after bazelbuild/bazel#8683 lands. Since the attribute is deprecated anyway (bazelbuild/bazel#7153), this will not be fixed in Bazel (bazelbuild/bazel#8683 (comment)). This change removes the tests that verify our rules work when targets define proto_library.proto_source_root.
Updated the PR with the fix for cc_proto_library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for looking into this! |
Before this PR, when strip_import_prefix was not specified, and the proto target was declared in an external repository, Bazel generated import path that leaked `external/<repository_name>` path fragment. That path fragment is an implementation detail of Bazel's execroot layout and should not be used in proto source files. This PR adds a correct path fragment to the import paths. It also introduces an incompatible flag `--incompatible_do_not_emit_buggy_external_repo_import` that removes the incorrect import path from Bazel. Fixes bazelbuild#8030. Incompatible change: bazelbuild#8711. Closes bazelbuild#8683. PiperOrigin-RevId: 255606742
Before this PR, when strip_import_prefix was not specified, and the proto target was declared in an external repository, Bazel generated import path that leaked `external/<repository_name>` path fragment. That path fragment is an implementation detail of Bazel's execroot layout and should not be used in proto source files. This PR adds a correct path fragment to the import paths. It also introduces an incompatible flag `--incompatible_do_not_emit_buggy_external_repo_import` that removes the incorrect import path from Bazel. Fixes bazelbuild#8030. Incompatible change: bazelbuild#8711. Closes bazelbuild#8683. PiperOrigin-RevId: 255606742
Before this PR, when strip_import_prefix was not specified, and the proto target
was declared in an external repository, Bazel generated import path that leaked
external/<repository_name>
path fragment. That path fragment is animplementation detail of Bazel's execroot layout and should not be used in proto
source files.
This PR removes this path fragment from the import path.
Fixes #8030.