Skip to content

Commit

Permalink
Add arguments parameter to RunEnvironmentInfo
Browse files Browse the repository at this point in the history
Executable Starlark rules can use the new `arguments` parameter on
`RunEnvironmentInfo` to specify the arguments that Bazel should pass
on the command line with `test` or `run`.

If set to a non-`None` value, this parameter overrides the value of
the `args` attribute that is implicitly defined for all rules. This
allows Starlark rules to implement their own version of this attribute
which isn't bound to its proprietary processing (data label expansion
and tokenization).

Along the way, this commit adds test coverage and documentation for the
interplay between `RunEnvironmentInfo`'s `environment` and `--test_env`.

The value of the `arguments` field of `RunEnvironmentInfo` is
intentionally not exposed to Starlark yet: It is not clear how these
arguments should be represented and whether rules relying on the magic
`args` attribute should also provide this field.

RELNOTES: Executable starlark rules can use the `arguments` parameter of
`RunEnvironmentInfo` to specify their command-line arguments with `bazel
run` and `bazel test`.
  • Loading branch information
fmeum committed Oct 8, 2022
1 parent 7cc4812 commit d747d2e
Show file tree
Hide file tree
Showing 12 changed files with 210 additions and 42 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,7 @@ java_library(
name = "run_environment_info",
srcs = ["RunEnvironmentInfo.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ private TestProvider initializeTestProvider(FilesToRunProvider filesToRunProvide
if (environmentProvider != null) {
testActionBuilder.addExtraEnv(environmentProvider.getEnvironment());
testActionBuilder.addExtraInheritedEnv(environmentProvider.getInheritedEnvironment());
testActionBuilder.setStarlarkTargetArgs(environmentProvider.getArguments());
}

TestParams testParams =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,18 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.BuiltinProvider;
import com.google.devtools.build.lib.packages.NativeInfo;
import com.google.devtools.build.lib.starlarkbuildapi.RunEnvironmentInfoApi;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Sequence;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkList;

/**
Expand All @@ -40,16 +43,19 @@ public final class RunEnvironmentInfo extends NativeInfo implements RunEnvironme

private final ImmutableMap<String, String> environment;
private final ImmutableList<String> inheritedEnvironment;
@Nullable private final CommandLine arguments;
private final boolean shouldErrorOnNonExecutableRule;

/** Constructs a new provider with the given fixed and inherited environment variables. */
public RunEnvironmentInfo(
Map<String, String> environment,
List<String> inheritedEnvironment,
@Nullable CommandLine arguments,
boolean shouldErrorOnNonExecutableRule) {
this.environment = ImmutableMap.copyOf(Preconditions.checkNotNull(environment));
this.inheritedEnvironment =
ImmutableList.copyOf(Preconditions.checkNotNull(inheritedEnvironment));
this.arguments = arguments;
this.shouldErrorOnNonExecutableRule = shouldErrorOnNonExecutableRule;
}

Expand All @@ -76,6 +82,11 @@ public ImmutableList<String> getInheritedEnvironment() {
return inheritedEnvironment;
}

@Nullable
public CommandLine getArguments() {
return arguments;
}

/**
* Returns whether advertising this provider on a non-executable (and thus non-test) rule should
* result in an error or a warning. The latter is required to not break testing.TestEnvironment,
Expand All @@ -95,11 +106,12 @@ private RunEnvironmentInfoProvider() {

@Override
public RunEnvironmentInfoApi constructor(
Dict<?, ?> environment, Sequence<?> inheritedEnvironment) throws EvalException {
Dict<?, ?> environment, Sequence<?> inheritedEnvironment, Object arguments) throws EvalException {
return new RunEnvironmentInfo(
Dict.cast(environment, String.class, String.class, "environment"),
StarlarkList.immutableCopyOf(
Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")),
arguments == Starlark.NONE ? null : CommandLine.of(Sequence.cast(arguments, String.class, "arguments")),
/* shouldErrorOnNonExecutableRule= */ true);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,20 @@ public static RunfilesSupport withExecutable(
computeActionEnvironment(ruleContext));
}

/**
* Creates and returns a {@link RunfilesSupport} object for the given rule and executable without
* computing arguments based on the value of the "args" attribute.
*/
public static RunfilesSupport withExecutableNoArgs(
RuleContext ruleContext, Runfiles runfiles, Artifact executable) {
return RunfilesSupport.create(
ruleContext,
executable,
runfiles,
CommandLine.EMPTY,
computeActionEnvironment(ruleContext));
}

/**
* Creates and returns a {@link RunfilesSupport} object for the given rule and executable. Note
* that this method calls back into the passed in rule to obtain the runfiles.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -343,30 +343,40 @@ private static void addProviders(
}
}

DefaultInfo defaultInfo = null;
boolean defaultProviderProvidedExplicitly = false;
boolean starlarkArgsProvided = false;

for (Info declaredProvider : declaredProviders.values()) {
if (getProviderKey(declaredProvider).equals(DefaultInfo.PROVIDER.getKey())) {
parseDefaultProviderFields((DefaultInfo) declaredProvider, context, builder);
defaultInfo = (DefaultInfo) declaredProvider;
defaultProviderProvidedExplicitly = true;
} else if (getProviderKey(declaredProvider).equals(RunEnvironmentInfo.PROVIDER.getKey())
&& !(context.isExecutable() || context.getRuleContext().isTestTarget())) {
String message =
"Returning RunEnvironmentInfo from a non-executable, non-test target has no effect";
RunEnvironmentInfo runEnvironmentInfo = (RunEnvironmentInfo) declaredProvider;
if (runEnvironmentInfo.shouldErrorOnNonExecutableRule()) {
context.getRuleContext().ruleError(message);
} else {
context.getRuleContext().ruleWarning(message);
builder.addStarlarkDeclaredProvider(declaredProvider);
} else if (getProviderKey(declaredProvider).equals(RunEnvironmentInfo.PROVIDER.getKey())) {
if (((RunEnvironmentInfo) declaredProvider).getArguments() != null) {
// Ensure that RunfilesSupport does not parse the "args" attribute, the rule handles it in
// a custom way.
starlarkArgsProvided = true;
}
if (!(context.isExecutable() || context.getRuleContext().isTestTarget())) {
String message =
"Returning RunEnvironmentInfo from a non-executable, non-test target has no effect";
RunEnvironmentInfo runEnvironmentInfo = (RunEnvironmentInfo) declaredProvider;
if (runEnvironmentInfo.shouldErrorOnNonExecutableRule()) {
context.getRuleContext().ruleError(message);
} else {
context.getRuleContext().ruleWarning(message);
}
}
builder.addStarlarkDeclaredProvider(declaredProvider);
} else {
builder.addStarlarkDeclaredProvider(declaredProvider);
}
}

if (!defaultProviderProvidedExplicitly) {
parseDefaultProviderFields(oldStyleProviders, context, builder);
if (defaultProviderProvidedExplicitly) {
parseDefaultProviderFields(defaultInfo, context, builder, starlarkArgsProvided);
} else {
parseDefaultProviderFields(oldStyleProviders, context, builder, starlarkArgsProvided);
}

for (String field : oldStyleProviders.getFieldNames()) {
Expand Down Expand Up @@ -491,7 +501,10 @@ private static Provider.Key getProviderKey(Info info) throws EvalException {
* throws an {@link EvalException} if there are unknown fields.
*/
private static void parseDefaultProviderFields(
StructImpl info, StarlarkRuleContext context, RuleConfiguredTargetBuilder builder)
StructImpl info,
StarlarkRuleContext context,
RuleConfiguredTargetBuilder builder,
boolean starlarkArgsProvided)
throws EvalException {
Depset files = null;
Runfiles statelessRunfiles = null;
Expand Down Expand Up @@ -591,7 +604,8 @@ private static void parseDefaultProviderFields(
files,
statelessRunfiles,
dataRunfiles,
defaultRunfiles);
defaultRunfiles,
starlarkArgsProvided);
}

private static void addSimpleProviders(
Expand All @@ -601,7 +615,8 @@ private static void addSimpleProviders(
@Nullable Depset files,
Runfiles statelessRunfiles,
Runfiles dataRunfiles,
Runfiles defaultRunfiles)
Runfiles defaultRunfiles,
boolean starlarkArgsProvided)
throws EvalException {

// TODO(bazel-team) if both 'files' and 'executable' are provided, 'files' overrides
Expand Down Expand Up @@ -643,8 +658,13 @@ private static void addSimpleProviders(
RunfilesSupport runfilesSupport = null;
if (!computedDefaultRunfiles.isEmpty()) {
Preconditions.checkNotNull(executable, "executable must not be null");
runfilesSupport =
RunfilesSupport.withExecutable(ruleContext, computedDefaultRunfiles, executable);
if (starlarkArgsProvided) {
runfilesSupport = RunfilesSupport.withExecutableNoArgs(
ruleContext, computedDefaultRunfiles, executable);
} else {
runfilesSupport =
RunfilesSupport.withExecutable(ruleContext, computedDefaultRunfiles, executable);
}
assertExecutableSymlinkPresent(runfilesSupport.getRunfiles(), executable);
}
builder.setRunfilesSupport(runfilesSupport, executable);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.analysis.Allowlist;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
Expand Down Expand Up @@ -98,12 +99,14 @@ public NestedSet<PackageGroupContents> getPackageSpecifications() {
private int explicitShardCount;
private final Map<String, String> extraEnv;
private final Set<String> extraInheritedEnv;
@Nullable private CommandLine starlarkTargetArgs;

public TestActionBuilder(RuleContext ruleContext) {
this.ruleContext = ruleContext;
this.extraEnv = new TreeMap<>();
this.extraInheritedEnv = new TreeSet<>();
this.additionalTools = new ImmutableList.Builder<>();
this.starlarkTargetArgs = null;
}

/**
Expand Down Expand Up @@ -180,6 +183,12 @@ public TestActionBuilder addExtraInheritedEnv(List<String> extraInheritedEnv) {
return this;
}

@CanIgnoreReturnValue
public TestActionBuilder setStarlarkTargetArgs(@Nullable CommandLine starlarkTargetArgs) {
this.starlarkTargetArgs = starlarkTargetArgs;
return this;
}

/** Set the explicit shard count. Note that this may be overridden by the sharding strategy. */
@CanIgnoreReturnValue
public TestActionBuilder setShardCount(int explicitShardCount) {
Expand Down Expand Up @@ -340,6 +349,7 @@ private TestParams createTestAction(int shards)
ruleContext,
runfilesSupport,
executable,
starlarkTargetArgs,
instrumentedFileManifest,
shards,
runsPerTest);
Expand All @@ -352,7 +362,13 @@ private TestParams createTestAction(int shards)
} else {
executionSettings =
new TestTargetExecutionSettings(
ruleContext, runfilesSupport, executable, null, shards, runsPerTest);
ruleContext,
runfilesSupport,
executable,
starlarkTargetArgs,
null,
shards,
runsPerTest);
}

extraTestEnv.putAll(extraEnv);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public final class TestTargetExecutionSettings {
RuleContext ruleContext,
RunfilesSupport runfilesSupport,
Artifact executable,
@Nullable CommandLine starlarkTargetArgs,
Artifact instrumentedFileManifest,
int shards,
int runs)
Expand All @@ -62,7 +63,8 @@ public final class TestTargetExecutionSettings {
BuildConfigurationValue config = ruleContext.getConfiguration();
TestConfiguration testConfig = config.getFragment(TestConfiguration.class);

CommandLine targetArgs = runfilesSupport.getArgs();
CommandLine targetArgs =
starlarkTargetArgs != null ? starlarkTargetArgs : runfilesSupport.getArgs();
testArguments =
CommandLine.concat(targetArgs, ImmutableList.copyOf(testConfig.getTestArguments()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public RunEnvironmentInfo testEnvironment(
Dict.cast(environment, String.class, String.class, "environment"),
StarlarkList.immutableCopyOf(
Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")),
/* arguments= */null,
/* shouldErrorOnNonExecutableRule= */ false);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,19 +176,28 @@ protected BuildResult processRequest(final CommandEnvironment env, BuildRequest
public void editOptions(OptionsParser optionsParser) { }

/**
* Compute the arguments the binary should be run with by concatenating the arguments in its
* {@code args} attribute and the arguments on the Blaze command line.
* Compute the arguments the binary should be run with by concatenating the arguments provided by
* its {@link RunEnvironmentInfo} or, if not set, in its {@code args} attribute, and the arguments
* on the Blaze command line.
*/
@Nullable
private List<String> computeArgs(ConfiguredTarget targetToRun, List<String> commandLineArgs)
throws InterruptedException, CommandLineExpansionException {
List<String> args = Lists.newArrayList();

FilesToRunProvider provider = targetToRun.getProvider(FilesToRunProvider.class);
RunfilesSupport runfilesSupport = provider == null ? null : provider.getRunfilesSupport();
if (runfilesSupport != null && runfilesSupport.getArgs() != null) {
CommandLine targetArgs = runfilesSupport.getArgs();
Iterables.addAll(args, targetArgs.arguments());
// If a Starlark rule explicitly sets arguments on RunEnvironmentInfo, these arguments take
// precedence over those obtained from the implicit "args" attribute on all rules so that
// Starlark rules can implement their own logic for this attribute.
RunEnvironmentInfo runEnvironmentInfo =
(RunEnvironmentInfo) targetToRun.get(RunEnvironmentInfo.PROVIDER.getKey());
if (runEnvironmentInfo != null && runEnvironmentInfo.getArguments() != null) {
Iterables.addAll(args, runEnvironmentInfo.getArguments().arguments());
} else {
FilesToRunProvider provider = targetToRun.getProvider(FilesToRunProvider.class);
RunfilesSupport runfilesSupport = provider == null ? null : provider.getRunfilesSupport();
if (runfilesSupport != null && runfilesSupport.getArgs() != null) {
CommandLine targetArgs = runfilesSupport.getArgs();
Iterables.addAll(args, targetArgs.arguments());
}
}
args.addAll(commandLineArgs);
return args;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.starlarkbuildapi;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.docgen.annot.StarlarkConstructor;
import com.google.devtools.build.lib.starlarkbuildapi.core.ProviderApi;
Expand All @@ -26,11 +27,12 @@
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.NoneType;
import net.starlark.java.eval.Sequence;

/**
* Provider containing any additional environment variables for use when running executables, either
* in test actions or when executed via the run command.
* Provider containing any additional environment variables and arguments for use when running
* executables, either in test actions or when executed via the run command.
*/
@StarlarkBuiltin(
name = "RunEnvironmentInfo",
Expand Down Expand Up @@ -78,7 +80,9 @@ interface RunEnvironmentInfoApiProvider extends ProviderApi {
doc =
"A map of string keys and values that represent environment variables and their"
+ " values. These will be made available when the target that returns this"
+ " provider is executed, either as a test or via the run command."),
+ " provider is executed, either as a test or via the run command. In the"
+ " case of a test, environment variables specified on the command line via"
+ " <code>--test_env</code> take precedence over values specified here."),
@Param(
name = "inherited_environment",
allowedTypes = {@ParamType(type = Sequence.class, generic1 = String.class)},
Expand All @@ -91,13 +95,29 @@ interface RunEnvironmentInfoApiProvider extends ProviderApi {
+ " when the target that returns this provider is executed, either as a"
+ " test or via the run command. If a variable is contained in both <code>"
+ "environment</code> and <code>inherited_environment</code>, the value"
+ " inherited from the shell environment will take precedence if set.")
+ " inherited from the shell environment will take precedence if set."),
@Param(
name = "arguments",
allowedTypes = {
@ParamType(type = Sequence.class, generic1 = String.class),
@ParamType(type = NoneType.class)
},
defaultValue = "None",
named = true,
positional = false,
doc = "A list of arguments. If set to a value other than <code>None</code>, this"
+ " list will be appended to the command line when the target that returns"
+ " this provider is executed, either as a test or via the run command. In"
+ " this case, the arguments specified in the <code>args</code> attribute"
+ " implicitly defined for all rules are <strong>not</code> appended"
+ " automatically.")
},
selfCall = true)
@StarlarkConstructor
RunEnvironmentInfoApi constructor(
Dict<?, ?> environment, // <String, String> expected
Sequence<?> inheritedEnvironment /* <String> expected */)
Sequence<?> inheritedEnvironment, /* <String> expected */
Object arguments /* Sequence<String> or NoneType expected */)
throws EvalException;
}
}
Loading

0 comments on commit d747d2e

Please sign in to comment.