Flag sets: emit certain info & warning messages persistently.

Skyframe skips warnings and info messages on cache hits.

So instead of reporting them
inside `FlagSetFunction`, return them in `FlagSetValue` and let the caller emit
them.

PiperOrigin-RevId: 710146391
Change-Id: I3871804d024ad1d1fe3b808d275070f095c914d6

src/main/java/com/google/devtools/build/lib/analysis/Project.java

@@ -301,6 +301,8 @@ public static FlagSetValue modifyBuildOptionsWithFlagSets(
           "Cannot parse options: " + result.getError().getException().getMessage(),
           Code.INVALID_BUILD_OPTIONS);
     }
-    return (FlagSetValue) result.get(flagSetKey);
+    FlagSetValue value = (FlagSetValue) result.get(flagSetKey);
+    value.getPersistentMessages().forEach(eventHandler::handle);
+    return value;
   }
 }

src/main/java/com/google/devtools/build/lib/skyframe/config/BUILD

@@ -98,6 +98,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_options",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
+        "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",

src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetFunction.java

@@ -26,7 +26,6 @@
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.runtime.ConfigFlagDefinitions;
 import com.google.devtools.build.lib.skyframe.ProjectValue;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -96,17 +95,21 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                         key.getSclConfig())));
       }
       // --noenforce_project_configs. Nothing to do.
-      return FlagSetValue.create(ImmutableSet.of());
+      return FlagSetValue.create(ImmutableSet.of(), ImmutableSet.of());
     }
     ProjectValue projectValue =
         (ProjectValue) env.getValue(new ProjectValue.Key(key.getProjectFile()));
     if (projectValue == null) {
       return null;
     }
 
+    // Skyframe doesn't replay warnings or info messages on cache hits: see Event.storeForReplay and
+    // Reportable.storeForReplay. We want some flag set messages to be more persistent, so we
+    // return them in the Skyvalue for the caller to emit.
+    ImmutableSet.Builder<Event> persistentMessages = ImmutableSet.builder();
     ImmutableSet<String> sclConfigAsStarlarkList =
-        getSclConfig(key, projectValue, env.getListener());
-    return FlagSetValue.create(sclConfigAsStarlarkList);
+        getSclConfig(key, projectValue, persistentMessages);
+    return FlagSetValue.create(sclConfigAsStarlarkList, persistentMessages.build());
   }
 
   /**
@@ -115,7 +118,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
    */
   @SuppressWarnings("unchecked")
   private static ImmutableSet<String> getSclConfig(
-      FlagSetValue.Key key, ProjectValue sclContent, ExtendedEventHandler eventHandler)
+      FlagSetValue.Key key, ProjectValue sclContent, ImmutableSet.Builder<Event> persistentMessages)
       throws FlagSetFunctionException {
     Label projectFile = key.getProjectFile();
     String sclConfigName = key.getSclConfig();
@@ -215,9 +218,9 @@ sclConfigName, supportedConfigsDesc(projectFile, configs))),
         buildOptionsAsStrings,
         key.getUserOptions(),
         optionsToApply,
-        eventHandler,
+        persistentMessages,
         projectFile);
-    eventHandler.handle(
+    persistentMessages.add(
         Event.info(
             String.format(
                 "Applying flags from the config '%s' defined in %s: %s ",
@@ -365,7 +368,7 @@ private static void validateNoExtraFlagsSet(
       ImmutableList<String> buildOptionsAsStrings,
       ImmutableMap<String, String> userOptions,
       ImmutableSet<String> flagsFromSelectedConfig,
-      ExtendedEventHandler eventHandler,
+      ImmutableSet.Builder<Event> persistentMessages,
       Label projectFile)
       throws FlagSetFunctionException {
     ImmutableSet<String> overlap =
@@ -429,7 +432,7 @@ private static void validateNoExtraFlagsSet(
     }
     // This appears in the WARN case, or for a COMPATIBLE project file that doesn't have
     // conflicting flags. We never hit this in the STRICT case, since we've already thrown.
-    eventHandler.handle(
+    persistentMessages.add(
         Event.warn(
             String.format(
                 "This build uses a project file (%s), but also sets output-affecting"

src/main/java/com/google/devtools/build/lib/skyframe/config/FlagSetValue.java

@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.runtime.ConfigFlagDefinitions;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -35,6 +36,12 @@ public class FlagSetValue implements SkyValue {
 
   private final ImmutableSet<String> flags;
 
+  /**
+   * Warnings and info messages for the caller to emit. This lets the caller persistently emit
+   * messages that Skyframe ignores on cache hits. See {@link Reportable#storeForReplay}).
+   */
+  private final ImmutableSet<Event> persistentMessages;
+
   /** Key for {@link FlagSetValue} based on the raw flags. */
   @ThreadSafety.Immutable
   @AutoCodec
@@ -146,16 +153,22 @@ public int hashCode() {
     }
   }
 
-  public static FlagSetValue create(ImmutableSet<String> flags) {
-    return new FlagSetValue(flags);
+  public static FlagSetValue create(
+      ImmutableSet<String> flags, ImmutableSet<Event> persistentMessages) {
+    return new FlagSetValue(flags, persistentMessages);
   }
 
-  public FlagSetValue(ImmutableSet<String> flags) {
+  public FlagSetValue(ImmutableSet<String> flags, ImmutableSet<Event> persistentMessages) {
     this.flags = flags;
+    this.persistentMessages = persistentMessages;
   }
 
   /** Returns the set of flags to be applied to the build from the flagset, in flag=value form. */
   public ImmutableSet<String> getOptionsFromFlagset() {
     return flags;
   }
+
+  public ImmutableSet<Event> getPersistentMessages() {
+    return persistentMessages;
+  }
 }

src/test/java/com/google/devtools/build/lib/skyframe/config/BUILD

@@ -21,6 +21,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",

src/test/java/com/google/devtools/build/lib/skyframe/config/FlagSetsFunctionTest.java

@@ -23,6 +23,8 @@
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.analysis.util.DummyTestFragment;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
 import com.google.devtools.build.lib.runtime.ConfigFlagDefinitions;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
@@ -47,6 +49,26 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() {
     return builder.build();
   }
 
+  /**
+   * Asserts a {@link FlagSetValue} contains a given kind of event with a given message that
+   * occurred a given number of times.
+   *
+   * <p>Only applies to messages that are expected to persistently display, even on Skyframe cache
+   * hits: see {@link FlagSetValue#getPersistentMessages}.
+   */
+  private void assertContainsPersistentMessage(
+      FlagSetValue value, EventKind kind, int frequency, String message) {
+    int count = 0;
+    for (Event event : value.getPersistentMessages()) {
+      if (event.getKind() != kind) {
+        continue;
+      }
+      count++;
+      assertThat(event.getMessage()).contains(message);
+    }
+    assertThat(count).isEqualTo(frequency);
+  }
+
   /**
    * Given "//foo:myflag" and "default_value", creates the BUILD and .bzl files to realize a
    * string_flag with that label and default value.
@@ -311,10 +333,11 @@ public void enforceCanonicalConfigsValidConfig() throws Exception {
 
     assertThat(flagSetsValue.getOptionsFromFlagset())
         .contains("--//test:myflag=other_config_value");
-
-    assertContainsEvent("Applying flags from the config 'other_config'");
-    // TODO: b/380581463 - Reenable the frequency check once the initial event is deduplicated.
-    // assertContainsEventWithFrequency("Applying flags from the config 'other_config'", 1);
+    assertContainsPersistentMessage(
+        flagSetsValue,
+        EventKind.INFO,
+        /* frequency= */ 1,
+        "Applying flags from the config 'other_config'");
   }
 
   @Test
@@ -399,8 +422,10 @@ public void enforceCanonicalConfigsFlag_warnPolicy_passes() throws Exception {
             /* configFlagDefinitions= */ ConfigFlagDefinitions.NONE,
             /* enforceCanonical= */ true);
 
-    var unused = executeFunction(key);
-    assertContainsEvent(
+    assertContainsPersistentMessage(
+        executeFunction(key),
+        EventKind.WARNING,
+        /* frequency= */ 1,
         "also sets output-affecting flags in the command line or user bazelrc:"
             + " ['--define=foo=bar']");
   }
@@ -444,8 +469,10 @@ public void enforceCanonicalConfigsFlag_compatiblePolicy_unrelatedFlag_warns() t
             /* configFlagDefinitions= */ ConfigFlagDefinitions.NONE,
             /* enforceCanonical= */ true);
 
-    var unused = executeFunction(key);
-    assertContainsEvent(
+    assertContainsPersistentMessage(
+        executeFunction(key),
+        EventKind.WARNING,
+        /* frequency= */ 1,
         "also sets output-affecting flags in the command line or user bazelrc:"
             + " ['--define=foo=bar']");
   }
@@ -1053,10 +1080,11 @@ public void enforceCanonicalConfigsNoSclConfigFlagValidDefaultConfig() throws Ex
 
     assertThat(flagSetsValue.getOptionsFromFlagset())
         .containsExactly("--//test:myflag=test_config_value");
-
-    assertContainsEvent("Applying flags from the config 'test_config'");
-    // TODO: b/380581463 - Reenable the frequency check once the initial event is deduplicated.
-    // assertContainsEventWithFrequency("Applying flags from the config 'test_config'", 1);
+    assertContainsPersistentMessage(
+        flagSetsValue,
+        EventKind.INFO,
+        /* frequency= */ 1,
+        "Applying flags from the config 'test_config'");
   }
 
   @Test
@@ -1121,7 +1149,11 @@ public void basicFlagsetFunctionalityWithTopLevelProjectSchema() throws Exceptio
 
     assertThat(flagSetsValue.getOptionsFromFlagset())
         .containsExactly("--//test:myflag=test_config_value");
-    assertContainsEvent("Applying flags from the config 'test_config'");
+    assertContainsPersistentMessage(
+        flagSetsValue,
+        EventKind.INFO,
+        /* frequency= */ 1,
+        "Applying flags from the config 'test_config'");
   }
 
   @Test