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 6875b6d8260dc7..1966fa6aac6cbb 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 @@ -1015,7 +1015,11 @@ public String expandLocation( checkMutable("expand_location"); try { ImmutableMap> labelMap = - makeLabelMap(Sequence.cast(targets, TransitiveInfoCollection.class, "targets")); + makeLabelMap( + Sequence.cast(targets, TransitiveInfoCollection.class, "targets"), + thread + .getSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE)); LocationExpander expander; if (!shortPaths) { expander = LocationExpander.withExecPaths(ruleContext, labelMap); @@ -1238,10 +1242,13 @@ private static Map> checkLabelDict(Map labelDict * Builds a map: Label -> List of files from the given labels * * @param knownLabels List of known labels + * @param locationsPrefersExecutable whether to prefer an executable over a list of files that + * isn't a singleton when requesting a plural location function * @return Immutable map with immutable collections as values */ public static ImmutableMap> makeLabelMap( - Iterable knownLabels) throws EvalException { + Iterable knownLabels, boolean locationsPrefersExecutable) + throws EvalException { var targetsMap = new LinkedHashMap>(); for (TransitiveInfoCollection current : knownLabels) { Label label = AliasProvider.getDependencyLabel(current); @@ -1250,7 +1257,22 @@ public static ImmutableMap> makeLabelMap( "Label %s is found more than once in 'targets' list.", Starlark.repr(label.toString())); } - targetsMap.put(label, current.getProvider(FileProvider.class).getFilesToBuild().toList()); + var filesToRunProvider = current.getProvider(FilesToRunProvider.class); + ImmutableCollection expansion; + // Only use the executable if requesting the files to build via a singleton location function + // would fail to ensure backwards compatibility (this function used to always return the + // files to build). The case of a plural location function is potentially breaking, but gated + // behind locationsPrefersExecutable. + // Avoid flattening a nested set if the first branch is taken. + if (filesToRunProvider != null + && filesToRunProvider.getExecutable() != null + && locationsPrefersExecutable + && !current.getProvider(FileProvider.class).getFilesToBuild().isSingleton()) { + expansion = ImmutableList.of(filesToRunProvider.getExecutable()); + } else { + expansion = current.getProvider(FileProvider.class).getFilesToBuild().toList(); + } + targetsMap.put(label, expansion); } return ImmutableMap.copyOf(targetsMap); 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 cbfd0154c4745e..b7f5bb636391df 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 @@ -792,6 +792,18 @@ public final class BuildLanguageOptions extends OptionsBase { help = "If true, enable the set data type and set() constructor in Starlark.") public boolean experimentalEnableStarlarkSet; + @Option( + name = "incompatible_locations_prefers_executable", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "Whether a target that provides an executable expands to the executable rather than the" + + " files in DefaultInfo.files under $(locations ...) expansion if the" + + " number of files is not 1.") + public boolean incompatibleLocationsPrefersExecutable; + /** * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should * never accumulate a large number of these and being able to shortcut on object identity makes a @@ -900,6 +912,8 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool( INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS, incompatibleSimplifyUnconditionalSelectsInRuleAttrs) + .setBool( + INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE, incompatibleLocationsPrefersExecutable) .setBool( StarlarkSemantics.EXPERIMENTAL_ENABLE_STARLARK_SET, experimentalEnableStarlarkSet) .build(); @@ -1004,6 +1018,8 @@ public StarlarkSemantics toStarlarkSemantics() { "+incompatible_disallow_ctx_resolve_tools"; public static final String INCOMPATIBLE_SIMPLIFY_UNCONDITIONAL_SELECTS_IN_RULE_ATTRS = "+incompatible_simplify_unconditional_selects_in_rule_attrs"; + public static final String INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE = + "+incompatible_locations_prefers_executable"; // non-booleans public static final StarlarkSemantics.Key EXPERIMENTAL_BUILTINS_BZL_PATH = new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%"); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD b/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD index a06fee687c1e9d..9db236b51003dc 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/BUILD @@ -34,6 +34,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/rules/apple", "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkInternal.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkInternal.java index f18582b836caad..78b1e00b22c460 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkInternal.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcStarlarkInternal.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.analysis.Expander; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.rules.apple.AppleConfiguration; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import java.util.List; @@ -95,7 +96,10 @@ public Sequence expandAndTokenize( starlarkRuleContext.getRuleContext().getPrerequisites("data"), starlarkRuleContext .getRuleContext() - .getPrerequisites("additional_linker_inputs"))))) + .getPrerequisites("additional_linker_inputs"))), + starlarkRuleContext + .getStarlarkSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE))) .withDataExecLocations(); ImmutableList expandedFlags = expander.tokenized(attributeName, Sequence.cast(flags, String.class, attributeName)); diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java index 48da3354745573..3380b6a9b95c1b 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleContextApi.java @@ -448,7 +448,13 @@ String expandMakeVariables( }, defaultValue = "[]", named = true, - doc = "List of targets for additional lookup information."), + doc = + "List of targets for additional lookup information. These are expanded as follows:" + + " A target with a single file in DefaultInfo.files expands to" + + " that file. Other targets expand to their" + + " DefaultInfo.executable file if set and if" + + " --incompatible_locations_prefers_executable is enabled," + + " otherwise they expand to DefaultInfo.files."), @Param( name = "short_paths", named = true, 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 8f3868d3f49e91..8e3f77e834edc1 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_disallow_struct_provider_syntax=" + rand.nextBoolean(), "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(), "--incompatible_enable_deprecated_label_apis=" + rand.nextBoolean(), + "--incompatible_locations_prefers_executable=" + rand.nextBoolean(), "--incompatible_merge_fixed_and_default_shell_env=" + rand.nextBoolean(), "--incompatible_no_attr_license=" + rand.nextBoolean(), "--incompatible_no_implicit_file_export=" + rand.nextBoolean(), @@ -192,6 +193,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { BuildLanguageOptions.INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_ENABLE_DEPRECATED_LABEL_APIS, rand.nextBoolean()) + .setBool(BuildLanguageOptions.INCOMPATIBLE_LOCATIONS_PREFERS_EXECUTABLE, rand.nextBoolean()) .setBool( BuildLanguageOptions.INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_NO_ATTR_LICENSE, rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java index a2c537ba939f38..8786d9ba761fa6 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java @@ -46,6 +46,7 @@ import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.Runfiles; +import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction; import com.google.devtools.build.lib.analysis.actions.SpawnAction; @@ -69,6 +70,8 @@ import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.OsUtils; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; @@ -91,10 +94,9 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; /** Tests for Starlark functions relating to rule implementation. */ -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public final class StarlarkRuleImplementationFunctionsTest extends BuildViewTestCase { private final BazelEvaluationTestCase ev = new BazelEvaluationTestCase(); @@ -703,6 +705,134 @@ public void testExpandLocationWithDollarSignsAndCurlys() throws Exception { .isEqualTo("${abc} $(echo) $$ $"); } + @Test + public void testExpandedLocationWithSingleFileDifferentFromExecutable( + @TestParameter boolean locationsPrefersExecutable) throws Exception { + setBuildLanguageOptions( + "--incompatible_locations_prefers_executable=" + locationsPrefersExecutable); + + scratch.file( + "test/defs.bzl", + "def _my_binary_impl(ctx):", + " executable = ctx.actions.declare_file(ctx.attr.name + '_executable')", + " ctx.actions.write(executable, '', is_executable = True)", + " file = ctx.actions.declare_file(ctx.attr.name + '_file')", + " ctx.actions.write(file, '')", + " return [DefaultInfo(executable = executable, files = depset([file]))]", + "my_binary = rule(", + " implementation = _my_binary_impl,", + " executable = True,", + ")", + "def _expand_location_rule_impl(ctx):", + " expansions = []", + " for data in ctx.attr.data:", + " expansions.append(", + " ctx.expand_location('$(location ' + str(data.label) + ')', ctx.attr.data),", + " )", + " expansions.append(", + " ctx.expand_location('$(locations ' + str(data.label) + ')', ctx.attr.data)", + " )", + " file = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.write(file, '\\n'.join(expansions))", + " return [DefaultInfo(files = depset([file]))]", + "expand_location_rule = rule(", + " implementation = _expand_location_rule_impl,", + " attrs = {", + " 'data': attr.label_list(),", + " },", + ")"); + + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'expand_location_rule', 'my_binary')", + "my_binary(name = 'main')", + "expand_location_rule(", + " name = 'expand',", + " data = [':main'],", + ")"); + + TransitiveInfoCollection expandTarget = getConfiguredTarget("//test:expand"); + Artifact artifact = + Iterables.getOnlyElement( + expandTarget.getProvider(FileProvider.class).getFilesToBuild().toList()); + FileWriteAction action = (FileWriteAction) getGeneratingAction(artifact); + assertThat(action.getFileContents()) + .matches( + """ + ^\\S*/bin/test/main_file + \\S*/bin/test/main_file$\ + """); + } + + @Test + public void testExpandedLocationsWithMultipleFilesAndExecutable( + @TestParameter boolean locationsPrefersExecutable) throws Exception { + setBuildLanguageOptions( + "--incompatible_locations_prefers_executable=" + locationsPrefersExecutable); + + scratch.file( + "test/defs.bzl", + "def _my_binary_impl(ctx):", + " executable = ctx.actions.declare_file(ctx.attr.name + '_executable')", + " ctx.actions.write(executable, '', is_executable = True)", + " file1 = ctx.actions.declare_file(ctx.attr.name + '_file1')", + " file2 = ctx.actions.declare_file(ctx.attr.name + '_file2')", + " ctx.actions.write(file1, '')", + " ctx.actions.write(file2, '')", + " return [DefaultInfo(executable = executable, files = depset([file1, file2]))]", + "my_binary = rule(", + " implementation = _my_binary_impl,", + " executable = True,", + ")", + "def _expand_location_rule_impl(ctx):", + " expansions = []", + " for data in ctx.attr.data:", + " expansions.append(", + " ctx.expand_location('$(location ' + str(data.label) + ')', ctx.attr.data),", + " )", + " expansions.append(", + " ctx.expand_location('$(locations ' + str(data.label) + ')', ctx.attr.data)", + " )", + " file = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.write(file, '\\n'.join(expansions))", + " return [DefaultInfo(files = depset([file]))]", + "expand_location_rule = rule(", + " implementation = _expand_location_rule_impl,", + " attrs = {", + " 'data': attr.label_list(),", + " },", + ")"); + + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'expand_location_rule', 'my_binary')", + "my_binary(name = 'main')", + "expand_location_rule(", + " name = 'expand',", + " data = [':main'],", + ")"); + + reporter.removeHandler(failFastHandler); + TransitiveInfoCollection expandTarget = getConfiguredTarget("//test:expand"); + if (locationsPrefersExecutable) { + Artifact artifact = + Iterables.getOnlyElement( + expandTarget.getProvider(FileProvider.class).getFilesToBuild().toList()); + FileWriteAction action = (FileWriteAction) getGeneratingAction(artifact); + assertThat(action.getFileContents()) + .matches( + """ + ^\\S*/bin/test/main_executable + \\S*/bin/test/main_executable$\ + """); + } else { + assertContainsEvent( + "label '//test:main' in $(location) expression expands to more than one file"); + assertContainsEvent("/bin/test/main_file1,"); + assertContainsEvent("/bin/test/main_file2]"); + } + } + /** * Invokes ctx.expand_location() with the given parameters and checks whether this led to the * expected result