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.

Sign up for GitHub

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.2.0] Add NestedSet<PathFragment>- and Artifact-typed compile build var… #22557

Merged
merged 1 commit into from
May 28, 2024
Merged

[7.2.0] Add NestedSet<PathFragment>- and Artifact-typed compile build var… #22557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 7 additions & 23 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,14 +1271,6 @@ private CcToolchainVariables setupCompileBuildVariables(
ImmutableMap<String, String> additionalBuildVariables)
throws RuleErrorException, InterruptedException {
Artifact sourceFile = builder.getSourceFile();
String dotdFileExecPath = null;
if (builder.getDotdFile() != null) {
dotdFileExecPath = builder.getDotdFile().getExecPathString();
}
String diagnosticsFileExecPath = null;
if (builder.getDiagnosticsFile() != null) {
diagnosticsFileExecPath = builder.getDiagnosticsFile().getExecPathString();
}
if (needsFdoBuildVariables && fdoContext.hasArtifacts(cppConfiguration)) {
// This modifies the passed-in builder, which is a surprising side-effect, and makes it unsafe
// to call this method multiple times for the same builder.
Expand Down Expand Up @@ -1348,30 +1340,22 @@ private CcToolchainVariables setupCompileBuildVariables(

CompileBuildVariables.setupSpecificVariables(
buildVariables,
toPathString(sourceFile),
toPathString(builder.getOutputFile()),
sourceFile,
builder.getOutputFile(),
enableCoverage,
toPathString(gcnoFile),
toPathString(dwoFile),
gcnoFile,
dwoFile,
isUsingFission,
toPathString(ltoIndexingFile),
/* thinLtoIndex= */ null,
/* thinLtoInputBitcodeFile= */ null,
/* thinLtoOutputObjectFile= */ null,
ltoIndexingFile,
getCopts(builder.getSourceFile(), sourceLabel),
dotdFileExecPath,
diagnosticsFileExecPath,
builder.getDotdFile(),
builder.getDiagnosticsFile(),
usePic,
ccCompilationContext.getExternalIncludeDirs(),
additionalBuildVariables);
return buildVariables.build();
}

@Nullable
private static String toPathString(Artifact a) {
return a == null ? null : a.getExecPathString();
}

/**
* Returns a {@code CppCompileActionBuilder} with the common fields for a C++ compile action being
* initialized.
Expand Down
170 changes: 157 additions & 13 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ExpansionException;
import com.google.devtools.build.lib.starlarkbuildapi.cpp.CcToolchainVariablesApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -1000,7 +1001,8 @@ private StringSequence(Iterable<String> values) {

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(values.size());
for (String value : values) {
sequences.add(new StringValue(value));
}
Expand Down Expand Up @@ -1059,8 +1061,10 @@ private StringSetSequence(NestedSet<String> values) {

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
for (String value : values.toList()) {
ImmutableList<String> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (String value : valuesList) {
sequences.add(new StringValue(value));
}
return sequences.build();
Expand Down Expand Up @@ -1093,6 +1097,53 @@ public int hashCode() {
}
}

@Immutable
private static final class PathFragmentSetSequence extends VariableValueAdapter {
private final NestedSet<PathFragment> values;

private PathFragmentSetSequence(NestedSet<PathFragment> values) {
Preconditions.checkNotNull(values);
this.values = values;
}

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList<PathFragment> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (PathFragment value : valuesList) {
sequences.add(new StringValue(value.getSafePathString()));
}
return sequences.build();
}

@Override
public String getVariableTypeName() {
return Sequence.SEQUENCE_VARIABLE_TYPE_NAME;
}

@Override
public boolean isTruthy() {
return !values.isEmpty();
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (!(other instanceof PathFragmentSetSequence otherPathFragments)) {
return false;
}
return values.shallowEquals(otherPathFragments.values);
}

@Override
public int hashCode() {
return values.shallowHashCode();
}
}

/**
* Single structure value. Be careful not to create sequences of single structures, as the memory
* overhead is prohibitively big.
Expand Down Expand Up @@ -1145,7 +1196,7 @@ public int hashCode() {
}

/**
* The leaves in the variable sequence node tree are simple string values. Note that this should
* Most leaves in the variable sequence node tree are simple string values. Note that this should
* never live outside of {@code expand}, as the object overhead is prohibitively expensive.
*/
@Immutable
Expand Down Expand Up @@ -1221,6 +1272,53 @@ public boolean isTruthy() {
}
}

/**
* Represents leaves in the variable sequence node tree that are paths of artifacts. Note that
* this should never live outside of {@code expand}, as the object overhead is prohibitively
* expensive.
*/
@Immutable
private static final class ArtifactValue extends VariableValueAdapter {
private static final String ARTIFACT_VARIABLE_TYPE_NAME = "artifact";

private final Artifact value;

ArtifactValue(Artifact value) {
this.value = value;
}

@Override
public String getStringValue(String variableName) {
return value.getExecPathString();
}

@Override
public String getVariableTypeName() {
return ARTIFACT_VARIABLE_TYPE_NAME;
}

@Override
public boolean isTruthy() {
return true;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (!(other instanceof ArtifactValue otherValue)) {
return false;
}
return value.equals(otherValue.value);
}

@Override
public int hashCode() {
return value.hashCode();
}
}

public static Builder builder() {
return new Builder(null);
}
Expand Down Expand Up @@ -1256,6 +1354,34 @@ public Builder addStringVariable(String name, String value) {
return this;
}

/** Add an artifact variable that expands {@code name} to {@code value}. */
@CanIgnoreReturnValue
public Builder addArtifactVariable(String name, Artifact value) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(value, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, value);
return this;
}

/**
* Add an artifact or string variable that expands {@code name} to {@code value}.
*
* <p>Prefer {@link #addArtifactVariable} and {@link #addStringVariable}. This method is only
* meant to support string-based Starlark API.
*/
@CanIgnoreReturnValue
public Builder addArtifactOrStringVariable(String name, Object value) {
return switch (value) {
case String s -> addStringVariable(name, s);
case Artifact artifact -> addArtifactVariable(name, artifact);
case null ->
throw new IllegalArgumentException(
"Cannot set null as a value for variable '" + name + "'");
default ->
throw new IllegalArgumentException("Unsupported value type: " + value.getClass());
};
}

/** Overrides a variable to expands {@code name} to {@code value} instead. */
@CanIgnoreReturnValue
public Builder overrideStringVariable(String name, String value) {
Expand Down Expand Up @@ -1309,6 +1435,19 @@ public Builder addStringSequenceVariable(String name, Iterable<String> values) {
return this;
}

/**
* Add a sequence variable that expands {@code name} to {@link PathFragment} {@code values}.
*
* <p>Accepts values as NestedSet. Nested set is stored directly, not cloned, not flattened.
*/
@CanIgnoreReturnValue
public Builder addPathFragmentSequenceVariable(String name, NestedSet<PathFragment> values) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(values, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, new PathFragmentSetSequence(values));
return this;
}

/**
* Add a variable built using {@code VariableValueBuilder} api that expands {@code name} to the
* value returned by the {@code builder}.
Expand Down Expand Up @@ -1356,15 +1495,24 @@ public Builder addAllNonTransitive(CcToolchainVariables variables) {
/** @return a new {@link CcToolchainVariables} object. */
public CcToolchainVariables build() {
if (variablesMap.size() == 1) {
Object o = variablesMap.values().iterator().next();
VariableValue variableValue =
o instanceof String ? new StringValue((String) o) : (VariableValue) o;
return new SingleVariables(parent, variablesMap.keySet().iterator().next(), variableValue);
return new SingleVariables(
parent,
variablesMap.keySet().iterator().next(),
asVariableValue(variablesMap.values().iterator().next()));
}
return new MapVariables(parent, variablesMap);
}
}

/** Wraps a raw variablesMap value into an appropriate VariableValue if necessary. */
private static VariableValue asVariableValue(Object o) {
return switch (o) {
case String s -> new StringValue(s);
case Artifact artifact -> new ArtifactValue(artifact);
default -> (VariableValue) o;
};
}

/**
* A group of extra {@code Variable} instances, packaged as logic for adding to a {@code Builder}
*/
Expand Down Expand Up @@ -1424,11 +1572,7 @@ void addVariablesToMap(Map<String, Object> variablesMap) {
@Override
VariableValue getNonStructuredVariable(String name) {
if (keyToIndex.containsKey(name)) {
Object o = values.get(keyToIndex.get(name));
if (o instanceof String) {
return new StringValue((String) o);
}
return (VariableValue) o;
return CcToolchainVariables.asVariableValue(values.get(keyToIndex.get(name)));
}

if (parent != null) {
Expand Down
Loading
Loading