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

Allow starlark rules to pass --run_under to executables #16232

Open
UebelAndre opened this issue Sep 7, 2022 · 8 comments
Open

Allow starlark rules to pass --run_under to executables #16232

UebelAndre opened this issue Sep 7, 2022 · 8 comments
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: feature request

Comments

@UebelAndre
Copy link
Contributor

UebelAndre commented Sep 7, 2022

Description of the feature request:

I would like to be able to intercept the --run_under flag in a starlark rule to be able to pass it as an environment variable or argument to a test or action to have better control over when it's used. In rules_rust, executables are the compiled Rust binary which allows me to pass --run_under and start a debugger (see bazelbuild/rules_rust#370 (comment)). However, there's some interest in being able to instead generate a process wrapper that would subprocess or exec into the compiled executable (bazelbuild/rules_rust#1303). If this were to be the case, the current use case of --run_under would break as the thing being executed by --run_under is a process wrapper. I want to be able to use process wrappers and maintain the intent of --run_under being run on the correct executable and not a process wrapper.

What underlying problem are you trying to solve with this feature?

By being able to control when --run_under is used, rules authors would be able to more accurately translate the intent of --run_under to run the correct executable under the specified command while still handling ephemeral things the process may emit (coverage or profiling reports, etc).

Which operating system are you running Bazel on?

Linux, MacOS, Windows

What is the output of bazel info release?

release 5.3.0

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 type: feature request team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. untriaged labels Sep 7, 2022
@Wyverald Wyverald added team-Build-Language and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Sep 27, 2022
@UebelAndre
Copy link
Contributor Author

@fmeum wondering if you have thoughts on this one. Could this be handled similar to the proposal for RunEnvironmentInfo.arguments (#16076) where RunEnvironmentInfo gains a run_under field that:

  • If set will follow the standard --run_under behavior.
  • If set to None, will explicitly disable --run_under.

Additionally, the --run_under would then be consumable via ctx.var where there'd be some RUN_UNDER value equivalent to whatever the user set (perhaps, not sure). This way rule authors could set custom environment variables via RunEnvironmentInfo to have their process wrappers invoke the intended binary for the --run_under content.

@fmeum
Copy link
Collaborator

fmeum commented Oct 8, 2022

I'm not familiar enough with --run_under to have a qualified opinion.

Would your proposal work with both simple commands and Bazel targets passed in as --run_under? Could you give an example where a rule would need to act on the particular value of --run_under at analysis time?

@UebelAndre
Copy link
Contributor Author

UebelAndre commented Oct 8, 2022

Today I have the rust_test rule which looks something like the following

def _rust_test_impl(ctx):
    test_binary = rust_common.compile(
        ctx,
        # ...
    )

    return DefaultInfo(
        files = depset([test_binary]),
        executable = test_binary,
    )

rust_test(
    test = True,
    _implementation = _rust_test_impl,
    # ...
)

The way this is written, I'm able to run a similar command to the following to start the test and attach a debugger to the test binary.

bazel test //my:test --run_under=/usr/local/bin/lldb

This works because the argv for the test roughly looks like ["/usr/local/bin/lldb", "bazel-bin/my/test"]. However there's a feature request to do some preprocessing before starting Rust tests (e.g. bazelbuild/rules_rust#927). The implementation of this would look something like the following.

def _rust_test_impl(ctx):
    test_binary = rust_common.compile(
        ctx,
        # ...
    )

    # Note the creation of process wrapper here.
    process_wrapper = _create_rust_test_process_wrapper(
        ctx,
        # ...
    )

    return [
        DefaultInfo(
            files = depset([test_binary]),
            runfiles = ctx.runfiles(files=[test_binary])
            # Note that the process wrapper is the executable
            executable = process_wrapper,
        ),
        RunEnvironmentInfo(
            arguments=[test_binary.short_path],
        ),
    ]

rust_test(
    test = True,
    _implementation = _rust_test_impl,
    # ...
)

This then breaks the use of --run_under with the test binary as argv would have transformed into ["/usr/local/bin/lldb", "bazel-bin/my/process_wrapper", "bazel-bin/my/test"]. My proposal is to be able to make a similar update to the rule:

def _rust_test_impl(ctx):
    test_binary = rust_common.compile(
        ctx,
        # ...
    )

    process_wrapper = _create_rust_test_process_wrapper(
        ctx,
        # ...
    )

    # This is not the proposed way to acess --run_under, just an example to demonstrate it's use
    run_under = ctx.var.get("RUN_UNDER", "")

    return [
        DefaultInfo(
            files = depset([test_binary]),
            runfiles = ctx.runfiles(files=[test_binary])
            executable = process_wrapper,
        ),
        RunEnvironmentInfo(
            environment={
                "RUST_TEST_RUN_UNDER": run_under,
            }
            arguments=[test_binary.short_path],
            # Note that this prevents the command line argument for --run_under from being passed to the
            # process wrapper. The process wrapper instead is aware of the uniquely named environment variable
            # RUST_TEST_RUN_UNDER and would be able to use the value there to correctly invoke the `test_binary`
            # created in this rule.
            run_under="",
        ),
    ]

rust_test(
    test = True,
    _implementation = _rust_test_impl,
    # ...
)

Does this provide some clarity?

@fmeum
Copy link
Collaborator

fmeum commented Oct 8, 2022

Thanks for the extensive example, that definitely helped.

It looks like a much simpler API may suffice for this use case: Adding a run_under_variable argument to RunEnvironmentInfo that, if set to FOO_RUN_UNDER, would run the executable with the environment variable FOO_RUN_UNDER set to the binary or command pointed to by --run_under instead of prepending that command to the test invocation's command line.

@UebelAndre Does that sound sufficient? It would get around the potentially problematic analysis time access to the --run_under value.

@UebelAndre
Copy link
Contributor Author

@UebelAndre Does that sound sufficient? It would get around the potentially problematic analysis time access to the --run_under value.

Yeah, great idea! This particular feature would help immensely in the developer workflow story for many projects I work on.

@UebelAndre
Copy link
Contributor Author

@fmeum Over the weekend I was able to do #16540 thanks to the other PR you recently opened that changes RunEnvironmentInfo. Does the approach there seem reasonable and would you or someone else be able to help me finish the implementation?

@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 P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Sep 14, 2023
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 Nov 18, 2024
@UebelAndre
Copy link
Contributor Author

This is still desired.

@github-actions github-actions bot removed the stale Issues or PRs that are stale (no activity for 30 days) label Nov 19, 2024
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: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants