bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.21k stars 4.07k forks source link

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

Open UebelAndre opened 2 years ago

UebelAndre commented 2 years ago

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 https://github.com/bazelbuild/rules_rust/issues/370#issuecomment-796592288). However, there's some interest in being able to instead generate a process wrapper that would subprocess or exec into the compiled executable (https://github.com/bazelbuild/rules_rust/issues/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

UebelAndre commented 2 years ago

@fmeum wondering if you have thoughts on this one. Could this be handled similar to the proposal for RunEnvironmentInfo.arguments (https://github.com/bazelbuild/bazel/issues/16076) where RunEnvironmentInfo gains a run_under field that:

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 commented 2 years ago

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 commented 2 years ago

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. https://github.com/bazelbuild/rules_rust/issues/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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@fmeum Over the weekend I was able to do https://github.com/bazelbuild/bazel/pull/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?