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

[7.1.0] Return labels instead of strings from DescribableExecutionUnit methods. #20788

Merged
merged 1 commit into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ public void maybeReportSubcommand(Spawn spawn) {
/* environmentVariablesToClear= */ null,
getExecRoot().getPathString(),
spawn.getConfigurationChecksum(),
spawn.getExecutionPlatformLabelString());
spawn.getExecutionPlatformLabel());
getEventHandler().handle(Event.of(EventKind.SUBCOMMAND, null, "# " + reason + "\n" + message));
}

Expand Down
10 changes: 4 additions & 6 deletions src/main/java/com/google/devtools/build/lib/actions/Spawn.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.util.DescribableExecutionUnit;
import java.util.Collection;
import java.util.Objects;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -171,9 +170,9 @@ default boolean isMandatoryOutput(ActionInput output) {

@Override
@Nullable
default String getExecutionPlatformLabelString() {
default Label getExecutionPlatformLabel() {
PlatformInfo executionPlatform = getExecutionPlatform();
return executionPlatform == null ? null : Objects.toString(executionPlatform.label());
return executionPlatform != null ? executionPlatform.label() : null;
}

@Override
Expand All @@ -184,9 +183,8 @@ default String getConfigurationChecksum() {

@Override
@Nullable
default String getTargetLabel() {
Label label = getResourceOwner().getOwner().getLabel();
return label == null ? null : label.toString();
default Label getTargetLabel() {
return getResourceOwner().getOwner().getLabel();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public void logSpawn(
builder.setMnemonic(spawn.getMnemonic());

if (spawn.getTargetLabel() != null) {
builder.setTargetLabel(spawn.getTargetLabel());
builder.setTargetLabel(spawn.getTargetLabel().toString());
}

SpawnMetrics metrics = result.getMetrics();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import java.util.Base64;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;
import net.starlark.java.eval.EvalException;

Expand Down Expand Up @@ -260,9 +259,9 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream)
/* environmentVariablesToClear= */ null,
/* cwd= */ null,
action.getOwner().getConfigurationChecksum(),
action.getExecutionPlatform() == null
? null
: Objects.toString(action.getExecutionPlatform().label())))
action.getExecutionPlatform() != null
? action.getExecutionPlatform().label()
: null))
.append("\n");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
ENV_VARIABLES_TO_CLEAR,
runCommandLine.workingDir.getPathString(),
builtTargets.configuration.checksum(),
/* executionPlatformAsLabelString= */ null);
/* executionPlatformLabel= */ null);

PathFragment shExecutable = ShToolchain.getPathForHost(builtTargets.configuration);
if (shExecutable.isEmpty()) {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/util/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ java_library(
name = "describable_execution_unit",
srcs = ["DescribableExecutionUnit.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//third_party:guava",
"//third_party:jsr305",
],
Expand All @@ -100,6 +101,7 @@ java_library(
":describable_execution_unit",
":os",
":shell_escaper",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/shell",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//third_party:error_prone_annotations",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Ordering;
import com.google.devtools.build.lib.cmdline.Label;
import java.io.File;
import java.util.Collection;
import java.util.Comparator;
Expand Down Expand Up @@ -173,7 +174,7 @@ public static String describeCommand(
@Nullable List<String> environmentVariablesToClear,
@Nullable String cwd,
@Nullable String configurationChecksum,
@Nullable String executionPlatformAsLabelString) {
@Nullable Label executionPlatformLabel) {

Preconditions.checkNotNull(form);
StringBuilder message = new StringBuilder();
Expand Down Expand Up @@ -267,9 +268,9 @@ public static String describeCommand(
message.append("# Configuration: ").append(configurationChecksum);
}

if (executionPlatformAsLabelString != null) {
if (executionPlatformLabel != null) {
message.append("\n");
message.append("# Execution platform: ").append(executionPlatformAsLabelString);
message.append("# Execution platform: ").append(executionPlatformLabel);
}
}

Expand All @@ -288,8 +289,8 @@ static String describeCommandFailure(
Map<String, String> env,
@Nullable String cwd,
@Nullable String configurationChecksum,
@Nullable String targetLabel,
@Nullable String executionPlatformAsLabelString) {
@Nullable Label targetLabel,
@Nullable Label executionPlatformLabel) {

String commandName = commandLineElements.iterator().next();
// Extract the part of the command name after the last "/", if any.
Expand Down Expand Up @@ -318,7 +319,7 @@ static String describeCommandFailure(
null,
cwd,
configurationChecksum,
executionPlatformAsLabelString));
executionPlatformLabel));
return shortCommandName + " failed: " + output;
}

Expand All @@ -332,6 +333,6 @@ public static String describeCommandFailure(
cwd,
command.getConfigurationChecksum(),
command.getTargetLabel(),
command.getExecutionPlatformLabelString());
command.getExecutionPlatformLabel());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import javax.annotation.Nullable;

/**
Expand All @@ -23,7 +24,7 @@
public interface DescribableExecutionUnit {

@Nullable
default String getTargetLabel() {
default Label getTargetLabel() {
return null;
}

Expand All @@ -38,7 +39,7 @@ default String getTargetLabel() {

/** Returns the Label of the execution platform for the command, if any, as a String. */
@Nullable
default String getExecutionPlatformLabelString() {
default Label getExecutionPlatformLabel() {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,6 @@ public String toString() {
/* environmentVariablesToClear= */ null,
execRoot.getPathString(),
/* configurationChecksum= */ null,
/* executionPlatformAsLabelString= */ null);
/* executionPlatformLabel= */ null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class CommandFailureUtilsTest {

@Test
public void describeCommandFailure() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[3];
args[0] = "/bin/sh";
args[1] = "-c";
Expand All @@ -51,7 +51,7 @@ public void describeCommandFailure() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"sh failed: error executing Mnemonic command (from target //foo:bar) "
Expand All @@ -60,7 +60,7 @@ public void describeCommandFailure() throws Exception {

@Test
public void describeCommandFailure_verbose() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[3];
args[0] = "/bin/sh";
args[1] = "-c";
Expand All @@ -80,7 +80,7 @@ public void describeCommandFailure_verbose() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"sh failed: error executing Mnemonic command (from target //foo:bar) \n"
Expand All @@ -94,7 +94,7 @@ public void describeCommandFailure_verbose() throws Exception {

@Test
public void describeCommandFailure_longMessage() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[40];
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
Expand All @@ -117,7 +117,7 @@ public void describeCommandFailure_longMessage() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"some_command failed: error executing Mnemonic command (from target //foo:bar) "
Expand All @@ -130,7 +130,7 @@ public void describeCommandFailure_longMessage() throws Exception {

@Test
public void describeCommandFailure_longMessage_verbose() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[40];
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
Expand All @@ -153,7 +153,7 @@ public void describeCommandFailure_longMessage_verbose() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"some_command failed: error executing Mnemonic command (from target //foo:bar) \n"
Expand All @@ -172,7 +172,7 @@ public void describeCommandFailure_longMessage_verbose() throws Exception {

@Test
public void describeCommandFailure_singleSkippedArgument() throws Exception {
String target = "//foo:bar";
Label target = Label.parseCanonicalUnchecked("//foo:bar");
String[] args = new String[35]; // Long enough to make us skip 1 argument below.
args[0] = "some_command";
for (int i = 1; i < args.length; i++) {
Expand All @@ -191,7 +191,7 @@ public void describeCommandFailure_singleSkippedArgument() throws Exception {
cwd,
"cfg12345",
target,
executionPlatform.label().toString());
executionPlatform.label());
assertThat(message)
.isEqualTo(
"some_command failed: error executing Mnemonic command (from target //foo:bar)"
Expand Down Expand Up @@ -230,7 +230,7 @@ public void describeCommandPrettyPrintArgs() throws Exception {
envToClear,
cwd,
"cfg12345",
executionPlatform.label().toString());
executionPlatform.label());

assertThat(message)
.isEqualTo(
Expand Down
Loading