Skip to content

Commit

Permalink
Automated rollback of commit 18bd1056b00e1fd58638d4db81e02cfb7df8ce69.
Browse files Browse the repository at this point in the history
    *** Reason for rollback ***

    This CL has introduced a bug where using a starlark transition that outputs user-defined build configs can cause a build error. Details are still being investigated. - b/152078818

    *** Original change description ***

    Allows using split_attr on Starlark split transitions.

    RELNOTES: ctx.split_attr now includes attributes with Starlark split transitions.
    PiperOrigin-RevId: 302525924
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 1854dfe commit 6b54458
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 191 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

package com.google.devtools.build.lib.analysis.skylark;

import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
import static com.google.devtools.build.lib.packages.RuleClass.Builder.SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;

import com.google.common.base.Function;
import com.google.common.base.Optional;
Expand Down Expand Up @@ -44,17 +44,18 @@
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.TransitionMode;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.FragmentCollection;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.AspectDescriptor;
import com.google.devtools.build.lib.packages.Attribute;
Expand All @@ -80,7 +81,6 @@
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.Location;
import com.google.devtools.build.lib.syntax.NoneType;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Sequence;
Expand All @@ -102,7 +102,7 @@
import javax.annotation.Nullable;

/**
* A Starlark API for the ruleContext.
* A Skylark API for the ruleContext.
*
* <p>"This object becomes featureless once the rule implementation function that it was created for
* has completed. To achieve this, the {@link #nullify()} should be called once the evaluation of
Expand Down Expand Up @@ -377,7 +377,7 @@ private void checkMutable() throws EvalException {
}

public boolean isExecutable() {
return ruleContext.getRule().getRuleClassObject().isExecutableStarlark();
return ruleContext.getRule().getRuleClassObject().isExecutableSkylark();
}

public boolean isDefaultExecutableCreated() {
Expand Down Expand Up @@ -451,7 +451,7 @@ private static StructImpl buildSplitAttributeInfo(
// If the split transition is not in effect, then the key will be missing since there's
// nothing to key on because the dependencies aren't split and getSplitPrerequisites()
// behaves like getPrerequisites(). This also means there should be only one entry in
// the map. Use None in Starlark to represent this.
// the map. Use None in Skylark to represent this.
Preconditions.checkState(splitPrereqs.size() == 1);
splitPrereqsMap.put(Starlark.NONE, value);
}
Expand Down Expand Up @@ -501,7 +501,7 @@ public SkylarkActionFactory actions() {
@Override
public StarlarkValue createdActions() throws EvalException {
checkMutable("created_actions");
if (ruleContext.getRule().getRuleClassObject().isStarlarkTestable()) {
if (ruleContext.getRule().getRuleClassObject().isSkylarkTestable()) {
return ActionsProvider.create(
ruleContext.getAnalysisEnvironment().getRegisteredActions());
} else {
Expand All @@ -525,21 +525,21 @@ public StructImpl getSplitAttr() throws EvalException {
return splitAttributes;
}

/** See {@link RuleContext#getExecutablePrerequisite(String, TransitionMode)}. */
/** See {@link RuleContext#getExecutablePrerequisite(String, Mode)}. */
@Override
public StructImpl getExecutable() throws EvalException {
checkMutable("executable");
return attributesCollection.getExecutable();
}

/** See {@link RuleContext#getPrerequisiteArtifact(String, TransitionMode)}. */
/** See {@link RuleContext#getPrerequisiteArtifact(String, Mode)}. */
@Override
public StructImpl getFile() throws EvalException {
checkMutable("file");
return attributesCollection.getFile();
}

/** See {@link RuleContext#getPrerequisiteArtifacts(String, TransitionMode)}. */
/** See {@link RuleContext#getPrerequisiteArtifacts(String, Mode)}. */
@Override
public StructImpl getFiles() throws EvalException {
checkMutable("files");
Expand Down Expand Up @@ -602,7 +602,7 @@ public Object getBuildSettingValue() throws EvalException {
} else {
return ruleContext
.attributes()
.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, buildSettingType);
.get(SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, buildSettingType);
}
}

Expand Down Expand Up @@ -723,7 +723,7 @@ public String expand(
checkMutable("expand");
try {
Map<Label, Iterable<Artifact>> labelMap = new HashMap<>();
for (Artifact artifact : Sequence.cast(artifacts, Artifact.class, "artifacts")) {
for (Artifact artifact : artifacts.getContents(Artifact.class, "artifacts")) {
labelMap.put(artifactsLabelMap.get(artifact), ImmutableList.of(artifact));
}
return LabelExpander.expand(expression, labelMap, labelResolver);
Expand Down Expand Up @@ -802,8 +802,7 @@ public boolean checkPlaceholders(String template, Sequence<?> allowedPlaceholder
checkMutable("check_placeholders");
List<String> actualPlaceHolders = new LinkedList<>();
Set<String> allowedPlaceholderSet =
ImmutableSet.copyOf(
Sequence.cast(allowedPlaceholders, String.class, "allowed_placeholders"));
ImmutableSet.copyOf(allowedPlaceholders.getContents(String.class, "allowed_placeholders"));
ImplicitOutputsFunction.createPlaceholderSubstitutionFormatString(template, actualPlaceHolders);
for (String placeholder : actualPlaceHolders) {
if (!allowedPlaceholderSet.contains(placeholder)) {
Expand All @@ -819,7 +818,7 @@ public String expandMakeVariables(
throws EvalException {
checkMutable("expand_make_variables");
final Map<String, String> additionalSubstitutionsMap =
Dict.cast(additionalSubstitutions, String.class, String.class, "additional_substitutions");
additionalSubstitutions.getContents(String.class, String.class, "additional_substitutions");
return expandMakeVariables(attributeName, command, additionalSubstitutionsMap);
}

Expand Down Expand Up @@ -863,11 +862,11 @@ public Artifact getVolatileWorkspaceStatus() throws InterruptedException, EvalEx
public String getBuildFileRelativePath() throws EvalException {
checkMutable("build_file_path");
Package pkg = ruleContext.getRule().getPackage();
return pkg.getSourceRoot().get().relativize(pkg.getBuildFile().getPath()).getPathString();
return pkg.getSourceRoot().relativize(pkg.getBuildFile().getPath()).getPathString();
}

/**
* A Starlark built-in function to create and register a SpawnAction using a dictionary of
* A Skylark built-in function to create and register a SpawnAction using a dictionary of
* parameters: action( inputs = [input1, input2, ...], outputs = [output1, output2, ...],
* executable = executable, arguments = [argument1, argument2, ...], mnemonic = 'Mnemonic',
* command = 'command', )
Expand Down Expand Up @@ -937,7 +936,7 @@ public String expandLocation(String input, Sequence<?> targets, StarlarkThread t
try {
return LocationExpander.withExecPaths(
getRuleContext(),
makeLabelMap(Sequence.cast(targets, TransitiveInfoCollection.class, "targets")))
makeLabelMap(targets.getContents(TransitiveInfoCollection.class, "targets")))
.expand(input);
} catch (IllegalStateException ise) {
throw new EvalException(null, ise);
Expand Down Expand Up @@ -998,24 +997,24 @@ public Runfiles runfiles(
builder.addRunfiles(getRuleContext(), RunfilesProvider.DEFAULT_RUNFILES);
}
if (!files.isEmpty()) {
builder.addArtifacts(Sequence.cast(files, Artifact.class, "files"));
builder.addArtifacts(files.getContents(Artifact.class, "files"));
}
if (transitiveFiles != Starlark.NONE) {
builder.addTransitiveArtifacts(
((Depset) transitiveFiles).getSetFromParam(Artifact.class, "transitive_files"));
}
if (!symlinks.isEmpty()) {
// If Starlark code directly manipulates symlinks, activate more stringent validity checking.
// If Skylark code directly manipulates symlinks, activate more stringent validity checking.
checkConflicts = true;
for (Map.Entry<String, Artifact> entry :
Dict.cast(symlinks, String.class, Artifact.class, "symlinks").entrySet()) {
symlinks.getContents(String.class, Artifact.class, "symlinks").entrySet()) {
builder.addSymlink(PathFragment.create(entry.getKey()), entry.getValue());
}
}
if (!rootSymlinks.isEmpty()) {
checkConflicts = true;
for (Map.Entry<String, Artifact> entry :
Dict.cast(rootSymlinks, String.class, Artifact.class, "root_symlinks").entrySet()) {
rootSymlinks.getContents(String.class, Artifact.class, "root_symlinks").entrySet()) {
builder.addRootSymlink(PathFragment.create(entry.getKey()), entry.getValue());
}
}
Expand All @@ -1040,10 +1039,10 @@ public Tuple<Object> resolveCommand(
checkMutable("resolve_command");
Label ruleLabel = getLabel();
Map<Label, Iterable<Artifact>> labelDict = checkLabelDict(labelDictUnchecked);
// The best way to fix this probably is to convert CommandHelper to Starlark.
// The best way to fix this probably is to convert CommandHelper to Skylark.
CommandHelper helper =
CommandHelper.builder(getRuleContext())
.addToolDependencies(Sequence.cast(tools, TransitiveInfoCollection.class, "tools"))
.addToolDependencies(tools.getContents(TransitiveInfoCollection.class, "tools"))
.addLabelMap(labelDict)
.build();
String attribute = Type.STRING.convertOptional(attributeUnchecked, "attribute", ruleLabel);
Expand All @@ -1059,12 +1058,12 @@ public Tuple<Object> resolveCommand(
List<Artifact> inputs = new ArrayList<>();
// TODO(lberki): This flattens a NestedSet.
// However, we can't turn this into a Depset because it's an incompatible change to
// Starlark.
// Skylark.
inputs.addAll(helper.getResolvedTools().toList());

ImmutableMap<String, String> executionRequirements =
ImmutableMap.copyOf(
Dict.noneableCast(
Dict.castSkylarkDictOrNoneToDict(
executionRequirementsUnchecked,
String.class,
String.class,
Expand All @@ -1090,7 +1089,7 @@ public Tuple<Object> resolveTools(Sequence<?> tools) throws EvalException {
checkMutable("resolve_tools");
CommandHelper helper =
CommandHelper.builder(getRuleContext())
.addToolDependencies(Sequence.cast(tools, TransitiveInfoCollection.class, "tools"))
.addToolDependencies(tools.getContents(TransitiveInfoCollection.class, "tools"))
.build();
return Tuple.<Object>of(
Depset.of(Artifact.TYPE, helper.getResolvedTools()), helper.getToolsRunfilesSuppliers());
Expand Down
Loading

0 comments on commit 6b54458

Please sign in to comment.