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

proto_common.compile() should not require plugin_output parameter #18263

Closed
haberman opened this issue Apr 28, 2023 · 1 comment
Closed

proto_common.compile() should not require plugin_output parameter #18263

haberman opened this issue Apr 28, 2023 · 1 comment
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request

Comments

@haberman
Copy link
Contributor

Description of the bug:

proto_common.compile() has a plugin_output parameter that a user must pass to specify where the output must go. But specifying it properly requires some tricky logic to handle all edge cases. Users have to write something like this:

load("//tools/build_defs/proto:proto_common.bzl", "proto_common")

def output_dir(ctx, proto_info):
    """Returns the output directory where generated proto files will be placed.

    Args:
      ctx: Rule context.
      proto_info: ProtoInfo provider.

    Returns:
      A string specifying the output directory
    """
    proto_root = proto_info.proto_source_root
    if proto_root.startswith(ctx.bin_dir.path):
        path = proto_root
    else:
        path = ctx.bin_dir.path + "/" + proto_root

    if proto_root == ".":
        path = ctx.bin_dir.path
    return path

def proto_common_compile(ctx, proto_info, proto_lang_toolchain_info, generated_files):
    """A wrapper around proto_common.compile that automatically calculates the output dir.

    Args:
      ctx: Rule context.
      proto_info: ProtoInfo provider.
      proto_lang_toolchain_info: ProtoLangToolchainInfo provider.
      generated_files: The files we expect to be generated from this protoc invocation.
    """
    proto_common.compile(
        actions = ctx.actions,
        proto_info = proto_info,
        proto_lang_toolchain_info = proto_lang_toolchain_info,
        generated_files = generated_files,
        plugin_output = output_dir(ctx, proto_info),
    )

proto_common.compile() should handle this internally, rather than making users copy and paste this code.

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

Use proto_common.compile() without the special code above and watch it break with errors like:

ERROR: /usr/local/google/home/haberman/.cache/bazel/_bazel_haberman/17449526a2508fe9f4def2619c761b7e/external/com_google_googleapis/BUILD.bazel:31:14: output 'external/com_google_googleapis/google/rpc/status.upb.h' was not created

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

release 6.1.2

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

No response

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

No response

Have you found anything relevant by searching the web?

No response

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

No response

@sgowroji sgowroji added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Apr 28, 2023
@meteorcloudy meteorcloudy added team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels May 2, 2023
@comius comius self-assigned this May 16, 2023
@comius comius added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels May 16, 2023
@comius
Copy link
Contributor

comius commented May 16, 2023

I'm working on this. It's not completely true. The output parameter is formatted differently if protoc generates a single file. For example for desciptor sets, java .jar file. Possible solution is to add a that bit of information to proto_lang_toolchain and then completely remove the parameter from proto_common.compile

copybara-service bot pushed a commit that referenced this issue Jun 20, 2023
…_info

NEW: don't try to compute plugin_output automatically

Additional information is needed whether protoc generates a single file or multiple files.

Add output_files to proto_lang_toolchain (enum "single","multiple") and propagate it through ProtoLangToolchainInfo.

Add experimental_output_files to proto_common.compile, that can override the value, for faster migration path.

When the value is set, automatically compute plugin_output.

When the (legacy) plugin_output is not set to a file, set it automatically to correct directory.

AI: Cherry-pick this change to Bazel minor release and follow up with updates to proto_lang_toolchain.

Fixes: #18263
Tracking issue: #18623
PiperOrigin-RevId: 541964181
Change-Id: Ie4b4792287723798ffdd4047562d62eb05d1b731
traversaro pushed a commit to traversaro/bazel that referenced this issue Jun 24, 2023
Additional information is needed whether protoc generates a single file or multiple files.

Add output_files to proto_lang_toolchain (enum "single","multiple") and propagate it through ProtoLangToolchainInfo.

Add experimental_output_files to proto_common.compile, that can override the value, for faster migration path.

When the value is set, automatically compute plugin_output.

When the (legacy) plugin_output is not set to a file, set it automatically to correct directory.

AI: Cherry-pick this change to Bazel minor release and follow up with updates to proto_lang_toolchain.

Fixes: bazelbuild#18263
Tracking issue: bazelbuild#18623
PiperOrigin-RevId: 539047857
Change-Id: Id5c3f48f02ad245bee90eb113d5ba4f681c650ac
comius added a commit to comius/bazel that referenced this issue Jun 26, 2023
… from proto_info

NEW: don't try to compute plugin_output automatically

Additional information is needed whether protoc generates a single file or multiple files.

Add output_files to proto_lang_toolchain (enum "single","multiple") and propagate it through ProtoLangToolchainInfo.

Add experimental_output_files to proto_common.compile, that can override the value, for faster migration path.

When the value is set, automatically compute plugin_output.

When the (legacy) plugin_output is not set to a file, set it automatically to correct directory.

AI: Cherry-pick this change to Bazel minor release and follow up with updates to proto_lang_toolchain.

Fixes: bazelbuild#18263
Tracking issue: bazelbuild#18623
PiperOrigin-RevId: 541964181
Change-Id: Ie4b4792287723798ffdd4047562d62eb05d1b731
iancha1992 added a commit that referenced this issue Jul 12, 2023
…d2be27ab… (#18773)

* Rollforward of 482d2be: Compute the value of plugin_output from proto_info

NEW: don't try to compute plugin_output automatically

Additional information is needed whether protoc generates a single file or multiple files.

Add output_files to proto_lang_toolchain (enum "single","multiple") and propagate it through ProtoLangToolchainInfo.

Add experimental_output_files to proto_common.compile, that can override the value, for faster migration path.

When the value is set, automatically compute plugin_output.

When the (legacy) plugin_output is not set to a file, set it automatically to correct directory.

AI: Cherry-pick this change to Bazel minor release and follow up with updates to proto_lang_toolchain.

Fixes: #18263
Tracking issue: #18623
PiperOrigin-RevId: 541964181
Change-Id: Ie4b4792287723798ffdd4047562d62eb05d1b731

* Fix tests

---------

Co-authored-by: Ian (Hee) Cha <heec@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants