Refactor artifact_name_pattern in CROSSTOOL

Instead of using a string pattern, we replace it with a prefix and an
extension.

RELNOTES: NONE
PiperOrigin-RevId: 197132215

7 files changed, 57 insertions, 79 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/ArtifactCategory.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/ArtifactCategory.java
index 836c62e8d5..09e4f212e4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/ArtifactCategory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/ArtifactCategory.java
@@ -18,30 +18,32 @@ package com.google.devtools.build.lib.rules.cpp;
  * select a single artifact.
  */
 public enum ArtifactCategory {
-  STATIC_LIBRARY("lib%{base_name}.a"),
-  ALWAYSLINK_STATIC_LIBRARY("lib%{base_name}.lo"),
-  DYNAMIC_LIBRARY("lib%{base_name}.so"),
-  EXECUTABLE("%{base_name}"),
-  INTERFACE_LIBRARY("lib%{base_name}.ifso"),
-  PIC_FILE("%{output_name}.pic"),
-  INCLUDED_FILE_LIST("%{output_name}.d"),
-  OBJECT_FILE("%{output_name}.o"),
-  PIC_OBJECT_FILE("%{output_name}.pic.o"),
-  CPP_MODULE("%{output_name}.pcm"),
-  GENERATED_ASSEMBLY("%{output_name}.s"),
-  PROCESSED_HEADER("%{output_name}.processed"),
-  GENERATED_HEADER("%{output_name}.h"),
-  PREPROCESSED_C_SOURCE("%{output_name}.i"),
-  PREPROCESSED_CPP_SOURCE("%{output_name}.ii"),
-  COVERAGE_DATA_FILE("%{output_name}.gcno"),
+  STATIC_LIBRARY("lib", ".a"),
+  ALWAYSLINK_STATIC_LIBRARY("lib", ".lo"),
+  DYNAMIC_LIBRARY("lib", ".so"),
+  EXECUTABLE("", ""),
+  INTERFACE_LIBRARY("lib", ".ifso"),
+  PIC_FILE("", ".pic"),
+  INCLUDED_FILE_LIST("", ".d"),
+  OBJECT_FILE("", ".o"),
+  PIC_OBJECT_FILE("", ".pic.o"),
+  CPP_MODULE("", ".pcm"),
+  GENERATED_ASSEMBLY("", ".s"),
+  PROCESSED_HEADER("", ".processed"),
+  GENERATED_HEADER("", ".h"),
+  PREPROCESSED_C_SOURCE("", ".i"),
+  PREPROCESSED_CPP_SOURCE("", ".ii"),
+  COVERAGE_DATA_FILE("", ".gcno"),
   // A matched-clif protobuf. Typically in binary format, but could be text depending on
   // the options passed to the clif_matcher.
-  CLIF_OUTPUT_PROTO("%{output_name}.opb");
+  CLIF_OUTPUT_PROTO("", ".opb");
 
-  private final String defaultPattern;
+  private final String defaultPrefix;
+  private final String defaultExtension;
 
-  ArtifactCategory(String defaultPattern) {
-    this.defaultPattern = defaultPattern;
+  ArtifactCategory(String prefix, String extension) {
+    this.defaultPrefix = prefix;
+    this.defaultExtension = extension;
   }
 
   /** Returns the name of the category. */
@@ -49,7 +51,11 @@ public enum ArtifactCategory {
     return this.toString().toLowerCase();
   }
 
-  public String getDefaultPattern() {
-    return defaultPattern;
+  public String getDefaultPrefix() {
+    return defaultPrefix;
+  }
+
+  public String getDefaultExtension() {
+    return defaultExtension;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
index 5091a3692e..e6a53e362e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -48,7 +48,6 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -937,7 +936,8 @@ public class CcToolchainFeatures implements Serializable {
   private static class ArtifactNamePattern {
 
     private final ArtifactCategory artifactCategory;
-    private final ImmutableList<StringChunk> chunks;
+    private final String prefix;
+    private final String extension;
 
     private ArtifactNamePattern(CToolchain.ArtifactNamePattern artifactNamePattern)
         throws InvalidConfigurationException {
@@ -955,9 +955,8 @@ public class CcToolchainFeatures implements Serializable {
                 artifactNamePattern.getCategoryName()));
       }
       this.artifactCategory = foundCategory;
-      
-      StringValueParser parser = new StringValueParser(artifactNamePattern.getPattern());
-      this.chunks = parser.getChunks();
+      this.prefix = artifactNamePattern.getPrefix();
+      this.extension = artifactNamePattern.getExtension();
     }
 
     /** Returns the ArtifactCategory for this ArtifactNamePattern. */
@@ -965,18 +964,9 @@ public class CcToolchainFeatures implements Serializable {
       return this.artifactCategory;
     }
 
-    /**
-     * Returns the artifact name that this pattern selects.
-     */
-    public String getArtifactName(Map<String, String> variables) {
-      StringBuilder resultBuilder = new StringBuilder();
-      CcToolchainVariables artifactNameVariables =
-          new CcToolchainVariables.Builder().addAllStringVariables(variables).build();
-      for (StringChunk chunk : chunks) {
-        resultBuilder.append(chunk.expand(artifactNameVariables));
-      }
-      String result = resultBuilder.toString();
-      return result.charAt(0) == '/' ? result.substring(1) : result;
+    /** Returns the artifact name that this pattern selects. */
+    public String getArtifactName(String baseName) {
+      return prefix + baseName + extension;
     }
   }
 
@@ -1457,10 +1447,8 @@ public class CcToolchainFeatures implements Serializable {
               MISSING_ARTIFACT_NAME_PATTERN_ERROR_TEMPLATE, artifactCategory.getCategoryName()));
     }
 
-    return patternForCategory.getArtifactName(ImmutableMap.of(
-        "output_name", outputName,
-        "base_name", output.getBaseName(),
-        "output_directory", output.getParentDirectory().getPathString()));
+    return output.getParentDirectory()
+        .getChild(patternForCategory.getArtifactName(output.getBaseName())).getPathString();
   }
 
   /** Returns true if the toolchain defines an ArtifactNamePattern for the given category. */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java
index bd67bbb36e..f9aa787622 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java
@@ -345,11 +345,13 @@ public final class CppToolchainInfo {
     }
 
     for (ArtifactCategory category : ArtifactCategory.values()) {
-      if (!definedCategories.contains(category) && category.getDefaultPattern() != null) {
+      if (!definedCategories.contains(category) && category.getDefaultPrefix() != null
+          && category.getDefaultExtension() != null) {
         toolchainBuilder.addArtifactNamePattern(
             ArtifactNamePattern.newBuilder()
                 .setCategoryName(category.toString().toLowerCase())
-                .setPattern(category.getDefaultPattern())
+                .setPrefix(category.getDefaultPrefix())
+                .setExtension(category.getDefaultExtension())
                 .build());
       }
     }
diff --git a/src/main/protobuf/crosstool_config.proto b/src/main/protobuf/crosstool_config.proto
index 7323d75526..3ad7c7a0e1 100644
--- a/src/main/protobuf/crosstool_config.proto
+++ b/src/main/protobuf/crosstool_config.proto
@@ -260,10 +260,10 @@ message CToolchain {
     // categories include "linked_output" or "debug_symbols". An error is thrown
     // if no category is matched.
     required string category_name = 1;
-    // The pattern for creating the artifact for this selection.  The given
-    // pattern is templated by bazel and then used to create an artifact in
-    // a target-specific directory in bazel-bin.
-    required string pattern = 2;
+    // The prefix and extension for creating the artifact for this selection.
+    // They are used to create an artifact name based on the target name.
+    required string prefix = 2;
+    required string extension = 3;
   }
 
   // An action config corresponds to a blaze action, and allows selection of
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
index 64ed3c068d..4cc1620711 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
@@ -477,21 +477,16 @@ public abstract class MockCcSupport {
       ""
           + "artifact_name_pattern {"
           + "   category_name: 'static_library'"
-          + "   pattern: 'lib%{base_name}.tweaked.a'"
+          + "   prefix: 'lib'"
+          + "   extension: '.tweaked.a'"
           + "}";
 
   public static final String STATIC_LINK_AS_DOT_A_CONFIGURATION =
       ""
           + "artifact_name_pattern {"
           + "   category_name: 'static_library'"
-          + "   pattern: 'lib%{base_name}.a'"
-          + "}";
-
-  public static final String STATIC_LINK_BAD_TEMPLATE_CONFIGURATION =
-      ""
-          + "artifact_name_pattern {"
-          + "   category_name: 'static_library'"
-          + "   pattern: 'foo%{bad_variable}bar'"
+          + "   prefix: 'lib'"
+          + "   extension: '.a'"
           + "}";
 
   public static final String EMPTY_COMPILE_ACTION_CONFIG =
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 14779030d9..dd864bc08c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.rules.cpp;
 
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
 
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
@@ -359,10 +358,12 @@ public class CcLibraryConfiguredTargetTest extends BuildViewTestCase {
   public void testObjectFileNamesCanBeSpecifiedInToolchain() throws Exception {
     AnalysisMock.get()
         .ccSupport()
-        .setupCrosstool(mockToolsConfig,
+        .setupCrosstool(
+            mockToolsConfig,
             "artifact_name_pattern {"
                 + "   category_name: 'object_file'"
-                + "   pattern: '%{output_name}.test.o'"
+                + "   prefix: ''"
+                + "   extension: '.test.o'"
                 + "}");
 
     useConfiguration();
@@ -383,22 +384,6 @@ public class CcLibraryConfiguredTargetTest extends BuildViewTestCase {
   }
 
   @Test
-  public void testArtifactSelectionErrorOnBadTemplateVariable() throws Exception {
-    AnalysisMock.get()
-        .ccSupport()
-        .setupCrosstool(mockToolsConfig, MockCcSupport.STATIC_LINK_BAD_TEMPLATE_CONFIGURATION);
-    useConfiguration("--features=" + Link.LinkTargetType.STATIC_LIBRARY.getActionName());
-    try {
-      getConfiguredTarget("//hello:hello");
-      fail("Should fail");
-    } catch (AssertionError e) {
-      assertThat(e)
-          .hasMessageThat()
-          .contains("Invalid toolchain configuration: Cannot find variable named 'bad_variable'");
-    }
-  }
-
-  @Test
   public void testArtifactsToAlwaysBuild() throws Exception {
     useConfiguration("--cpu=k8");
     // ArtifactsToAlwaysBuild should apply both for static libraries.
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
index 6c04b605eb..a780425d10 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
@@ -1724,7 +1724,8 @@ public class CcToolchainFeaturesTest extends FoundationTestCase {
       buildFeatures(
           "artifact_name_pattern {",
           "category_name: 'NONEXISTENT_CATEGORY'",
-          "pattern: 'some_pattern'}");
+          "prefix: 'foo'",
+          "extension: 'bar'}");
       fail("Should throw InvalidConfigurationException.");
     } catch (InvalidConfigurationException e) {
       assertThat(e)
@@ -1740,7 +1741,8 @@ public class CcToolchainFeaturesTest extends FoundationTestCase {
           buildFeatures(
               "artifact_name_pattern {",
               "category_name: 'static_library'",
-              "pattern: 'some_pattern'}");
+              "prefix: 'foo'",
+              "extension: 'bar'}");
       toolchainFeatures.getArtifactNameForCategory(ArtifactCategory.DYNAMIC_LIBRARY, "output_name");
       fail("Should throw InvalidConfigurationException.");
     } catch (InvalidConfigurationException e) {