From db4e8f92042d7b7f61aa09e18045610677ed7f42 Mon Sep 17 00:00:00 2001 From: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> Date: Mon, 3 Oct 2022 20:24:09 -0700 Subject: [PATCH 1/5] fix: expand_location binary target inputs The previous behaviour was that location expansion on native rules would accept binary targets as inputs and resolve to a single location. This was not an option on custom rules. This patch aligns the behaviour of Starlark's ctx.expand_location to the native rules. E.g. a py_binary can be expanded using location/execpath/rootpath instead of previously only locations/execpaths/rootpaths. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> --- .../starlark/StarlarkRuleContext.java | 11 ++++-- ...arlarkRuleImplementationFunctionsTest.java | 35 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) 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 ba336444d7dbee..421509475e4763 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 @@ -1133,7 +1133,7 @@ private static void checkDeprecated(String newApi, String oldApi, StarlarkSemant } /** - * Builds a map: Label -> List of files from the given labels + * Builds a map: Label -> List of runfiles from the given labels. * * @param knownLabels List of known labels * @return Immutable map with immutable collections as values @@ -1143,9 +1143,14 @@ public static ImmutableMap> makeLabelMap( ImmutableMap.Builder> builder = ImmutableMap.builder(); for (TransitiveInfoCollection current : knownLabels) { + Label label = AliasProvider.getDependencyLabel(current); + FilesToRunProvider filesToRun = current.getProvider(FilesToRunProvider.class); + Artifact executableArtifact = filesToRun.getExecutable(); builder.put( - AliasProvider.getDependencyLabel(current), - current.getProvider(FileProvider.class).getFilesToBuild().toList()); + label, + executableArtifact != null + ? ImmutableList.of(executableArtifact) + : filesToRun.getFilesToRun().toList()); } return builder.buildOrThrow(); 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 3ecf4c1950728e..54457a66a30c57 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 @@ -658,6 +658,41 @@ public void testExpandLocationWithShortPaths() throws Exception { assertThat(loc).isEqualTo("foo/libjl.jar"); } + /** Asserts that a custom rule has the same behaviour as native rules when + * expanding executable target locations. + */ + @Test + public void testExpandLocationExecutableTargets() throws Exception { + scratch.file( + "test/defs.bzl", + "def _impl(ctx):", + " env = {}", + " for k, v in ctx.attr.env.items():", + " env[k] = ctx.expand_location(v, targets = ctx.attr.data)", + " return [DefaultInfo()]", + "expand_location_env_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'data': attr.label_list(allow_files=True),", + " 'env': attr.string_dict(),", + " }", + ")"); + + scratch.file("test/main.py", ""); + + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'expand_location_env_rule')", + "py_binary(name = 'main', srcs = ['main.py'])", + "expand_location_env_rule(", + " name = 'expand_location_env',", + " data = [':main'],", + " env = {'MAIN_EXECPATH': '$(execpath :main)'},", + ")"); + + assertThat(getConfiguredTarget("//test:expand_location_env")).isNotNull(); + } + /** Regression test to check that expand_location allows ${var} and $$. */ @Test public void testExpandLocationWithDollarSignsAndCurlys() throws Exception { From 941c8f5298ac570f6b404faed84cb52be9615fff Mon Sep 17 00:00:00 2001 From: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> Date: Tue, 4 Oct 2022 11:14:37 -0700 Subject: [PATCH 2/5] fix: be defensive by also looking into "files to build" Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> --- .../starlark/StarlarkRuleContext.java | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) 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 421509475e4763..034a0d49fea8a1 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 @@ -1140,17 +1140,23 @@ private static void checkDeprecated(String newApi, String oldApi, StarlarkSemant */ public static ImmutableMap> makeLabelMap( Iterable knownLabels) { + ImmutableMap.Builder> builder = ImmutableMap.builder(); - for (TransitiveInfoCollection current : knownLabels) { - Label label = AliasProvider.getDependencyLabel(current); - FilesToRunProvider filesToRun = current.getProvider(FilesToRunProvider.class); - Artifact executableArtifact = filesToRun.getExecutable(); - builder.put( - label, - executableArtifact != null - ? ImmutableList.of(executableArtifact) - : filesToRun.getFilesToRun().toList()); + for (TransitiveInfoCollection label : knownLabels) { + FilesToRunProvider filesToRun = label.getProvider(FilesToRunProvider.class); + if (filesToRun != null) { + Artifact executableArtifact = filesToRun.getExecutable(); + builder.put( + AliasProvider.getDependencyLabel(label), + executableArtifact != null + ? ImmutableList.of(executableArtifact) + : filesToRun.getFilesToRun().toList()); + } else { + builder.put( + AliasProvider.getDependencyLabel(label), + label.getProvider(FileProvider.class).getFilesToBuild().toList()); + } } return builder.buildOrThrow(); From 4bd03955aaeee7e1c8b7d94df88e20080a119a8a Mon Sep 17 00:00:00 2001 From: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> Date: Tue, 4 Oct 2022 11:15:16 -0700 Subject: [PATCH 3/5] doc: why executable targets are special cased Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> --- .../build/lib/analysis/starlark/StarlarkRuleContext.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 034a0d49fea8a1..9d75c2863934b3 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 @@ -1133,7 +1133,11 @@ private static void checkDeprecated(String newApi, String oldApi, StarlarkSemant } /** - * Builds a map: Label -> List of runfiles from the given labels. + * Builds a map: Label -> List of files from the given labels. + * It first looks into the runfiles and then into files. If the label being iterated is an + * executable, then it will have the same behaviour as native rules special attributes (data, + * tools) in which only the executable file is mapped to the label. This allows executable targets + * to have the location expansion work with the singular form of location/execpath/rootpath. * * @param knownLabels List of known labels * @return Immutable map with immutable collections as values From 61f1b836398a26b015ce6af0217a6050181209e9 Mon Sep 17 00:00:00 2001 From: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> Date: Tue, 4 Oct 2022 14:46:15 -0700 Subject: [PATCH 4/5] test: add more coverage Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> --- ...arlarkRuleImplementationFunctionsTest.java | 58 +++++++++++++++---- 1 file changed, 48 insertions(+), 10 deletions(-) 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 54457a66a30c57..e207a283f5f968 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 @@ -50,6 +50,7 @@ import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; +import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -665,32 +666,69 @@ public void testExpandLocationWithShortPaths() throws Exception { public void testExpandLocationExecutableTargets() throws Exception { scratch.file( "test/defs.bzl", - "def _impl(ctx):", + "def _my_binary_impl(ctx):", + " executable = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.write(executable, '', is_executable = True)", + " return [DefaultInfo(executable = executable, files = depset(ctx.files.srcs), runfiles = ctx.runfiles(ctx.files.srcs))]", + "my_binary = rule(", + " implementation = _my_binary_impl,", + " attrs = {'srcs': attr.label_list(allow_files=True)},", + " executable = True,", + ")", + "def _expand_location_env_rule_impl(ctx):", " env = {}", " for k, v in ctx.attr.env.items():", " env[k] = ctx.expand_location(v, targets = ctx.attr.data)", - " return [DefaultInfo()]", + " env_file = ctx.actions.declare_file('env_file')", + " ctx.actions.write(env_file, str(env))", + " return [DefaultInfo(files = depset([env_file]))]", "expand_location_env_rule = rule(", - " implementation = _impl,", + " implementation = _expand_location_env_rule_impl,", " attrs = {", " 'data': attr.label_list(allow_files=True),", " 'env': attr.string_dict(),", - " }", + " },", ")"); - - scratch.file("test/main.py", ""); + + scratch.file("test/file1", ""); + scratch.file("test/file2", ""); scratch.file( "test/BUILD", - "load('//test:defs.bzl', 'expand_location_env_rule')", - "py_binary(name = 'main', srcs = ['main.py'])", + "load('//test:defs.bzl', 'expand_location_env_rule', 'my_binary')", + "my_binary(name = 'main')", + "filegroup(name = 'files', srcs = ['file1', 'file2'])", "expand_location_env_rule(", - " name = 'expand_location_env',", + " name = 'expand_execpath_env',", " data = [':main'],", " env = {'MAIN_EXECPATH': '$(execpath :main)'},", + ")", + "expand_location_env_rule(", + " name = 'expand_execpaths_env',", + " data = [':files'],", + " env = {'FILES': '$(execpaths :files)'},", ")"); - assertThat(getConfiguredTarget("//test:expand_location_env")).isNotNull(); + TransitiveInfoCollection expandExecpathEnv = getConfiguredTarget("//test:expand_execpath_env"); + Artifact artifact = Iterables.getOnlyElement( + expandExecpathEnv + .getProvider(FileProvider.class) + .getFilesToBuild() + .toList()); + FileWriteAction action = (FileWriteAction) getGeneratingAction(artifact); + assertMatches( + "env file contains expanded location of runfile", + "\\{\"MAIN_EXECPATH\": \"[\\w-_/]+/test/main\"\\}", + action.getFileContents()); + + expandExecpathEnv = getConfiguredTarget("//test:expand_execpaths_env"); + artifact = Iterables.getOnlyElement( + expandExecpathEnv + .getProvider(FileProvider.class) + .getFilesToBuild() + .toList()); + action = (FileWriteAction) getGeneratingAction(artifact); + assertThat(action.getFileContents()).isEqualTo("{\"FILES\": \"test/file1 test/file2\"}"); } /** Regression test to check that expand_location allows ${var} and $$. */ From d8176177c95ff90c471db2c46085dfbc93fca4c1 Mon Sep 17 00:00:00 2001 From: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> Date: Tue, 4 Oct 2022 16:25:39 -0700 Subject: [PATCH 5/5] fix: runfiles -> files-to-run Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> --- .../build/lib/analysis/starlark/StarlarkRuleContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9d75c2863934b3..240abac7e05288 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 @@ -1134,7 +1134,7 @@ private static void checkDeprecated(String newApi, String oldApi, StarlarkSemant /** * Builds a map: Label -> List of files from the given labels. - * It first looks into the runfiles and then into files. If the label being iterated is an + * It first looks into the files-to-run and then into files. If the label being iterated is an * executable, then it will have the same behaviour as native rules special attributes (data, * tools) in which only the executable file is mapped to the label. This allows executable targets * to have the location expansion work with the singular form of location/execpath/rootpath.