Remove CrosstoolConfigurationLoader

Also remove special cache for crosstools, they are now fully managed by the Skyframe caches in the same way as any other file is managed. #6072 RELNOTES: None. PiperOrigin-RevId: 223147737
bazelbuild · Nov 28, 2018 · 922f1b8 · 922f1b8
1 parent 3f174a7
commit 922f1b8
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 189 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportFunction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeSupportFunction.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.cpp;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -29,6 +30,9 @@
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import com.google.protobuf.TextFormat;
+import com.google.protobuf.TextFormat.ParseException;
+import com.google.protobuf.UninitializedMessageException;
 import java.io.IOException;
 import java.io.InputStream;
 import javax.annotation.Nullable;
@@ -51,6 +55,7 @@
  */
 public class CcSkyframeSupportFunction implements SkyFunction {
 
+  static final String CROSSTOOL_CONFIGURATION_FILENAME = "CROSSTOOL";
   private final BlazeDirectories directories;
 
   public CcSkyframeSupportFunction(BlazeDirectories directories) {
@@ -80,9 +85,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
         // 2. Get crosstool file (FileValue)
         PathFragment crosstool =
-            packageIdentifier
-                .getPackageFragment()
-                .getRelative(CrosstoolConfigurationLoader.CROSSTOOL_CONFIGURATION_FILENAME);
+            packageIdentifier.getPackageFragment().getRelative(CROSSTOOL_CONFIGURATION_FILENAME);
         FileValue crosstoolFileValue =
             (FileValue)
                 env.getValue(
@@ -103,12 +106,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         }
         try (InputStream inputStream = crosstoolFile.getInputStream()) {
           String crosstoolContent = new String(FileSystemUtils.readContentAsLatin1(inputStream));
-          crosstoolRelease =
-              CrosstoolConfigurationLoader.toReleaseConfiguration(
-                  "CROSSTOOL file " + crosstool, () -> crosstoolContent, /* digestOrNull= */ null);
+          crosstoolRelease = toReleaseConfiguration(crosstoolContent);
         }
       } catch (IOException | InvalidConfigurationException e) {
-        throw new CcSkyframeSupportException(e, key);
+        throw new CcSkyframeSupportException(e.getMessage(), key);
       }
     }
 
@@ -121,6 +122,30 @@ public String extractTag(SkyKey skyKey) {
     return null;
   }
 
+  /**
+   * Reads the given <code>crosstoolContent</code>, which must be in ascii format, into a protocol
+   * buffer.
+   *
+   * @param crosstoolContent for the error messages
+   */
+  @VisibleForTesting
+  static CrosstoolRelease toReleaseConfiguration(String crosstoolContent)
+      throws InvalidConfigurationException {
+    CrosstoolRelease.Builder builder = CrosstoolRelease.newBuilder();
+    try {
+      TextFormat.merge(crosstoolContent, builder);
+      return builder.build();
+    } catch (ParseException e) {
+      throw new InvalidConfigurationException(
+          "Could not read the CROSSTOOL file because of a parser error (" + e.getMessage() + ")");
+    } catch (UninitializedMessageException e) {
+      throw new InvalidConfigurationException(
+          "Could not read the CROSSTOOL file because of an incomplete protocol buffer ("
+              + e.getMessage()
+              + ")");
+    }
+  }
+
   /** Exception encapsulating IOExceptions thrown in {@link CcSkyframeSupportFunction} */
   public static class CcSkyframeSupportException extends SkyFunctionException {
 

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
@@ -58,10 +58,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
     if (ruleContext.attributes().isAttributeValueExplicitlySpecified("proto")) {
       try {
         crosstoolFromProtoAttribute =
-            CrosstoolConfigurationLoader.toReleaseConfiguration(
-                "cc_toolchain_suite rule " + ruleContext.getLabel(),
-                () -> ruleContext.attributes().get("proto", Type.STRING),
-                /* digestOrNull= */ null);
+            CcSkyframeSupportFunction.toReleaseConfiguration(
+                ruleContext.attributes().get("proto", Type.STRING));
       } catch (InvalidConfigurationException e) {
         ruleContext.throwWithRuleError(e.getMessage());
       }

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
@@ -38,8 +38,7 @@ public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() {
   private final CpuTransformer cpuTransformer;
 
   /**
-   * Creates a new CrosstoolConfigurationLoader instance with the given configuration provider. The
-   * configuration provider is used to perform caller-specific configuration file lookup.
+   * Creates a new {@link CppConfigurationLoader} instance with the given {@link CpuTransformer}.
    */
   public CppConfigurationLoader(CpuTransformer cpuTransformer) {
     this.cpuTransformer = cpuTransformer;

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoader.java
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSelectionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSelectionTest.java
@@ -26,7 +26,6 @@
 import com.google.devtools.build.lib.packages.util.MockPlatformSupport;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.ToolPath;
-import java.util.List;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -42,15 +41,6 @@ public void setup() throws Exception {
         mockToolsConfig, analysisMock.ccSupport().getMockCrosstoolLabel());
   }
 
-  private CppCompileAction getCppCompileAction(String label) throws Exception {
-    ConfiguredTarget target = getConfiguredTarget(label);
-    List<CppCompileAction> compilationSteps =
-        actionsTestUtil()
-            .findTransitivePrerequisitesOf(
-                getFilesToBuild(target).iterator().next(), CppCompileAction.class);
-    return compilationSteps.get(0);
-  }
-
   private static final String CPP_TOOLCHAIN_TYPE =
       TestConstants.TOOLS_REPOSITORY + "//tools/cpp:toolchain_type";
 

diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CrosstoolConfigurationLoaderTest.java
@@ -15,14 +15,12 @@
 package com.google.devtools.build.lib.rules.cpp;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.TemplateVariableInfo;
 import com.google.devtools.build.lib.analysis.config.CompilationMode;
-import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
 import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -611,26 +609,6 @@ private void checkToolchainB(CppConfigurationLoader loader, String... args) thro
         .inOrder();
   }
 
-  private void assertStringStartsWith(String expected, String text) {
-    if (!text.startsWith(expected)) {
-      fail("expected <" + expected + ">, but got <" + text + ">");
-    }
-  }
-
-  @Test
-  public void testIncompleteFile() throws Exception {
-    try {
-      CrosstoolConfigurationLoader.getUncachedReleaseConfiguration(
-          "/CROSSTOOL", () -> "major_version: \"12\"");
-      fail();
-    } catch (InvalidConfigurationException e) {
-      assertStringStartsWith(
-          "Could not read the crosstool configuration file "
-              + "'/CROSSTOOL', because of an incomplete protocol buffer",
-          e.getMessage());
-    }
-  }
-
   /**
    * Returns a test crosstool config with the specified tool missing from the tool_path
    * set. Also allows injection of custom fields.
@@ -674,20 +652,6 @@ public void testNonFissionConfigWithMissingDwp() throws Exception {
     create(loader, "--cpu=banana_cpu");
   }
 
-  @Test
-  public void testInvalidFile() throws Exception {
-    try {
-      CrosstoolConfigurationLoader.getUncachedReleaseConfiguration(
-          "/CROSSTOOL", () -> "some xxx : yak \"");
-      fail();
-    } catch (InvalidConfigurationException e) {
-      assertStringStartsWith(
-          "Could not read the crosstool configuration file "
-              + "'/CROSSTOOL', because of a parser error",
-          e.getMessage());
-    }
-  }
-
   /**
    * Tests interpretation of static_runtimes_filegroup / dynamic_runtimes_filegroup.
    */