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

Generated Protos are no longer considered as virtual_imports in Bazel 7 #21075

Open
XuanWang-Amos opened this issue Jan 26, 2024 · 3 comments
Open
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: bug

Comments

@XuanWang-Amos
Copy link

Description of the bug:

One of our test starts failing in Bazel 7, after some investigation, we found out it's because Bazel 7.0.0 removed the generated file check and stopped symlink our generated proto to _virtual_imports.

  • Bazel 6.4.0 will symlink generated proto to _virtual_imports because it's a generated file (link to code):
    generate_protos_in_virtual_imports = False
    if ctx.fragments.proto.generated_protos_in_virtual_imports():
        generate_protos_in_virtual_imports = any([not src.is_source for src in srcs])

    if import_prefix != "" or strip_import_prefix != "" or generate_protos_in_virtual_imports:
        # Use virtual source roots
        return _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix)
  • Bazel 7.0.0 removed the generated file check thus it will not symlink generated proto to _virtual_imports (link to code):
    if import_prefix != "" or strip_import_prefix != "":
        # Use virtual source roots
        return _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix)
    else:
        # No virtual source roots
        return "", srcs

Is this change of behavior expected? If it is, can someone share some context on why it was changed?

Link to our issue: grpc/grpc#35391

Which category does this issue belong to?

Rules API

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Build this with Bazel 7: https://github.com/grpc/grpc/blob/714640d71f07d415a14aab2c68e8f37d74c644aa/test/distrib/bazel/python/BUILD#L123

Which operating system are you running Bazel on?

Not related

What is the output of bazel info release?

Not related

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

Not related

What's the output of git remote get-url origin; git rev-parse HEAD ?

Not related

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No

Have you found anything relevant by searching the web?

No

Any other information, logs, or outputs that you want to share?

No response

@iancha1992 iancha1992 added the team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts label Jan 26, 2024
@comius
Copy link
Contributor

comius commented Jan 29, 2024

Is this change of behavior expected? If it is, can someone share some context on why it was changed?

Yes, this change of behaviour is intentional and expected (I just wish I caught it earlier in grpc). _virtual_imports are created only when it's strictly necessary. That's when proto_library uses import_prefix or strip_import_prefix.

The reason for the change was, that the behaviour was is some cases incorrect (external repos using sibling repository layout) and creating _virtual_imports created a performance penalty.

Ideally proto generation would happen using proto_common.compile, where you don't need to specify output directories anymore. A quicker workaround would be to copy _output_directory implementation from proto_common.bzl in Bazel.

@comius comius added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Jan 29, 2024
@XuanWang-Amos
Copy link
Author

Thanks for the reply, do you mind share more details on the workaround?

By copy _output_directory implementation you mean we can use _output_directory to replace the outputs here? What about the parameters, I guess we can use copied_proto as root, what should be our proto_info here?

@XuanWang-Amos
Copy link
Author

@comius We're still trying to figure out the correct fix, any further details you can provide would be extremely helpful.

mbland added a commit to mbland/rules_scala that referenced this issue Oct 5, 2024
Related to bazelbuild#1482, bazelbuild#1618, and bazelbuild#1619. Results from the investigation
documented at:

- bazelbuild#1619 (comment)

Updates `_import_paths()` in `scala_proto_aspect.bzl` to handle
differences `ProtoInfo.proto_source_root` and `ProtoInfo.direct_sources`
values between Bazel 6 and Bazel 7.

Without this change, `_import_paths()` emits incorrect values under
Bazel 7, causing targets containing generated `.proto` inputs to fail,
e.g.  `//test/proto3:test_generated_proto`.

See also:

- Fix paths for sibling repository setup and generated .proto files
  bazelbuild/bazel@6c6c196

- The docstring for `ProtoInfo.proto_source_root` in the Bazel sources:
  https://github.com/bazelbuild/bazel/blob/7.3.2/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl#L155-L172

- Remove incompatible_generated_protos_in_virtual_imports
  bazelbuild/bazel@3efaa32

- Comment from: Generated Protos are no longer considered as
  virtual_imports in Bazel 7
  bazelbuild/bazel#21075 (comment)

---

I cherrypicked this commit into bazelbuild#1618. While it fixed the
`//test/proto3` build failure, it does _not_ fix the hanging
scalapb_workers from the ProtoScalaPBRule aspect.

I'll have to investiate further whether than hang is related to Bazel,
rules_proto, com_google_protobuf, or some mixture thereof. Still,
progress!
mbland added a commit to mbland/rules_scala that referenced this issue Oct 5, 2024
Related to bazelbuild#1482, bazelbuild#1618, and bazelbuild#1619. Results from the investigation
documented at:

- bazelbuild#1619 (comment)

Updates `_import_paths()` in `scala_proto_aspect.bzl` to handle
differences `ProtoInfo.proto_source_root` and `ProtoInfo.direct_sources`
values between Bazel 6 and Bazel 7.

Without this change, `_import_paths()` emits incorrect values under
Bazel 7, causing targets containing generated `.proto` inputs to fail,
e.g.  `//test/proto3:test_generated_proto`.

See also:

- Fix paths for sibling repository setup and generated .proto files
  bazelbuild/bazel@6c6c196

- The docstring for `ProtoInfo.proto_source_root` in the Bazel sources:
  https://github.com/bazelbuild/bazel/blob/7.3.2/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl#L155-L172

- Remove incompatible_generated_protos_in_virtual_imports
  bazelbuild/bazel@3efaa32

- Comment from: Generated Protos are no longer considered as
  virtual_imports in Bazel 7
  bazelbuild/bazel#21075 (comment)

---

I cherrypicked this commit into bazelbuild#1618. While it fixed the
`//test/proto3` build failure, it does _not_ fix the hanging
scalapb_workers from the ProtoScalaPBRule aspect.

I'll have to investiate further whether than hang is related to Bazel,
rules_proto, com_google_protobuf, or some mixture thereof. Still,
progress!
mbland added a commit to mbland/rules_scala that referenced this issue Oct 6, 2024
Related to bazelbuild#1482, bazelbuild#1618, and bazelbuild#1619. Results from the investigation
documented at:

- bazelbuild#1619 (comment)

Updates `_import_paths()` in `scala_proto_aspect.bzl` to handle
differences `ProtoInfo.proto_source_root` and `ProtoInfo.direct_sources`
values between Bazel 6 and Bazel 7.

Without this change, `_import_paths()` emits incorrect values under
Bazel 7, causing targets containing generated `.proto` inputs to fail,
e.g.  `//test/proto3:test_generated_proto`.

See also:

- Fix paths for sibling repository setup and generated .proto files
  bazelbuild/bazel@6c6c196

- The docstring for `ProtoInfo.proto_source_root` in the Bazel sources:
  https://github.com/bazelbuild/bazel/blob/7.3.2/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl#L155-L172

- Remove incompatible_generated_protos_in_virtual_imports
  bazelbuild/bazel@3efaa32

- Comment from: Generated Protos are no longer considered as
  virtual_imports in Bazel 7
  bazelbuild/bazel#21075 (comment)

---

I cherrypicked this commit into bazelbuild#1618. While it fixed the
`//test/proto3` build failure, it does _not_ fix the hanging
scalapb_workers from the ProtoScalaPBRule aspect.

I'll have to investiate further whether than hang is related to Bazel,
rules_proto, com_google_protobuf, or some mixture thereof. Still,
progress!
mbland added a commit to mbland/rules_scala that referenced this issue Oct 7, 2024
Related to bazelbuild#1482, bazelbuild#1618, and bazelbuild#1619. Results from the investigation
documented at:

- bazelbuild#1619 (comment)

Updates `_import_paths()` in `scala_proto_aspect.bzl` to handle
differences `ProtoInfo.proto_source_root` and `ProtoInfo.direct_sources`
values between Bazel 6 and Bazel 7.

Without this change, `_import_paths()` emits incorrect values under
Bazel 7, causing targets containing generated `.proto` inputs to fail,
e.g.  `//test/proto3:test_generated_proto`.

See also:

- Fix paths for sibling repository setup and generated .proto files
  bazelbuild/bazel@6c6c196

- The docstring for `ProtoInfo.proto_source_root` in the Bazel sources:
  https://github.com/bazelbuild/bazel/blob/7.3.2/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl#L155-L172

- Remove incompatible_generated_protos_in_virtual_imports
  bazelbuild/bazel@3efaa32

- Comment from: Generated Protos are no longer considered as
  virtual_imports in Bazel 7
  bazelbuild/bazel#21075 (comment)

---

I cherrypicked this commit into bazelbuild#1618. While it fixed the
`//test/proto3` build failure, it does _not_ fix the hanging
scalapb_workers from the ProtoScalaPBRule aspect.

I'll have to investiate further whether than hang is related to Bazel,
rules_proto, com_google_protobuf, or some mixture thereof. Still,
progress!
liucijus pushed a commit to bazelbuild/rules_scala that referenced this issue Oct 8, 2024
Related to #1482, #1618, and #1619. Results from the investigation
documented at:

- #1619 (comment)

Updates `_import_paths()` in `scala_proto_aspect.bzl` to handle
differences `ProtoInfo.proto_source_root` and `ProtoInfo.direct_sources`
values between Bazel 6 and Bazel 7.

Without this change, `_import_paths()` emits incorrect values under
Bazel 7, causing targets containing generated `.proto` inputs to fail,
e.g.  `//test/proto3:test_generated_proto`.

See also:

- Fix paths for sibling repository setup and generated .proto files
  bazelbuild/bazel@6c6c196

- The docstring for `ProtoInfo.proto_source_root` in the Bazel sources:
  https://github.com/bazelbuild/bazel/blob/7.3.2/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl#L155-L172

- Remove incompatible_generated_protos_in_virtual_imports
  bazelbuild/bazel@3efaa32

- Comment from: Generated Protos are no longer considered as
  virtual_imports in Bazel 7
  bazelbuild/bazel#21075 (comment)

---

I cherrypicked this commit into #1618. While it fixed the
`//test/proto3` build failure, it does _not_ fix the hanging
scalapb_workers from the ProtoScalaPBRule aspect.

I'll have to investiate further whether than hang is related to Bazel,
rules_proto, com_google_protobuf, or some mixture thereof. Still,
progress!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: bug
Projects
None yet
Development

No branches or pull requests

5 participants