diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java index f2dfa519191e4d..1388927b974824 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java @@ -24,8 +24,11 @@ import com.google.devtools.build.lib.packages.StarlarkProviderWrapper; import com.google.devtools.build.lib.packages.TriState; import com.google.devtools.build.lib.packages.Type; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.List; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Starlark; /** * Utility functions for use during analysis. @@ -73,6 +76,44 @@ public static boolean isStampingEnabled(RuleContext ruleContext) { return isStampingEnabled(ruleContext, ruleContext.getConfiguration()); } + /** + * Returns whether workspace status files ({@code ctx.info_file} / {@code ctx.version_file}) may + * be accessed for the given rule. + */ + public static boolean areWorkspaceStatusFilesAvailable(RuleContext ruleContext) { + BuildConfigurationValue config = ruleContext.getConfiguration(); + if (config.isToolConfiguration()) { + return false; + } + if (ruleContext.attributes().has("stamp", BuildType.TRISTATE) + || ruleContext.attributes().has("stamp", Type.INTEGER)) { + return isStampingEnabled(ruleContext, config); + } + return config.stampBinaries(); + } + + /** + * Verifies that workspace status files may be accessed, or fails with an error pointing at the + * calling rule implementation. + */ + public static void checkWorkspaceStatusFileAccess(RuleContext ruleContext, String apiName) + throws EvalException { + if (!ruleContext + .getAnalysisEnvironment() + .getStarlarkSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_PREVENT_STATUS_FILES_WITHOUT_STAMP)) { + return; + } + if (areWorkspaceStatusFilesAvailable(ruleContext)) { + return; + } + throw Starlark.errorf( + "%s cannot be accessed without stamping when" + + " --incompatible_prevent_status_files_without_stamp is enabled. Enable stamping with" + + " the stamp attribute or --stamp.", + apiName); + } + // TODO(bazel-team): These need Iterable because they need to // be called with Iterable. Once the configured target lockdown is complete, we // can eliminate the "extends" clauses. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java index 71b601b61d3e94..ba61f586ecb23a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.actions.extra.SpawnInfo; +import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.BashCommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.FilesToRunProvider; @@ -477,6 +478,9 @@ private Artifact transformBuildInfoFile( StarlarkThread thread) throws InterruptedException, EvalException { RuleContext ruleContext = getRuleContext(); + String apiName = + isVolatile ? "ctx.actions.transform_version_file" : "ctx.actions.transform_info_file"; + AnalysisUtils.checkWorkspaceStatusFileAccess(ruleContext, apiName); Artifact templateFile = (Artifact) templateObject; PathFragment fragment = ruleContext.getPackageDirectory().getRelative(PathFragment.create(outputFileName)); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java index e21280dab8f120..ed970343ebccdd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.analysis.ActionsProvider; import com.google.devtools.build.lib.analysis.AliasProvider; +import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.AspectContext; import com.google.devtools.build.lib.analysis.BashCommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; @@ -980,12 +981,14 @@ public boolean areRunfilesFromDeps(FilesToRunProvider executable) { @Override public Artifact getStableWorkspaceStatus() throws InterruptedException, EvalException { checkMutable("info_file"); + AnalysisUtils.checkWorkspaceStatusFileAccess(ruleContext, "ctx.info_file"); return ruleContext.getAnalysisEnvironment().getStableWorkspaceStatusArtifact(); } @Override public Artifact getVolatileWorkspaceStatus() throws InterruptedException, EvalException { checkMutable("version_file"); + AnalysisUtils.checkWorkspaceStatusFileAccess(ruleContext, "ctx.version_file"); return ruleContext.getAnalysisEnvironment().getVolatileWorkspaceStatusArtifact(); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 6f41b5475e23fc..37497c7a8482fa 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -725,6 +725,20 @@ public final class BuildLanguageOptions extends OptionsBase { + "from the top level target instead (No-op in Bazel)") public boolean incompatibleDisableObjcLibraryTransition; + @Option( + name = "incompatible_prevent_status_files_without_stamp", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If enabled, ctx.info_file and ctx.version_file (and the corresponding" + + " ctx.actions.transform_*_file functions) are only available when stamping is" + + " enabled for the target (via the stamp attribute or --stamp). Otherwise, rule" + + " implementations that access these files fail during analysis. See" + + " https://github.com/bazelbuild/bazel/issues/14341.") + public boolean incompatiblePreventStatusFilesWithoutStamp; + // remove after Bazel LTS in Nov 2023 @Option( name = "incompatible_fail_on_unknown_attributes", @@ -811,6 +825,20 @@ public final class BuildLanguageOptions extends OptionsBase { + " attributes of symbolic macros or attribute default values.") public boolean incompatibleSimplifyUnconditionalSelectsInRuleAttrs; + @Option( + name = "incompatible_prevent_status_files_without_stamp", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If enabled, ctx.info_file and ctx.version_file (and the corresponding" + + " ctx.actions.transform_*_file functions) are only available when stamping is" + + " enabled for the target (via the stamp attribute or --stamp). Otherwise, rule" + + " implementations that access these files fail during analysis. See" + + " https://github.com/bazelbuild/bazel/issues/14341.") + public abstract boolean getIncompatiblePreventStatusFilesWithoutStamp(); + @Option( name = "experimental_enable_starlark_set", defaultValue = "true", @@ -995,6 +1023,9 @@ private void setFlags(FlagConsumer consumer) { .setBool( INCOMPATIBLE_DISABLE_OBJC_LIBRARY_TRANSITION, incompatibleDisableObjcLibraryTransition) + .setBool( + INCOMPATIBLE_PREVENT_STATUS_FILES_WITHOUT_STAMP, + incompatiblePreventStatusFilesWithoutStamp) .setBool(INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES, incompatibleFailOnUnknownAttributes) .setBool( INCOMPATIBLE_ENABLE_PROTO_TOOLCHAIN_RESOLUTION, @@ -1144,6 +1175,9 @@ public FlagConsumer setBool(String key, boolean ignored) { "+incompatible_depset_for_libraries_to_link_getter"; public static final String INCOMPATIBLE_DISABLE_TARGET_PROVIDER_FIELDS = "-incompatible_disable_target_provider_fields"; + public static final String INCOMPATIBLE_PREVENT_STATUS_FILES_WITHOUT_STAMP = + "-incompatible_prevent_status_files_without_stamp"; + // Note that INCOMPATIBLE_DISALLOW_EMPTY_GLOB differs in Google and in OSS Bazel. public static final String INCOMPATIBLE_DISALLOW_EMPTY_GLOB = "+incompatible_disallow_empty_glob"; public static final String INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX = diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index 2ab8688213b3eb..6f586e18f0460a 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -142,6 +142,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--incompatible_always_check_depset_elements=" + rand.nextBoolean(), "--incompatible_depset_for_libraries_to_link_getter=" + rand.nextBoolean(), "--incompatible_disable_target_provider_fields=" + rand.nextBoolean(), + "--incompatible_prevent_status_files_without_stamp=" + rand.nextBoolean(), "--incompatible_disallow_empty_glob=" + rand.nextBoolean(), "--incompatible_disallow_struct_provider_syntax=" + rand.nextBoolean(), "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(), diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java index 76881a5e68e5a8..78a6c299861e48 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.analysis.actions.BuildInfoFileWriteAction; import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.actions.StarlarkAction; +import com.google.devtools.build.lib.analysis.config.CoreOptions; import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget; import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.analysis.starlark.StarlarkExecGroupCollection; @@ -4447,4 +4448,114 @@ public void transformFile_cannotBeAccessedOutsideOfAllowlist( " template = ':template.txt',", ")"); } -} + + @Test + public void infoFile_allowedWithoutStampByDefault() throws Exception { + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " ctx.actions.write(ctx.outputs.out, ctx.info_file.path)", + " return DefaultInfo(files = depset([ctx.outputs.out]))", + "status_rule = rule(", + " implementation = _impl,", + " attrs = {\"stamp\": attr.int(default = -1)},", + " outputs = {\"out\": \"%{name}.txt\"},", + ")", + testingRuleDefinition); + scratch.file( + "test/BUILD", + "load(':rules.bzl', 'status_rule')", + "status_rule(name = 'target')"); + + getConfiguredTarget("//test:target"); + } + + @Test + public void infoFile_errorsWithoutStampWhenPrevented() throws Exception { + setBuildLanguageOptions("--incompatible_prevent_status_files_without_stamp"); + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " ctx.actions.write(ctx.outputs.out, ctx.info_file.path)", + " return DefaultInfo(files = depset([ctx.outputs.out]))", + "status_rule = rule(", + " implementation = _impl,", + " attrs = {\"stamp\": attr.int(default = -1)},", + " outputs = {\"out\": \"%{name}.txt\"},", + ")", + testingRuleDefinition); + scratch.file( + "test/BUILD", + "load(':rules.bzl', 'status_rule')", + "status_rule(name = 'target')"); + + checkError( + "//test:target", + "ctx.info_file cannot be accessed without stamping", + "incompatible_prevent_status_files_without_stamp"); + } + + @Test + public void infoFile_allowedWithStampWhenPrevented() throws Exception { + setBuildLanguageOptions("--incompatible_prevent_status_files_without_stamp", "--stamp"); + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " ctx.actions.write(ctx.outputs.out, ctx.info_file.path)", + " return DefaultInfo(files = depset([ctx.outputs.out]))", + "status_rule = rule(", + " implementation = _impl,", + " attrs = {\"stamp\": attr.int(default = 1)},", + " outputs = {\"out\": \"%{name}.txt\"},", + ")", + testingRuleDefinition); + scratch.file( + "test/BUILD", + "load(':rules.bzl', 'status_rule')", + "status_rule(name = 'target')"); + + getConfiguredTarget("//test:target"); + } + + @Test + public void infoFile_allowedWhenStampEnabledByDependencyTransition() throws Exception { + setBuildLanguageOptions("--incompatible_prevent_status_files_without_stamp"); + useConfiguration("--nostamp"); + scratch.file( + "test/rules.bzl", + "def _stamp_transition_impl(settings, _attr):", + " return {'//command_line_option:stamp': not settings['//command_line_option:stamp']}", + "stamp_transition = transition(", + " implementation = _stamp_transition_impl,", + " inputs = ['//command_line_option:stamp'],", + " outputs = ['//command_line_option:stamp'],", + ")", + "def _stamped_dep_impl(ctx):", + " ctx.actions.write(ctx.outputs.out, ctx.info_file.path)", + " return DefaultInfo(files = depset([ctx.outputs.out]))", + "stamped_dep = rule(", + " implementation = _stamped_dep_impl,", + " outputs = {\"out\": \"%{name}.txt\"},", + ")", + "def _consumer_impl(ctx):", + " return DefaultInfo()", + "consumer = rule(", + " implementation = _consumer_impl,", + " attrs = {\"dep\": attr.label(cfg = stamp_transition)},", + ")", + testingRuleDefinition); + scratch.file( + "test/BUILD", + "load(':rules.bzl', 'consumer', 'stamped_dep')", + "stamped_dep(name = 'dep')", + "consumer(name = 'top', dep = ':dep')"); + + ConfiguredTarget top = getConfiguredTarget("//test:top"); + ConfiguredTarget dep = Iterables.getOnlyElement(getPrerequisites(top, "dep")); + + assertThat(getConfiguration(top).getOptions().get(CoreOptions.class).stampBinaries) + .isFalse(); + assertThat(getConfiguration(dep).getOptions().get(CoreOptions.class).stampBinaries) + .isTrue(); + assertThat(getFilesToBuild(dep)).isNotEmpty(); + } diff --git a/src/test/shell/bazel/bazel_workspace_status_test.sh b/src/test/shell/bazel/bazel_workspace_status_test.sh index ebac7f696493da..ecb4bb4df05a78 100755 --- a/src/test/shell/bazel/bazel_workspace_status_test.sh +++ b/src/test/shell/bazel/bazel_workspace_status_test.sh @@ -258,4 +258,40 @@ EOF } +function test_prevent_status_files_without_stamp() { + create_new_workspace + + cat > rules.bzl <<'EOF' +def _impl(ctx): + ctx.actions.write(ctx.outputs.out, ctx.info_file.path) + return DefaultInfo(files = depset([ctx.outputs.out])) + +uses_status = rule( + implementation = _impl, + attrs = {"stamp": attr.int(default = -1)}, + outputs = {"out": "%{name}.txt"}, +) +EOF + + cat > BUILD <<'EOF' +load(":rules.bzl", "uses_status") + +uses_status(name = "unstamped") +uses_status(name = "stamped", stamp = 1) +EOF + + # By default, status files are available without stamping. + bazel build //:unstamped &> $TEST_log || fail "expected build to succeed by default" + + # With the incompatible flag enabled, unstamped targets fail and point at the rule implementation. + bazel build --incompatible_prevent_status_files_without_stamp //:unstamped &> $TEST_log \ + && fail "expected build to fail" || true + expect_log "ctx.info_file cannot be accessed without stamping" + expect_log "rules.bzl" + + # Stamped targets succeed with the incompatible flag enabled. + bazel build --incompatible_prevent_status_files_without_stamp --stamp //:stamped \ + &> $TEST_log || fail "expected stamped build to succeed" +} + run_suite "workspace status tests"