Do not rely on package loading to get cc_toolchain_suite.proto

We can read the attribute value in the analysis phase just fine. RELNOTES: None. PiperOrigin-RevId: 222600298
bazelbuild · Nov 23, 2018 · 85ebae3 · 85ebae3
1 parent 2485ce0
commit 85ebae3
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 52 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -67,11 +67,11 @@ public ConfiguredTarget create(RuleContext ruleContext)
       return ruleConfiguredTargetBuilder.build();
     }
 
-    // This is either a legacy-package-loading-in-cpp-configuration-backed build, or a
-    // platforms-backed build, we will not analyze cc_toolchain_suite at all, and we are
+    // This is a platforms-backed build, we will not analyze cc_toolchain_suite at all, and we are
     // sure current cc_toolchain is the one selected. We can create CcToolchainProvider here.
     CcToolchainProvider ccToolchainProvider =
-        CcToolchainProviderHelper.getCcToolchainProvider(ruleContext, attributes);
+        CcToolchainProviderHelper.getCcToolchainProvider(
+            ruleContext, attributes, /* crosstoolFromCcToolchainSuiteProtoAttribute= */ null);
 
     if (ccToolchainProvider == null) {
       // Skyframe restart

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java
@@ -344,7 +344,9 @@ private static Artifact convertLLVMRawProfileToIndexed(
   }
 
   static CcToolchainProvider getCcToolchainProvider(
-      RuleContext ruleContext, CcToolchainAttributesProvider attributes)
+      RuleContext ruleContext,
+      CcToolchainAttributesProvider attributes,
+      CrosstoolRelease crosstoolFromCcToolchainSuiteProtoAttribute)
       throws RuleErrorException, InterruptedException {
     BuildConfiguration configuration = Preconditions.checkNotNull(ruleContext.getConfiguration());
     CppConfiguration cppConfiguration =
@@ -383,7 +385,7 @@ static CcToolchainProvider getCcToolchainProvider(
     // Is there a toolchain proto available on the target directly?
     CToolchain toolchain = parseToolchainFromAttributes(ruleContext, attributes);
     PackageIdentifier packageWithCrosstoolInIt = null;
-    if (toolchain == null && cppConfiguration.getCrosstoolFromCcToolchainProtoAttribute() == null) {
+    if (toolchain == null && crosstoolFromCcToolchainSuiteProtoAttribute == null) {
       packageWithCrosstoolInIt = ruleContext.getLabel().getPackageIdentifier();
     }
 
@@ -437,7 +439,12 @@ static CcToolchainProvider getCcToolchainProvider(
 
     CppToolchainInfo toolchainInfo =
         getCppToolchainInfo(
-            ruleContext, cppConfiguration, attributes, ccSkyframeSupportValue, toolchain);
+            ruleContext,
+            cppConfiguration,
+            attributes,
+            ccSkyframeSupportValue,
+            toolchain,
+            crosstoolFromCcToolchainSuiteProtoAttribute);
 
     String purposePrefix = attributes.getPurposePrefix();
     String runtimeSolibDirBase = attributes.getRuntimeSolibDirBase();
@@ -654,7 +661,8 @@ private static CppToolchainInfo getCppToolchainInfo(
       CppConfiguration cppConfiguration,
       CcToolchainAttributesProvider attributes,
       CcSkyframeSupportValue ccSkyframeSupportValue,
-      CToolchain toolchainFromCcToolchainAttribute)
+      CToolchain toolchainFromCcToolchainAttribute,
+      CrosstoolRelease crosstoolFromCcToolchainSuiteProtoAttribute)
       throws RuleErrorException {
 
     if (cppConfiguration.enableCcToolchainConfigInfoFromSkylark()) {
@@ -681,7 +689,11 @@ private static CppToolchainInfo getCppToolchainInfo(
     if (toolchain == null) {
       toolchain =
           getToolchainFromAttributes(
-              ruleContext, attributes, cppConfiguration, ccSkyframeSupportValue);
+              ruleContext,
+              attributes,
+              cppConfiguration,
+              crosstoolFromCcToolchainSuiteProtoAttribute,
+              ccSkyframeSupportValue);
     }
 
     // If we found a toolchain, use it.
@@ -749,13 +761,14 @@ private static CToolchain getToolchainFromAttributes(
       RuleContext ruleContext,
       CcToolchainAttributesProvider attributes,
       CppConfiguration cppConfiguration,
+      CrosstoolRelease crosstoolFromCcToolchainSuiteProtoAttribute,
       CcSkyframeSupportValue ccSkyframeSupportValue)
       throws RuleErrorException {
     try {
       CrosstoolRelease crosstoolRelease;
-      if (cppConfiguration.getCrosstoolFromCcToolchainProtoAttribute() != null) {
+      if (crosstoolFromCcToolchainSuiteProtoAttribute != null) {
         // We have cc_toolchain_suite.proto attribute set, let's use it
-        crosstoolRelease = cppConfiguration.getCrosstoolFromCcToolchainProtoAttribute();
+        crosstoolRelease = crosstoolFromCcToolchainSuiteProtoAttribute;
       } else {
         // We use the proto from the CROSSTOOL file
         crosstoolRelease = ccSkyframeSupportValue.getCrosstoolRelease();

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuite.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuite.java
@@ -27,9 +27,13 @@
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.TemplateVariableInfo;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
+import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease;
 import java.util.Map;
 
 /**
@@ -51,6 +55,18 @@ public ConfiguredTarget create(RuleContext ruleContext)
     String key = transformedCpu + (compiler == null ? "" : ("|" + compiler));
     Map<String, Label> toolchains =
         ruleContext.attributes().get("toolchains", BuildType.LABEL_DICT_UNARY);
+    CrosstoolRelease crosstoolFromProtoAttribute = null;
+    if (ruleContext.attributes().isAttributeValueExplicitlySpecified("proto")) {
+      try {
+        crosstoolFromProtoAttribute =
+            CrosstoolConfigurationLoader.toReleaseConfiguration(
+                "cc_toolchain_suite rule " + ruleContext.getLabel(),
+                () -> ruleContext.attributes().get("proto", Type.STRING),
+                /* digestOrNull= */ null);
+      } catch (InvalidConfigurationException e) {
+        ruleContext.throwWithRuleError(e.getMessage());
+      }
+    }
     Label selectedCcToolchain = toolchains.get(key);
     CcToolchainProvider ccToolchainProvider;
     PlatformConfiguration platformConfig =
@@ -78,7 +94,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
               compiler,
               selectedCcToolchain);
       ccToolchainProvider =
-          CcToolchainProviderHelper.getCcToolchainProvider(ruleContext, selectedAttributes);
+          CcToolchainProviderHelper.getCcToolchainProvider(
+              ruleContext, selectedAttributes, crosstoolFromProtoAttribute);
 
       if (ccToolchainProvider == null) {
         // Skyframe restart

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -38,7 +38,6 @@
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig;
-import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CrosstoolRelease;
 import javax.annotation.Nullable;
 
 /**
@@ -172,13 +171,6 @@ public String toString() {
   public static final String FDO_STAMP_MACRO = "BUILD_FDO_TYPE";
 
   private final Label crosstoolTop;
-  /**
-   * cc_toolchain_suite allows to override CROSSTOOL by using proto attribute. This attribute value
-   * is stored here so cc_toolchain can access it in the analysis. Don't use this for anything, it
-   * will be removed when b/113849758 is fixed. If you do, I'll send bubo to take your keyboard
-   * away.
-   */
-  @Deprecated private final CrosstoolRelease crosstoolFromCcToolchainProtoAttribute;
 
   private final String transformedCpuFromOptions;
   private final String compilerFromOptions;
@@ -235,7 +227,6 @@ static CppConfiguration create(CppConfigurationParameters params)
 
     return new CppConfiguration(
         params.crosstoolTop,
-        params.crosstoolFromCcToolchainProtoAttribute,
         params.transformedCpu,
         params.compiler,
         Preconditions.checkNotNull(params.commonOptions.cpu),
@@ -257,7 +248,6 @@ static CppConfiguration create(CppConfigurationParameters params)
 
   private CppConfiguration(
       Label crosstoolTop,
-      CrosstoolRelease crosstoolFromCcToolchainProtoAttribute,
       String transformedCpuFromOptions,
       String compilerFromOptions,
       String desiredCpu,
@@ -274,7 +264,6 @@ private CppConfiguration(
       CompilationMode compilationMode,
       CppToolchainInfo cppToolchainInfo) {
     this.crosstoolTop = crosstoolTop;
-    this.crosstoolFromCcToolchainProtoAttribute = crosstoolFromCcToolchainProtoAttribute;
     this.transformedCpuFromOptions = transformedCpuFromOptions;
     this.compilerFromOptions = compilerFromOptions;
     this.desiredCpu = desiredCpu;
@@ -669,17 +658,6 @@ public boolean disableLinkingModeFlags() {
     return cppOptions.disableLinkingModeFlags;
   }
 
-  /**
-   * cc_toolchain_suite allows to override CROSSTOOL by using proto attribute. This attribute value
-   * is stored here so cc_toolchain can access it in the analysis. Don't use this for anything, it
-   * will be removed when b/113849758 is fixed. If you do, I'll send bubo to take your keyboard
-   * away.
-   */
-  @Deprecated
-  public CrosstoolRelease getCrosstoolFromCcToolchainProtoAttribute() {
-    return crosstoolFromCcToolchainProtoAttribute;
-  }
-
   public boolean enableLinkoptsInUserLinkFlags() {
     return cppOptions.enableLinkoptsInUserLinkFlags;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.rules.cpp;
 
-import static com.google.devtools.build.lib.rules.cpp.CrosstoolConfigurationLoader.toReleaseConfiguration;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -36,7 +35,6 @@
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.util.StringUtil;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain;
@@ -91,12 +89,10 @@ public static class CppConfigurationParameters {
     protected final CcToolchainConfigInfo ccToolchainConfigInfo;
     protected final String transformedCpu;
     protected final String compiler;
-    protected final CrosstoolRelease crosstoolFromCcToolchainProtoAttribute;
 
     CppConfigurationParameters(
         String transformedCpu,
         String compiler,
-        CrosstoolRelease crosstoolFromCcToolchainProtoAttribute,
         BuildOptions buildOptions,
         PathFragment fdoPath,
         Label fdoOptimizeLabel,
@@ -105,7 +101,6 @@ public static class CppConfigurationParameters {
         CcToolchainConfigInfo ccToolchainConfigInfo) {
       this.transformedCpu = transformedCpu;
       this.compiler = compiler;
-      this.crosstoolFromCcToolchainProtoAttribute = crosstoolFromCcToolchainProtoAttribute;
       this.commonOptions = buildOptions.get(BuildConfiguration.Options.class);
       this.cppOptions = buildOptions.get(CppOptions.class);
       this.fdoPath = fdoPath;
@@ -197,19 +192,6 @@ protected CppConfigurationParameters createParameters(
       throw new InvalidConfigurationException(e);
     }
 
-    String ccToolchainSuiteProtoAttributeValue =
-        StringUtil.emptyToNull(
-            NonconfigurableAttributeMapper.of((Rule) crosstoolTop).get("proto", Type.STRING));
-
-    CrosstoolRelease crosstoolFromCcToolchainProtoAttribute = null;
-    if (ccToolchainSuiteProtoAttributeValue != null) {
-      crosstoolFromCcToolchainProtoAttribute =
-          toReleaseConfiguration(
-              "cc_toolchain_suite rule " + crosstoolTopLabel,
-              () -> ccToolchainSuiteProtoAttributeValue,
-              /* digestOrNull= */ null);
-    }
-
     PathFragment fdoPath = null;
     Label fdoProfileLabel = null;
     if (cppOptions.getFdoOptimize() != null) {
@@ -233,7 +215,6 @@ protected CppConfigurationParameters createParameters(
     return new CppConfigurationParameters(
         transformedCpu,
         cppOptions.cppCompiler,
-        crosstoolFromCcToolchainProtoAttribute,
         options,
         fdoPath,
         fdoProfileLabel,