From 524c49c2096232285f2c9a3b34108bf5f10a36cc Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 21 Feb 2024 08:12:58 -0800 Subject: [PATCH 1/2] Clean up `GenRuleBase` in preparation for merging https://github.com/bazelbuild/bazel/pull/21407. Isolate two `@ForOverride` methods and make everything else `private` to make it clear what behavior may diverge between bazel and blaze. PiperOrigin-RevId: 608999251 Change-Id: I16343d4a1b538d9c13544661ec421f13bf9b1c40 --- .../build/lib/bazel/rules/genrule/BUILD | 7 +- .../lib/bazel/rules/genrule/BazelGenRule.java | 28 +++- .../build/lib/rules/genrule/GenRuleBase.java | 129 ++++++++---------- 3 files changed, 86 insertions(+), 78 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD index 2b791a509d5363..ee47f9a708f6f6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD @@ -15,11 +15,16 @@ java_library( name = "genrule", srcs = glob(["*.java"]), deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory", + "//src/main/java/com/google/devtools/build/lib/analysis:file_provider", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", + "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/rules/genrule", - "//src/main/java/com/google/devtools/build/lib/util:filetype", + "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java index 967e8fd7801101..97edfaae574cca 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java @@ -14,14 +14,20 @@ package com.google.devtools.build.lib.bazel.rules.genrule; +import com.google.common.collect.ImmutableMap; +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 class BazelGenRule extends GenRuleBase { +/** An implementation of genrule for Bazel. */ +public final class BazelGenRule extends GenRuleBase { @Override protected boolean isStampingEnabled(RuleContext ruleContext) { @@ -30,4 +36,18 @@ protected boolean isStampingEnabled(RuleContext ruleContext) { } return ruleContext.attributes().get("stamp", Type.BOOLEAN); } + + @Override + protected ImmutableMap> collectSources( + List srcs) { + ImmutableMap.Builder> labelMap = + ImmutableMap.builderWithExpectedSize(srcs.size()); + + for (TransitiveInfoCollection dep : srcs) { + NestedSet files = dep.getProvider(FileProvider.class).getFilesToBuild(); + labelMap.put(AliasProvider.getDependencyLabel(dep), files); + } + + return labelMap.buildOrThrow(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java index e102b7721b8f1a..51a4c116b17937 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.actions.CommandLines; import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; -import com.google.devtools.build.lib.analysis.AliasProvider; import com.google.devtools.build.lib.analysis.CommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext; @@ -52,71 +51,20 @@ import com.google.devtools.build.lib.util.OnDemandString; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.errorprone.annotations.ForOverride; import java.util.List; import java.util.Map; import javax.annotation.Nullable; /** - * A base implementation of genrule, to be used by specific implementing rules which can change some - * of the semantics around when the execution info and inputs are changed. + * A base implementation of genrule, to be used by specific implementing rules which can change the + * semantics of {@link #isStampingEnabled} and {@link #collectSources}. */ public abstract class GenRuleBase implements RuleConfiguredTargetFactory { - /** - * Returns {@code true} if the rule should be stamped. - * - *

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}. - */ - protected abstract boolean isStampingEnabled(RuleContext ruleContext); - - /** Collects sources from src attribute. */ - protected ImmutableMap> collectSources( - List srcs) throws RuleErrorException { - ImmutableMap.Builder> labelMap = ImmutableMap.builder(); - - for (TransitiveInfoCollection dep : srcs) { - NestedSet files = dep.getProvider(FileProvider.class).getFilesToBuild(); - labelMap.put(AliasProvider.getDependencyLabel(dep), files); - } - - return labelMap.build(); - } - - enum CommandType { - BASH, - WINDOWS_BATCH, - WINDOWS_POWERSHELL, - } - - @Nullable - private static Pair determineCommandTypeAndAttribute( - RuleContext ruleContext) { - AttributeMap attributeMap = ruleContext.attributes(); - if (ruleContext.isExecutedOnWindows()) { - if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) { - return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps"); - } - if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bat")) { - return Pair.of(CommandType.WINDOWS_BATCH, "cmd_bat"); - } - } - if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bash")) { - return Pair.of(CommandType.BASH, "cmd_bash"); - } - if (attributeMap.isAttributeValueExplicitlySpecified("cmd")) { - return Pair.of(CommandType.BASH, "cmd"); - } - ruleContext.attributeError( - "cmd", - "missing value for `cmd` attribute, you can also set `cmd_ps` or `cmd_bat` on" - + " Windows and `cmd_bash` on other platforms."); - return null; - } - @Override @Nullable - public ConfiguredTarget create(RuleContext ruleContext) + public final ConfiguredTarget create(RuleContext ruleContext) throws InterruptedException, RuleErrorException, ActionConflictException { NestedSet filesToBuild = NestedSetBuilder.wrap(Order.STABLE_ORDER, ruleContext.getOutputArtifacts()); @@ -145,7 +93,9 @@ public ConfiguredTarget create(RuleContext ruleContext) // The CommandHelper class makes an explicit copy of this in the constructor, so flattening // here should be benign. CommandHelper commandHelper = - commandHelperBuilder(ruleContext) + CommandHelper.builder(ruleContext) + .addToolDependencies("tools") + .addToolDependencies("toolchains") .addLabelMap( labelMap.entrySet().stream() .collect(toImmutableMap(Map.Entry::getKey, e -> e.getValue().toList()))) @@ -167,7 +117,7 @@ public ConfiguredTarget create(RuleContext ruleContext) ruleContext, resolvedSrcs, filesToBuild, - /* makeVariableSuppliers = */ ImmutableList.of(), + /* makeVariableSuppliers= */ ImmutableList.of(), expandToWindowsPath); String command = ruleContext @@ -281,10 +231,49 @@ public String toString() { .build(); } - protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) { - return CommandHelper.builder(ruleContext) - .addToolDependencies("tools") - .addToolDependencies("toolchains"); + /** + * Returns {@code true} if the rule should be stamped. + * + *

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> collectSources( + List srcs) throws RuleErrorException; + + private enum CommandType { + BASH, + WINDOWS_BATCH, + WINDOWS_POWERSHELL, + } + + @Nullable + private static Pair determineCommandTypeAndAttribute( + RuleContext ruleContext) { + AttributeMap attributeMap = ruleContext.attributes(); + if (ruleContext.isExecutedOnWindows()) { + if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) { + return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps"); + } + if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bat")) { + return Pair.of(CommandType.WINDOWS_BATCH, "cmd_bat"); + } + } + if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bash")) { + return Pair.of(CommandType.BASH, "cmd_bash"); + } + if (attributeMap.isAttributeValueExplicitlySpecified("cmd")) { + return Pair.of(CommandType.BASH, "cmd"); + } + ruleContext.attributeError( + "cmd", + "missing value for `cmd` attribute, you can also set `cmd_ps` or `cmd_bat` on" + + " Windows and `cmd_bash` on other platforms."); + return null; } /** @@ -303,14 +292,14 @@ private static Artifact getExecutable(RuleContext ruleContext, NestedSet resolvedSrcs; private final NestedSet filesToBuild; private final boolean windowsPath; - public CommandResolverContext( + CommandResolverContext( RuleContext ruleContext, NestedSet resolvedSrcs, NestedSet filesToBuild, @@ -327,10 +316,6 @@ public CommandResolverContext( this.windowsPath = windowsPath; } - public RuleContext getRuleContext() { - return ruleContext; - } - @Override public String lookupVariable(String variableName) throws ExpansionException, InterruptedException { @@ -397,15 +382,13 @@ private String lookupVariableImpl(String variableName) * Returns the path of the sole element "artifacts", generating an exception with an informative * error message iff the set is not a singleton. Used to expand "$<", "$@". */ - private static final String expandSingletonArtifact( + private static String expandSingletonArtifact( NestedSet artifacts, String variable, String artifactName) throws ExpansionException { if (artifacts.isEmpty()) { - throw new ExpansionException("variable '" + variable - + "' : no " + artifactName); + throw new ExpansionException("variable '" + variable + "' : no " + artifactName); } else if (!artifacts.isSingleton()) { - throw new ExpansionException("variable '" + variable - + "' : more than one " + artifactName); + throw new ExpansionException("variable '" + variable + "' : more than one " + artifactName); } return artifacts.getSingleton().getExecPathString(); } From 3f64fa56d73a5b40cb07695b897a36c31cb18e1a Mon Sep 17 00:00:00 2001 From: Alessandro Patti Date: Wed, 21 Feb 2024 12:27:24 -0800 Subject: [PATCH 2/2] Fix genrule autostamping in bazel 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 --- .../lib/bazel/rules/genrule/BazelGenRule.java | 10 --------- .../bazel/rules/genrule/BazelGenRuleRule.java | 9 ++++---- .../build/lib/rules/genrule/GenRuleBase.java | 22 ++++++++++--------- .../genrule/GenRuleConfiguredTargetTest.java | 7 +++++- 4 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java index 97edfaae574cca..91a600fb6db7fc 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java @@ -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> collectSources( List srcs) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java index 685925c287ff7c..24cbe28c20ba4e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java @@ -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) { @@ -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(); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java index 51a4c116b17937..a492981b37df87 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java @@ -45,7 +45,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; @@ -58,7 +60,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 { @@ -231,20 +233,20 @@ public String toString() { .build(); } - /** - * Returns {@code true} if the rule should be stamped. - * - *

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> collectSources( List 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, diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java index 5a14522976a023..b44289e8a4f54f 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java @@ -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='')"); } @@ -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"); } @@ -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"); }