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

Always expose ctx.var["BINDIR"] to actions #15470

Open
alexeagle opened this issue May 11, 2022 · 3 comments
Open

Always expose ctx.var["BINDIR"] to actions #15470

alexeagle opened this issue May 11, 2022 · 3 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request

Comments

@alexeagle
Copy link
Contributor

alexeagle commented May 11, 2022

Description of the feature request:

At aspect we're working on a new ruleset where every action spawn of a js_binary needs to know the BINDIR variable.

Every Bazel action implicitly knows the bindir, as the paths for output files have it as a prefix. Thus adding it shouldn't cause any new action cache invalidations.

Today users can already do this, such as in genrule by referencing BINDIR=$(BINDIR) or in ctx.actions.run with env = { "BINDIR": ctx.bin_dir.path}. However, this makes an extra burden for users of rules there is no standard name for this environment variable, and the tool which needs this in the environment ought to be an abstraction which doesn't leak to the callsites.

@brandjon brandjon added team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-Build-Language labels Nov 5, 2022
@comius comius added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Nov 17, 2022
@gonzojive
Copy link
Contributor

I only marginally understand the issue here, but I believe this is getting in the way of using a Typescript binary target to implement a protobuf plugin (aspect-build/rules_js#397).

gonzojive added a commit to gonzojive/rules_proto_grpc that referenced this issue Nov 19, 2022
* Add an "env" property to ProtoPluginInfo that contains an environment variable
  dictionary.

* Add a corresponding "env" attribute to the the proto_plugin rule.

* For cases where environment variables must specified based on ctx, as
  described in bazelbuild/bazel#15470 and
  aspect-build/rules_js#397, modify proto_compile_impl
  and proto_compile to accept a base_env dictionary argument. Example usage is
  below:

```starlark

def _ts_proto_compile_impl(ctx):
    """
    Implementation function for ts_proto_compile.

    Args:
        ctx: The Bazel rule execution context object.

    Returns:
        Providers:
            - ProtoCompileInfo
            - DefaultInfo

    """
    base_env = {
        # Make up for bazelbuild/bazel#15470.
        "BAZEL_BINDIR": ctx.bin_dir.path,
    }
    return proto_compile_impl(ctx, base_env = base_env)

ts_proto_compile = rule(
    implementation = _ts_proto_compile_impl,
    attrs = dict(
        proto_compile_attrs,
        _plugins = attr.label_list(
            providers = [ProtoPluginInfo],
            default = [
                Label("//bazel/web/targets:ts_proto_compile"),
            ],
            doc = "List of protoc plugins to apply",
        ),
    ),
    toolchains = [
        str(Label("@rules_proto_grpc//protobuf:toolchain_type")),
    ],
)
```

```starlark
load("@npm//:ts-proto/package_json.bzl", _ts_proto_bin_factories = "bin")
load("@rules_proto_grpc//:defs.bzl", "proto_plugin")

_ts_proto_bin_factories.protoc_gen_ts_proto_binary(
    name = "protoc-gen-ts-proto",
)

proto_plugin(
    name = "ts_proto_compile",
    outputs = ["{protopath}.ts"],
    protoc_plugin_name = "ts_proto",
    tool = "//bazel/web/targets:protoc-gen-ts-proto",
    use_built_in_shell_environment = False,
    visibility = ["//visibility:public"],
)
```
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jan 24, 2024
@alexeagle
Copy link
Contributor Author

We could send a PR for this

@alexeagle alexeagle reopened this Jan 24, 2024
@github-actions github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants
@alexeagle @gonzojive @brandjon @comius @sgowroji and others