From 41877d0fefe3f021f3ff6d4ce398d0deb27157e6 Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 8 Apr 2021 15:09:21 -0700 Subject: [PATCH] Extract a separate StarlarkToolchainContext for starlark-only operations. Also make ToolchainContextApi use starlark threads. PiperOrigin-RevId: 367515900 --- .../google/devtools/build/lib/analysis/BUILD | 18 ++- .../analysis/ResolvedToolchainContext.java | 77 +---------- .../starlark/StarlarkExecGroupCollection.java | 3 +- .../starlark/StarlarkRuleContext.java | 18 +-- .../starlark/StarlarkToolchainContext.java | 127 ++++++++++++++++++ .../platform/ToolchainContextApi.java | 2 +- 6 files changed, 151 insertions(+), 94 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkToolchainContext.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 0c473184eea086..6b23175a20d6ad 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -349,6 +349,7 @@ java_library( ":starlark/starlark_command_line", ":starlark/starlark_exec_group_collection", ":starlark/starlark_late_bound_default", + ":starlark/starlark_toolchain_context", ":template_variable_info", ":test/analysis_failure", ":test/analysis_failure_info", @@ -990,8 +991,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_context_key", "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:unloaded_toolchain_context", - "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform", - "//src/main/java/net/starlark/java/eval", "//src/main/protobuf:failure_details_java_proto", "//third_party:auto_value", "//third_party:guava", @@ -2151,6 +2150,7 @@ java_library( srcs = ["starlark/StarlarkExecGroupCollection.java"], deps = [ ":resolved_toolchain_context", + ":starlark/starlark_toolchain_context", ":toolchain_collection", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform", "//src/main/java/net/starlark/java/eval", @@ -2197,6 +2197,20 @@ java_library( ], ) +java_library( + name = "starlark/starlark_toolchain_context", + srcs = ["starlark/StarlarkToolchainContext.java"], + deps = [ + ":resolved_toolchain_context", + "//src/main/java/com/google/devtools/build/lib/analysis/platform", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform", + "//src/main/java/net/starlark/java/eval", + "//third_party:auto_value", + "//third_party:jsr305", + ], +) + # TODO(b/144899336): This should be lib/analysis/test/BUILD java_library( name = "test/analysis_failure", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ResolvedToolchainContext.java b/src/main/java/com/google/devtools/build/lib/analysis/ResolvedToolchainContext.java index a707abd7c0687d..147efc0a1f5976 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ResolvedToolchainContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ResolvedToolchainContext.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.analysis; -import static java.util.stream.Collectors.joining; - import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -23,7 +21,6 @@ import com.google.devtools.build.lib.analysis.platform.ToolchainInfo; import com.google.devtools.build.lib.analysis.platform.ToolchainTypeInfo; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -31,12 +28,7 @@ import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.skyframe.ToolchainException; import com.google.devtools.build.lib.skyframe.UnloadedToolchainContext; -import com.google.devtools.build.lib.starlarkbuildapi.platform.ToolchainContextApi; import javax.annotation.Nullable; -import net.starlark.java.eval.EvalException; -import net.starlark.java.eval.Printer; -import net.starlark.java.eval.Starlark; -import net.starlark.java.eval.StarlarkSemantics; /** * Represents the data needed for a specific target's use of toolchains and platforms, including @@ -45,7 +37,7 @@ @AutoValue @Immutable @ThreadSafe -public abstract class ResolvedToolchainContext implements ToolchainContextApi, ToolchainContext { +public abstract class ResolvedToolchainContext implements ToolchainContext { /** * Finishes preparing the {@link ResolvedToolchainContext} by finding the specific toolchain @@ -107,15 +99,15 @@ public static ResolvedToolchainContext load( } /** Returns the repository mapping applied by the Starlark 'in' operator to string-form labels. */ - abstract ImmutableMap repoMapping(); + public abstract ImmutableMap repoMapping(); /** Returns a description of the target being used, for error messaging. */ public abstract String targetDescription(); /** Sets the map from requested {@link Label} to toolchain type provider. */ - abstract ImmutableMap requestedToolchainTypeLabels(); + public abstract ImmutableMap requestedToolchainTypeLabels(); - abstract ImmutableMap toolchains(); + public abstract ImmutableMap toolchains(); /** Returns the template variables that these toolchains provide. */ public abstract ImmutableList templateVariableProviders(); @@ -138,67 +130,6 @@ public ToolchainInfo forToolchainType(ToolchainTypeInfo toolchainType) { return toolchains().get(toolchainType); } - @Override - public boolean isImmutable() { - return true; - } - - @Override - public void repr(Printer printer) { - printer.append(""); - } - - private static Label transformKey( - Object key, ImmutableMap repoMapping) throws EvalException { - if (key instanceof Label) { - return (Label) key; - } else if (key instanceof ToolchainTypeInfo) { - return ((ToolchainTypeInfo) key).typeLabel(); - } else if (key instanceof String) { - try { - return Label.parseAbsolute((String) key, repoMapping); - } catch (LabelSyntaxException e) { - throw Starlark.errorf("Unable to parse toolchain label '%s': %s", key, e.getMessage()); - } - } else { - throw Starlark.errorf( - "Toolchains only supports indexing by toolchain type, got %s instead", - Starlark.type(key)); - } - } - - @Override - public ToolchainInfo getIndex(StarlarkSemantics semantics, Object key) throws EvalException { - Label toolchainTypeLabel = transformKey(key, repoMapping()); - - if (!containsKey(semantics, key)) { - // TODO(bazel-configurability): The list of available toolchain types is confusing in the - // presence of aliases, since it only contains the actual label, not the alias passed to the - // rule definition. - throw Starlark.errorf( - "In %s, toolchain type %s was requested but only types [%s] are configured", - targetDescription(), - toolchainTypeLabel, - requiredToolchainTypes().stream() - .map(ToolchainTypeInfo::typeLabel) - .map(Label::toString) - .collect(joining(", "))); - } - return forToolchainType(toolchainTypeLabel); - } - - @Override - public boolean containsKey(StarlarkSemantics semantics, Object key) throws EvalException { - Label toolchainTypeLabel = transformKey(key, repoMapping()); - return requestedToolchainTypeLabels().containsKey(toolchainTypeLabel); - } - /** * Exception used when a toolchain type is required but the resolved target does not have * ToolchainInfo. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkExecGroupCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkExecGroupCollection.java index e360a1c2469020..1c8e45f9b36227 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkExecGroupCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkExecGroupCollection.java @@ -81,7 +81,8 @@ public StarlarkExecGroupContext getIndex(StarlarkSemantics semantics, Object key execGroup, String.join(", ", getScrubbedExecGroups())); } - ToolchainContextApi toolchainContext = toolchainCollection().getToolchainContext(execGroup); + ToolchainContextApi toolchainContext = + StarlarkToolchainContext.create(toolchainCollection().getToolchainContext(execGroup)); return new AutoValue_StarlarkExecGroupCollection_StarlarkExecGroupContext(toolchainContext); } 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 82e6a403dfbde9..909467c6828425 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 @@ -39,7 +39,6 @@ import com.google.devtools.build.lib.analysis.LabelExpander; import com.google.devtools.build.lib.analysis.LabelExpander.NotUniqueExpansionException; import com.google.devtools.build.lib.analysis.LocationExpander; -import com.google.devtools.build.lib.analysis.ResolvedToolchainContext; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.RunfilesProvider; @@ -688,22 +687,7 @@ public Dict var() throws EvalException { @Override public ToolchainContextApi toolchains() throws EvalException { checkMutable("toolchains"); - ResolvedToolchainContext toolchainContext = ruleContext.getToolchainContext(); - if (toolchainContext == null) { - // Starlark rules are easier if this cannot be null, so return a no-op value instead. - return new ToolchainContextApi() { - @Override - public Object getIndex(StarlarkSemantics semantics, Object key) { - return Starlark.NONE; - } - - @Override - public boolean containsKey(StarlarkSemantics semantics, Object key) { - return false; - } - }; - } - return toolchainContext; + return StarlarkToolchainContext.create(ruleContext.getToolchainContext()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkToolchainContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkToolchainContext.java new file mode 100644 index 00000000000000..d195acc78ee122 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkToolchainContext.java @@ -0,0 +1,127 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.starlark; + +import static java.util.stream.Collectors.joining; + +import com.google.auto.value.AutoValue; +import com.google.devtools.build.lib.analysis.ResolvedToolchainContext; +import com.google.devtools.build.lib.analysis.platform.ToolchainInfo; +import com.google.devtools.build.lib.analysis.platform.ToolchainTypeInfo; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.starlarkbuildapi.platform.ToolchainContextApi; +import javax.annotation.Nullable; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Printer; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkSemantics; +import net.starlark.java.eval.StarlarkThread; + +/** + * An implementation of ToolchainContextApi that can better handle converting strings into Labels. + */ +@AutoValue +public abstract class StarlarkToolchainContext implements ToolchainContextApi { + + private static final ToolchainContextApi NO_OP = + new ToolchainContextApi() { + @Override + public Object getIndex( + StarlarkThread starlarkThread, StarlarkSemantics semantics, Object key) { + // TODO(jcater): throw Starlark.errorf instead of returning NONE. + return Starlark.NONE; + } + + @Override + public boolean containsKey( + StarlarkThread starlarkThread, StarlarkSemantics semantics, Object key) { + return false; + } + }; + + public static ToolchainContextApi create(@Nullable ResolvedToolchainContext toolchainContext) { + if (toolchainContext == null) { + return NO_OP; + } + + return new AutoValue_StarlarkToolchainContext(toolchainContext); + } + + protected abstract ResolvedToolchainContext toolchainContext(); + + @Override + public boolean isImmutable() { + return true; + } + + @Override + public void repr(Printer printer) { + printer.append(""); + } + + private Label transformKey(StarlarkThread starlarkThread, Object key) throws EvalException { + if (key instanceof Label) { + return (Label) key; + } else if (key instanceof ToolchainTypeInfo) { + return ((ToolchainTypeInfo) key).typeLabel(); + } else if (key instanceof String) { + try { + // TODO(jcater): Use LabelConversionContext and StarlarkThread to convert this instead. + // Also remove repoMapping from ResolvedToolchainContext. + return Label.parseAbsolute((String) key, toolchainContext().repoMapping()); + } catch (LabelSyntaxException e) { + throw Starlark.errorf("Unable to parse toolchain label '%s': %s", key, e.getMessage()); + } + } else { + throw Starlark.errorf( + "Toolchains only supports indexing by toolchain type, got %s instead", + Starlark.type(key)); + } + } + + @Override + public ToolchainInfo getIndex( + StarlarkThread starlarkThread, StarlarkSemantics semantics, Object key) throws EvalException { + Label toolchainTypeLabel = transformKey(starlarkThread, key); + + if (!containsKey(starlarkThread, semantics, key)) { + // TODO(bazel-configurability): The list of available toolchain types is confusing in the + // presence of aliases, since it only contains the actual label, not the alias passed to the + // rule definition. + throw Starlark.errorf( + "In %s, toolchain type %s was requested but only types [%s] are configured", + toolchainContext().targetDescription(), + toolchainTypeLabel, + toolchainContext().requiredToolchainTypes().stream() + .map(ToolchainTypeInfo::typeLabel) + .map(Label::toString) + .collect(joining(", "))); + } + return toolchainContext().forToolchainType(toolchainTypeLabel); + } + + @Override + public boolean containsKey(StarlarkThread starlarkThread, StarlarkSemantics semantics, Object key) + throws EvalException { + Label toolchainTypeLabel = transformKey(starlarkThread, key); + return toolchainContext().requestedToolchainTypeLabels().containsKey(toolchainTypeLabel); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform/ToolchainContextApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform/ToolchainContextApi.java index 227ed4c825511d..f98f7d03c379bb 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform/ToolchainContextApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform/ToolchainContextApi.java @@ -27,4 +27,4 @@ "Holds toolchains available for a particular exec group. Toolchain targets are accessed by" + " indexing with the toolchain type, as in" + " context[\"//pkg:my_toolchain_type\"].") -public interface ToolchainContextApi extends StarlarkValue, StarlarkIndexable {} +public interface ToolchainContextApi extends StarlarkValue, StarlarkIndexable.Threaded {}