Add OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH

See OptionMetadataTag.java for documentation on what the tag does. Also audited the transitive calls of OutputDirectories.buildMnemonic and annotated options that seemingly can safely have the tag. (Note that some, like AppleConfiguration, have a getOutputDirectory that is too intricate for me to comfortably add the tag to any of the associated options.) Related to #14023. PiperOrigin-RevId: 410335434
bazelbuild · Nov 16, 2021 · 75a16b7 · 75a16b7
1 parent 063b5c9
commit 75a16b7
Show file tree

Hide file tree

Showing 10 changed files with 121 additions and 26 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java
@@ -99,6 +99,7 @@ public static boolean platformIsDefault(Label platform) {
         OptionEffectTag.CHANGES_INPUTS,
         OptionEffectTag.LOADING_AND_ANALYSIS
       },
+      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help =
           "The labels of the platform rules describing the target platforms for the current "
               + "command.")

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
@@ -117,6 +117,7 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       converter = AutoCpuConverter.class,
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS},
+      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help = "The target CPU.")
   public String cpu;
 
@@ -226,6 +227,7 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
       defaultValue = "fastbuild",
       documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
       effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.ACTION_COMMAND_LINES},
+      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help = "Specify the mode the binary will be built in. Values: 'fastbuild', 'dbg', 'opt'.")
   public CompilationMode compilationMode;
 

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -17,6 +17,7 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX;
 import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
+import static java.util.Arrays.stream;
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.Joiner;
@@ -36,6 +37,7 @@
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.common.options.OptionDefinition;
+import com.google.devtools.common.options.OptionMetadataTag;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.lang.reflect.Field;
@@ -277,31 +279,32 @@ private static BuildOptions applyTransition(
     // toOptions being null means the transition hasn't changed anything. We avoid preemptively
     // cloning it from fromOptions since options cloning is an expensive operation.
     BuildOptions toOptions = null;
-    // The names and values of options (Starlark + native) that are different after this transition.
-    Set<String> convertedNewValues = new HashSet<>();
+    // The names of options (Starlark + native) that are different after this transition and must
+    //   be added to "affected by Starlark transition"
+    Set<String> convertedAffectedOptions = new HashSet<>();
     // Starlark options that are different after this transition. We collect all of them, then clone
     // the build options once with all cumulative changes. Native option changes, in contrast, are
     // set directly in the BuildOptions instance. The former approach is preferred since it makes
     // BuildOptions objects more immutable. Native options use the latter approach for legacy
     // reasons. While not preferred, direct mutation doesn't require expensive cloning.
     Map<Label, Object> changedStarlarkOptions = new LinkedHashMap<>();
     for (Map.Entry<String, Object> entry : newValues.entrySet()) {
-      String optionName = entry.getKey();
+      String optionKey = entry.getKey();
       Object optionValue = entry.getValue();
 
-      if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
+      if (!optionKey.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
         // The transition changes a Starlark option.
-        Label optionLabel = Label.parseAbsoluteUnchecked(optionName);
+        Label optionLabel = Label.parseAbsoluteUnchecked(optionKey);
         Object oldValue = fromOptions.getStarlarkOptions().get(optionLabel);
         if ((oldValue == null && optionValue != null)
             || (oldValue != null && optionValue == null)
             || (oldValue != null && !oldValue.equals(optionValue))) {
           changedStarlarkOptions.put(optionLabel, optionValue);
-          convertedNewValues.add(optionLabel.toString());
+          convertedAffectedOptions.add(optionLabel.toString());
         }
       } else {
         // The transition changes a native option.
-        optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());
+        String optionName = optionKey.substring(COMMAND_LINE_OPTION_PREFIX.length());
 
         // Convert NoneType to null.
         if (optionValue instanceof NoneType) {
@@ -363,7 +366,10 @@ private static BuildOptions applyTransition(
               toOptions = fromOptions.clone();
             }
             field.set(toOptions.get(optionInfo.getOptionClass()), convertedValue);
-            convertedNewValues.add(entry.getKey());
+
+            if (!optionInfo.hasMetadataTag(OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH)) {
+              convertedAffectedOptions.add(optionKey);
+            }
           }
 
         } catch (IllegalArgumentException e) {
@@ -392,11 +398,11 @@ private static BuildOptions applyTransition(
     if (starlarkTransition.isForAnalysisTesting()) {
       // We need to record every time we change a configuration option.
       // see {@link #updateOutputDirectoryNameFragment} for usage.
-      convertedNewValues.add("//command_line_option:evaluating for analysis test");
+      convertedAffectedOptions.add("//command_line_option:evaluating for analysis test");
       toOptions.get(CoreOptions.class).evaluatingForAnalysisTest = true;
     }
 
-    updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedNewValues);
+    updateAffectedByStarlarkTransition(toOptions.get(CoreOptions.class), convertedAffectedOptions);
     return toOptions;
   }
 
@@ -502,6 +508,10 @@ Class<? extends FragmentOptions> getOptionClass() {
     OptionDefinition getDefinition() {
       return definition;
     }
+
+    boolean hasMetadataTag(OptionMetadataTag tag) {
+      return stream(getDefinition().getOptionMetadataTags()).anyMatch(tag::equals);
+    }
   }
 
   private FunctionTransitionUtil() {}

diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
@@ -201,7 +201,7 @@ public static class Options extends FragmentOptions {
         converter = ConfigurationDistinguisherConverter.class,
         documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
         effectTags = OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION,
-        metadataTags = {OptionMetadataTag.INTERNAL})
+        metadataTags = {OptionMetadataTag.INTERNAL, OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH})
     public ConfigurationDistinguisher configurationDistinguisher;
 
     // TODO(blaze-configurability): Mark this as deprecated in favor of --android_platforms.

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -150,6 +150,7 @@ public String getTypeDescription() {
       defaultValue = "",
       documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
       effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help = "Specifies a suffix to be added to the configuration directory.")
   public String outputDirectoryTag;
 

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
@@ -151,6 +151,7 @@ public String getTypeDescription() {
         OptionEffectTag.LOADING_AND_ANALYSIS,
         OptionEffectTag.AFFECTS_OUTPUTS // because of "-py2"/"-py3" output root
       },
+      metadataTags = {OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH},
       help =
           "The Python major version mode, either `PY2` or `PY3`. Note that this is overridden by "
               + "`py_binary` and `py_test` targets (even if they don't explicitly specify a "

diff --git a/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java b/src/main/java/com/google/devtools/common/options/OptionFilterDescriptions.java
@@ -181,7 +181,10 @@ public static ImmutableMap<OptionMetadataTag, String> getOptionMetadataTagDescri
             "This option should not be used by a user, and should not be logged.")
         .put(
             OptionMetadataTag.INTERNAL, // Here for completeness, these options are UNDOCUMENTED.
-            "This option isn't even a option, and should not be logged.");
+            "This option isn't even a option, and should not be logged.")
+        .put(
+            OptionMetadataTag.EXPLICIT_IN_OUTPUT_PATH,
+            "This option is explicitly mentioned in the output directory.");
     return effectTagDescriptionBuilder.build();
   }
 }
diff --git a/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java b/src/main/java/com/google/devtools/common/options/OptionMetadataTag.java
@@ -45,20 +45,35 @@ public enum OptionMetadataTag {
    * These are flags that should never be set by a user. This tag is used to make sure that options
    * that form the protocol between the client and the server are not logged.
    *
-   * <p>These should be in category {@code OptionDocumentationCategory.UNDOCUMENTED}.
+   * <p>These should be in category {@link OptionDocumentationCategory.UNDOCUMENTED}.
    */
   HIDDEN(3),
 
   /**
    * Options which are INTERNAL are not recognized by the parser at all, and so cannot be used as
    * flags.
    *
-   * <p>These should be in category {@code OptionDocumentationCategory.UNDOCUMENTED}.
+   * <p>These should be in category {@link OptionDocumentationCategory.UNDOCUMENTED}.
    */
-  INTERNAL(4);
+  INTERNAL(4),
 
   // reserved TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES(5)
 
+  /**
+   * Options which are EXPLICIT_IN_OUTPUT_PATH are explicitly included in the output path by {@link
+   * OutputDirectories.buildMnemonic} (or indirectly in {@link Fragment.getOutputDirectoryName}) and
+   * thus should not be included in the hash of changed options used to generically disambiguate
+   * output directories of different configurations. (See {@link
+   * FunctionTransitionUtil.computeOutputDirectoryNameFragment}.)
+   *
+   * <p>This tag should only be added to options that can guarantee that any change to that option
+   * corresponds to a change to {@link OutputDirectories.buildMnemonic}. Put mathematically, given
+   * any two BuildOptions instances A and B with respective values for the marked option a and b
+   * (and all other options are the same): {@code a == b iff OutputDirectories.buildMnemonic(A) ==
+   * OutputDirectories.buildMnemonic(B)}
+   */
+  EXPLICIT_IN_OUTPUT_PATH(6);
+
   private final int value;
 
   OptionMetadataTag(int value) {

diff --git a/src/main/protobuf/option_filters.proto b/src/main/protobuf/option_filters.proto
@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 syntax = "proto3";
+
 package options;
 
 // option java_api_version = 2;
@@ -54,4 +55,5 @@ enum OptionMetadataTag {
   INTERNAL = 4;
   reserved "TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES";
   reserved 5;
+  EXPLICIT_IN_OUTPUT_PATH = 6;
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -1121,6 +1121,22 @@ private void writeRulesWithAttrTransitionBzl() throws Exception {
         ")");
   }
 
+  private CoreOptions getCoreOptions(ConfiguredTarget target) {
+    return getConfiguration(target).getOptions().get(CoreOptions.class);
+  }
+
+  private ImmutableMap<Label, Object> getStarlarkOptions(ConfiguredTarget target) {
+    return getConfiguration(target).getOptions().getStarlarkOptions();
+  }
+
+  private Object getStarlarkOption(ConfiguredTarget target, String absName) {
+    return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName));
+  }
+
+  private String getTransitionDirectoryNameFragment(ConfiguredTarget target) {
+    return getConfiguration(target).getTransitionDirectoryNameFragment();
+  }
+
   @Test
   public void testTransitionOnBuildSetting_fromDefault() throws Exception {
     writeAllowlistFile();
@@ -1297,20 +1313,64 @@ public void testTransitionOnBuildSetting_onlyTransitionsAffectsDirectory() throw
                 ImmutableList.of("//test/starlark:the-answer=42")));
   }
 
-  private CoreOptions getCoreOptions(ConfiguredTarget target) {
-    return getConfiguration(target).getOptions().get(CoreOptions.class);
-  }
+  @Test
+  public void testTransitionOnCompilationMode_hasNoHash() throws Exception {
+    writeAllowlistFile();
+    writeBuildSettingsBzl();
+    scratch.file(
+        "test/starlark/rules.bzl",
+        "load('//myinfo:myinfo.bzl', 'MyInfo')",
+        "def _transition_impl(settings, attr):",
+        "  return {",
+        "    '//command_line_option:compilation_mode': attr.cmode_for_dep,",
+        "}",
+        "my_transition = transition(",
+        "  implementation = _transition_impl,",
+        "  inputs = [],",
+        "  outputs = ['//command_line_option:compilation_mode']",
+        ")",
+        "def _rule_impl(ctx):",
+        "  return MyInfo(dep = ctx.attr.dep)",
+        "my_rule = rule(",
+        "  implementation = _rule_impl,",
+        "  attrs = {",
+        "    'dep': attr.label(cfg = my_transition),",
+        "    'cmode_for_dep': attr.string(),",
+        "    '_allowlist_function_transition': attr.label(",
+        "      default = '//tools/allowlists/function_transition_allowlist'),",
+        "  }",
+        ")");
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:rules.bzl', 'my_rule')",
+        "load('//test/starlark:build_settings.bzl', 'int_flag')",
+        "my_rule(name = 'test', dep = ':dep1', cmode_for_dep='opt')",
+        "my_rule(name = 'dep1', dep = ':dep2', cmode_for_dep='fastbuild')",
+        "my_rule(name = 'dep2')");
+    useConfiguration(ImmutableMap.of(), "--compilation_mode=fastbuild");
 
-  private ImmutableMap<Label, Object> getStarlarkOptions(ConfiguredTarget target) {
-    return getConfiguration(target).getOptions().getStarlarkOptions();
-  }
+    ConfiguredTarget test = getConfiguredTarget("//test/starlark:test");
 
-  private Object getStarlarkOption(ConfiguredTarget target, String absName) {
-    return getStarlarkOptions(target).get(Label.parseAbsoluteUnchecked(absName));
-  }
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep1 =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(test).getValue("dep"));
 
-  private String getTransitionDirectoryNameFragment(ConfiguredTarget target) {
-    return getConfiguration(target).getTransitionDirectoryNameFragment();
+    @SuppressWarnings("unchecked")
+    ConfiguredTarget dep2 =
+        Iterables.getOnlyElement(
+            (List<ConfiguredTarget>) getMyInfoFromTarget(dep1).getValue("dep"));
+
+    // Assert transitionDirectoryNameFragment is empty for all configurations
+    assertThat(getTransitionDirectoryNameFragment(test)).isEmpty();
+    assertThat(getTransitionDirectoryNameFragment(dep1)).isEmpty();
+    assertThat(getTransitionDirectoryNameFragment(dep2)).isEmpty();
+
+    // test and dep1 should have different configurations b/c compilation_mode changed
+    assertThat(getConfiguration(test)).isNotEqualTo(getConfiguration(dep1));
+
+    // test and dep2 should have identical configurations
+    assertThat(getConfiguration(test)).isEqualTo(getConfiguration(dep2));
   }
 
   @Test