Skip to content

Commit

Permalink
Fix genrule autostamping in bazel
Browse files Browse the repository at this point in the history
Genrule does not support conditional stamping based on the value of the `--stamp` flag. This is because genrule treats the `stamp` attribute as `boolean`, while all other rules use the `tristate` type. After this change, the stamp attribute in genrule can be set to `-1` to request conditional stamping.

RELNOTES: `genrule` now supports setting `stamp = -1` to request conditional stamping (based on the value of the build's `--stamp` flag).

Closes #21407.

PiperOrigin-RevId: 609085052
Change-Id: I873941e9aaae3760a545c1900cd13cb60a9ada39
  • Loading branch information
AlessandroPatti authored and copybara-github committed Feb 21, 2024
1 parent f8337c7 commit ee57d99
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,15 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.AliasProvider;
import com.google.devtools.build.lib.analysis.FileProvider;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.rules.genrule.GenRuleBase;
import java.util.List;

/** An implementation of genrule for Bazel. */
public final class BazelGenRule extends GenRuleBase {

@Override
protected boolean isStampingEnabled(RuleContext ruleContext) {
if (!ruleContext.attributes().has("stamp", Type.BOOLEAN)) {
return false;
}
return ruleContext.attributes().get("stamp", Type.BOOLEAN);
}

@Override
protected ImmutableMap<Label, NestedSet<Artifact>> collectSources(
List<? extends TransitiveInfoCollection> srcs) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,20 @@

import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
import static com.google.devtools.build.lib.packages.BuildType.TRISTATE;

import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.TriState;
import com.google.devtools.build.lib.rules.genrule.GenRuleBaseRule;

/**
* Rule definition for genrule for Bazel.
*/
public final class BazelGenRuleRule implements RuleDefinition {
public static final String GENRULE_SETUP_LABEL = "//tools/genrule:genrule-setup.sh";
private static final String GENRULE_SETUP_LABEL = "//tools/genrule:genrule-setup.sh";

@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
Expand All @@ -43,9 +44,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
attr("$genrule_setup", LABEL)
.cfg(ExecutionTransitionFactory.createFactory())
.value(env.getToolsLabel(GENRULE_SETUP_LABEL)))

// TODO(bazel-team): stamping doesn't seem to work. Fix it or remove attribute.
.add(attr("stamp", BOOLEAN).value(false))
.add(attr("stamp", TRISTATE).value(TriState.NO))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.packages.TriState;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.util.FileTypeSet;
import com.google.devtools.build.lib.util.OnDemandString;
Expand All @@ -56,7 +58,7 @@

/**
* A base implementation of genrule, to be used by specific implementing rules which can change the
* semantics of {@link #isStampingEnabled} and {@link #collectSources}.
* semantics of {@link #collectSources}.
*/
public abstract class GenRuleBase implements RuleConfiguredTargetFactory {

Expand Down Expand Up @@ -227,20 +229,20 @@ public String toString() {
.build();
}

/**
* Returns {@code true} if the rule should be stamped.
*
* <p>Genrule implementations can set this based on the rule context, including by defining their
* own attributes over and above what is present in {@link GenRuleBaseRule}.
*/
@ForOverride
protected abstract boolean isStampingEnabled(RuleContext ruleContext);

/** Collects sources from src attribute. */
@ForOverride
protected abstract ImmutableMap<Label, NestedSet<Artifact>> collectSources(
List<? extends TransitiveInfoCollection> srcs) throws RuleErrorException;

private static boolean isStampingEnabled(RuleContext ruleContext) {
// This intentionally does not call AnalysisUtils.isStampingEnabled(). That method returns false
// in the exec configuration (regardless of the attribute value), which is the behavior for
// binaries, but not genrules.
TriState stamp = ruleContext.attributes().get("stamp", BuildType.TRISTATE);
return stamp == TriState.YES
|| (stamp == TriState.AUTO && ruleContext.getConfiguration().stampBinaries());
}

private enum CommandType {
BASH,
WINDOWS_BATCH,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ private void createStampingTargets() throws Exception {
"u/BUILD",
"genrule(name='foo_stamp', srcs=[], outs=['uu'], stamp=1, cmd='')",
"genrule(name='foo_nostamp', srcs=[], outs=['vv'], stamp=0, cmd='')",
"genrule(name='foo_autostamp', srcs=[], outs=['aa'], stamp=-1, cmd='')",
"genrule(name='foo_default', srcs=[], outs=['xx'], cmd='')");
}

Expand Down Expand Up @@ -562,6 +563,8 @@ public void testStampingWithNoStamp() throws Exception {
assertStamped(getExecConfiguredTarget("//u:foo_stamp"));
assertNotStamped("//u:foo_nostamp");
assertNotStamped(getExecConfiguredTarget("//u:foo_nostamp"));
assertNotStamped("//u:foo_autostamp");
assertNotStamped(getExecConfiguredTarget("//u:foo_autostamp"));
assertNotStamped("//u:foo_default");
}

Expand All @@ -571,8 +574,10 @@ public void testStampingWithStamp() throws Exception {
createStampingTargets();
assertStamped("//u:foo_stamp");
assertStamped(getExecConfiguredTarget("//u:foo_stamp"));
// assertStamped("//u:foo_nostamp");
assertNotStamped("//u:foo_nostamp");
assertNotStamped(getExecConfiguredTarget("//u:foo_nostamp"));
assertStamped("//u:foo_autostamp");
assertNotStamped(getExecConfiguredTarget("//u:foo_autostamp"));
assertNotStamped("//u:foo_default");
}

Expand Down

0 comments on commit ee57d99

Please sign in to comment.