Report workspace file events as they happen

This is closer to how events for proper packages are reported (immediately after
they're loaded), and it makes it less likely that we forget to report events. It
also lets us stop retaining Posts in Packages and gets us close to being able to
stop retaining Events as well.

Also cleans up a few test bugs this exposed ("WORKSPACE" as first line of test
workspace files).

PiperOrigin-RevId: 321144813

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -207,7 +207,6 @@ private NameConflictException(String message) {
   private ImmutableSet<String> features;
 
   private ImmutableList<Event> events;
-  private ImmutableList<Postable> posts;
 
   private ImmutableList<String> registeredExecutionPlatforms;
   private ImmutableList<String> registeredToolchains;
@@ -439,7 +438,6 @@ private void finishInit(Builder builder) {
     this.defaultDistributionSet = builder.defaultDistributionSet;
     this.features = ImmutableSortedSet.copyOf(builder.features);
     this.events = ImmutableList.copyOf(builder.events);
-    this.posts = ImmutableList.copyOf(builder.posts);
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
     this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
@@ -534,10 +532,6 @@ public boolean containsErrors() {
     return containsErrors;
   }
 
-  public List<Postable> getPosts() {
-    return posts;
-  }
-
   public List<Event> getEvents() {
     return events;
   }

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -533,22 +533,24 @@ public Package createPackageForTesting(
             .restrictStringEscapes(semantics.incompatibleRestrictStringEscapes())
             .build();
     StarlarkFile file = StarlarkFile.parse(input, options);
-    Package result =
+    Package.Builder packageBuilder =
         createPackageFromAst(
-                externalPkg.getWorkspaceName(),
-                /*repositoryMapping=*/ ImmutableMap.of(),
-                packageId,
-                buildFile,
-                file,
-                /*loadedModules=*/ ImmutableMap.<String, Module>of(),
-                /*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC,
-                semantics,
-                globber)
-            .build();
-    for (Postable post : result.getPosts()) {
+            externalPkg.getWorkspaceName(),
+            /*repositoryMapping=*/ ImmutableMap.of(),
+            packageId,
+            buildFile,
+            file,
+            /*loadedModules=*/ ImmutableMap.<String, Module>of(),
+            /*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC,
+            semantics,
+            globber);
+    Package result = packageBuilder.build();
+
+    for (Postable post : packageBuilder.getPosts()) {
       eventHandler.post(post);
     }
-    Event.replayEventsOn(eventHandler, result.getEvents());
+    Event.replayEventsOn(eventHandler, packageBuilder.getEvents());
+
     return result;
   }
 

src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java

@@ -225,8 +225,6 @@ public void setParent(
     this.loadedModules.putAll(loadedModules);
     builder.setWorkspaceName(aPackage.getWorkspaceName());
     // Transmit the content of the parent package to the new package builder.
-    builder.addPosts(aPackage.getPosts());
-    builder.addEvents(aPackage.getEvents());
     if (aPackage.containsErrors()) {
       builder.setContainsErrors();
     }

src/main/java/com/google/devtools/build/lib/repository/ExternalPackageHelper.java

@@ -20,7 +20,6 @@
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.BuildFileName;
 import com.google.devtools.build.lib.packages.Package;
@@ -74,7 +73,7 @@ public ImmutableSortedSet<String> getNotSymlinkedInExecrootDirectories(Environme
   public Rule getRuleByName(String ruleName, Environment env)
       throws ExternalPackageException, InterruptedException {
 
-    ExternalPackageRuleExtractor extractor = new ExternalPackageRuleExtractor(env, ruleName);
+    ExternalPackageRuleExtractor extractor = new ExternalPackageRuleExtractor(ruleName);
     if (!iterateWorkspaceFragments(env, extractor)) {
       // Values missing
       return null;
@@ -145,21 +144,18 @@ private boolean iterateWorkspaceFragments(Environment env, WorkspaceFileValuePro
   }
 
   private static class ExternalPackageRuleExtractor implements WorkspaceFileValueProcessor {
-    private final Environment env;
     private final String ruleName;
     private ExternalPackageException exception;
     private Rule rule;
 
-    private ExternalPackageRuleExtractor(Environment env, String ruleName) {
-      this.env = env;
+    private ExternalPackageRuleExtractor(String ruleName) {
       this.ruleName = ruleName;
     }
 
     @Override
     public boolean processAndShouldContinue(WorkspaceFileValue workspaceFileValue) {
       Package externalPackage = workspaceFileValue.getPackage();
       if (externalPackage.containsErrors()) {
-        Event.replayEventsOn(env.getListener(), externalPackage.getEvents());
         exception =
             new ExternalPackageException(
                 new BuildFileContainsErrorsException(

src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java

@@ -20,7 +20,6 @@
 import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
-import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.ErrorDeterminingRepositoryException;
 import com.google.devtools.build.lib.packages.Package;
@@ -171,10 +170,6 @@ private Optional<LocalRepositoryLookupValue> maybeCheckWorkspaceForRepository(
       }
 
       Package externalPackage = value.getPackage();
-      if (externalPackage.containsErrors()) {
-        Event.replayEventsOn(env.getListener(), externalPackage.getEvents());
-      }
-
       // Find all local_repository rules in the WORKSPACE, and check if any have a "path" attribute
       // the same as the requested directory.
       Iterable<Rule> localRepositories =

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -341,11 +341,6 @@ private SkyValue getExternalPackage(Environment env)
     }
 
     Package pkg = workspace.getPackage();
-    Event.replayEventsOn(env.getListener(), pkg.getEvents());
-    for (Postable post : pkg.getPosts()) {
-      env.getListener().post(post);
-    }
-
     if (packageFactory != null) {
       try {
         packageFactory.afterDoneLoadingPackage(

src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java

@@ -18,6 +18,8 @@
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
@@ -79,20 +81,16 @@ public SkyValue compute(SkyKey skyKey, Environment env)
             workspaceFile, ruleClassProvider.getRunfilesPrefix(), starlarkSemantics);
 
     if (workspaceASTValue.getASTs().isEmpty()) {
-      try {
-        return new WorkspaceFileValue(
-            /* pkg = */ builder.build(),
-            /* loadedModules = */ ImmutableMap.<String, Module>of(),
-            /* loadToChunkMap = */ ImmutableMap.<String, Integer>of(),
-            /* bindings = */ ImmutableMap.<String, Object>of(),
-            workspaceFile,
-            /* idx = */ 0, // first fragment
-            /* hasNext = */ false,
-            ImmutableMap.of(),
-            ImmutableSortedSet.of());
-      } catch (NoSuchPackageException e) {
-        throw new WorkspaceFileFunctionException(e, Transience.TRANSIENT);
-      }
+      return new WorkspaceFileValue(
+          buildAndReportEvents(builder, env),
+          /* loadedModules = */ ImmutableMap.<String, Module>of(),
+          /* loadToChunkMap = */ ImmutableMap.<String, Integer>of(),
+          /* bindings = */ ImmutableMap.<String, Object>of(),
+          workspaceFile,
+          /* idx = */ 0, // first fragment
+          /* hasNext = */ false,
+          ImmutableMap.of(),
+          ImmutableSortedSet.of());
     }
     WorkspaceFactory parser;
     WorkspaceFileValue prevValue = null;
@@ -141,20 +139,33 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
     }
 
+    return new WorkspaceFileValue(
+        buildAndReportEvents(builder, env),
+        parser.getLoadedModules(),
+        createLoadToChunkMap(prevValue, parser, key),
+        parser.getVariableBindings(),
+        workspaceFile,
+        key.getIndex(),
+        key.getIndex() < workspaceASTValue.getASTs().size() - 1,
+        ImmutableMap.copyOf(parser.getManagedDirectories()),
+        parser.getDoNotSymlinkInExecrootPaths());
+  }
+
+  private static Package buildAndReportEvents(Package.Builder pkgBuilder, Environment env)
+      throws WorkspaceFileFunctionException {
+    Package result;
     try {
-      return new WorkspaceFileValue(
-          builder.build(),
-          parser.getLoadedModules(),
-          createLoadToChunkMap(prevValue, parser, key),
-          parser.getVariableBindings(),
-          workspaceFile,
-          key.getIndex(),
-          key.getIndex() < workspaceASTValue.getASTs().size() - 1,
-          ImmutableMap.copyOf(parser.getManagedDirectories()),
-          parser.getDoNotSymlinkInExecrootPaths());
+      result = pkgBuilder.build();
     } catch (NoSuchPackageException e) {
       throw new WorkspaceFileFunctionException(e, Transience.TRANSIENT);
     }
+
+    Event.replayEventsOn(env.getListener(), pkgBuilder.getEvents());
+    for (Postable postable : pkgBuilder.getPosts()) {
+      env.getListener().post(postable);
+    }
+
+    return result;
   }
 
   /**

src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java

@@ -35,7 +35,6 @@
 import com.google.devtools.build.lib.rules.repository.ManagedDirectoriesKnowledgeImpl.ManagedDirectoriesListener;
 import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
 import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.testutil.MoreAsserts;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -122,7 +121,6 @@ public void testLoadToChunkMapSimple() throws Exception {
     scratch.file("BUILD", "");
     RootedPath workspace =
         createWorkspaceFile(
-            "WORKSPACE",
             "workspace(name = 'good')",
             "load('//:a.bzl', 'a')",
             "x = 1  #for chunk break",
@@ -145,7 +143,6 @@ public void testLoadToChunkMapDoesNotOverrideDuplicate() throws Exception {
     scratch.file("BUILD", "");
     RootedPath workspace =
         createWorkspaceFile(
-            "WORKSPACE",
             "workspace(name = 'good')",
             "load('//:a.bzl', 'a')",
             "x = 1  #for chunk break",
@@ -261,6 +258,9 @@ public void testManagedDirectories() throws Exception {
 
       createWorkspaceFileValueForTest();
 
+      // Test intentionally introduces errors.
+      reporter.removeHandler(failFastHandler);
+
       assertManagedDirectoriesParsingError(
           "{'@repo1': 'dir1', '@repo2': ['dir3']}",
           "managed_directories attribute value should be of the type attr.string_list_dict(),"
@@ -340,7 +340,7 @@ private WorkspaceFileValue parseWorkspaceFileValue(String... lines) throws Excep
     Package pkg = workspaceFileValue.getPackage();
     if (pkg.containsErrors()) {
       throw new RuntimeException(
-          Preconditions.checkNotNull(Iterables.getFirst(pkg.getEvents(), null)).getMessage());
+          Preconditions.checkNotNull(Iterables.getFirst(eventCollector, null)).getMessage());
     }
     return workspaceFileValue;
   }
@@ -350,7 +350,7 @@ private void parseWorkspaceFileValueWithError(String expectedError, String... li
     WorkspaceFileValue workspaceFileValue = parseWorkspaceFileValueImpl(lines);
     Package pkg = workspaceFileValue.getPackage();
     assertThat(pkg.containsErrors()).isTrue();
-    MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedError);
+    assertContainsEvent(expectedError);
   }
 
   private WorkspaceFileValue parseWorkspaceFileValueImpl(String[] lines)
@@ -363,12 +363,15 @@ private WorkspaceFileValue parseWorkspaceFileValueImpl(String[] lines)
 
   @Test
   public void testInvalidRepo() throws Exception {
+    // Test intentionally introduces errors.
+    reporter.removeHandler(failFastHandler);
+
     RootedPath workspacePath = createWorkspaceFile("workspace(name = 'foo$')");
     SkyKey key = ExternalPackageFunction.key(workspacePath);
     EvaluationResult<PackageValue> evaluationResult = eval(key);
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(pkg.containsErrors()).isTrue();
-    MoreAsserts.assertContainsEvent(pkg.getEvents(), "foo$ is not a legal workspace name");
+    assertContainsEvent("foo$ is not a legal workspace name");
   }
 
   @Test
@@ -381,7 +384,7 @@ public void testBindFunction() throws Exception {
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(getLabelMapping(pkg, "foo/bar"))
         .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of()));
-    MoreAsserts.assertNoEvents(pkg.getEvents());
+    assertNoEvents();
   }
 
   @Test
@@ -394,11 +397,14 @@ public void testBindArgsReversed() throws Exception {
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(getLabelMapping(pkg, "foo/bar"))
         .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of()));
-    MoreAsserts.assertNoEvents(pkg.getEvents());
+    assertNoEvents();
   }
 
   @Test
   public void testNonExternalBinding() throws Exception {
+    // Test intentionally introduces errors.
+    reporter.removeHandler(failFastHandler);
+
     // name must be a valid label name.
     String[] lines = {"bind(name = 'foo:bar', actual = '//bar/baz')"};
     RootedPath workspacePath = createWorkspaceFile(lines);
@@ -407,11 +413,14 @@ public void testNonExternalBinding() throws Exception {
     EvaluationResult<PackageValue> evaluationResult = eval(key);
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(pkg.containsErrors()).isTrue();
-    MoreAsserts.assertContainsEvent(pkg.getEvents(), "target names may not contain ':'");
+    assertContainsEvent("target names may not contain ':'");
   }
 
   @Test
   public void testWorkspaceFileParsingError() throws Exception {
+    // Test intentionally introduces errors.
+    reporter.removeHandler(failFastHandler);
+
     // //external:bar:baz is not a legal package.
     String[] lines = {"bind(name = 'foo/bar', actual = '//external:bar:baz')"};
     RootedPath workspacePath = createWorkspaceFile(lines);
@@ -420,7 +429,7 @@ public void testWorkspaceFileParsingError() throws Exception {
     EvaluationResult<PackageValue> evaluationResult = eval(key);
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(pkg.containsErrors()).isTrue();
-    MoreAsserts.assertContainsEvent(pkg.getEvents(), "target names may not contain ':'");
+    assertContainsEvent("target names may not contain ':'");
   }
 
   @Test
@@ -433,7 +442,7 @@ public void testNoWorkspaceFile() throws Exception {
     EvaluationResult<PackageValue> evaluationResult = eval(key);
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(pkg.containsErrors()).isFalse();
-    MoreAsserts.assertNoEvents(pkg.getEvents());
+    assertNoEvents();
   }
 
   @Test
@@ -448,7 +457,7 @@ public void testListBindFunction() throws Exception {
     Package pkg = evaluationResult.get(key).getPackage();
     assertThat(getLabelMapping(pkg, "foo/bar"))
         .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of()));
-    MoreAsserts.assertNoEvents(pkg.getEvents());
+    assertNoEvents();
   }
 
   @Test
@@ -496,6 +505,9 @@ public void testDoNotSymlinkInExecroot() throws Exception {
                   .getDoNotSymlinkInExecrootPaths())
           .isEmpty();
 
+      // Test now intentionally introduces errors.
+      reporter.removeHandler(failFastHandler);
+
       parseWorkspaceFileValueWithError(
           "toplevel_output_directories should not "
               + "contain duplicate values: \"out\" is specified more then once.",