Report all events from package loading in PackageLoader

There can be events besides those attached to packages. Moving forward, we're
looking to remove per-package event tracking, since it's completely unused
in bazel proper. While this is a loss of precision for PackageLoader, it is
more accurate (we get all of the events) and less likely to become outdated
(if we move events around).

PiperOrigin-RevId: 321776401

src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Reporter;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.BuildFileName;
 import com.google.devtools.build.lib.packages.CachingPackageLocator;
@@ -126,7 +127,7 @@ public Diff getDiff(WalkableGraph fromGraph, Version fromVersion, Version toVers
           return preinjectedDiff;
         }
       };
-  private final Reporter reporter;
+  private final Reporter commonReporter;
   protected final ConfiguredRuleClassProvider ruleClassProvider;
   private final PackageFactory pkgFactory;
   protected StarlarkSemantics starlarkSemantics;
@@ -149,7 +150,7 @@ public abstract static class Builder {
     protected ExternalFilesHelper externalFilesHelper;
     protected ConfiguredRuleClassProvider ruleClassProvider = getDefaultRuleClassProvider();
     protected StarlarkSemantics starlarkSemantics;
-    protected Reporter reporter = new Reporter(new EventBus());
+    protected Reporter commonReporter = new Reporter(new EventBus());
     protected Map<SkyFunctionName, SkyFunction> extraSkyFunctions = new HashMap<>();
     List<PrecomputedValue.Injected> extraPrecomputedValues = new ArrayList<>();
     int legacyGlobbingThreads = 1;
@@ -194,8 +195,9 @@ public Builder useDefaultStarlarkSemantics() {
       return this;
     }
 
-    public Builder setReporter(Reporter reporter) {
-      this.reporter = reporter;
+    /** Sets the reporter used by all skyframe evaluations. */
+    public Builder setCommonReporter(Reporter commonReporter) {
+      this.commonReporter = commonReporter;
       return this;
     }
 
@@ -258,7 +260,7 @@ public final PackageLoader build() {
   AbstractPackageLoader(Builder builder) {
     this.ruleClassProvider = builder.ruleClassProvider;
     this.starlarkSemantics = builder.starlarkSemantics;
-    this.reporter = builder.reporter;
+    this.commonReporter = builder.commonReporter;
     this.extraSkyFunctions = ImmutableMap.copyOf(builder.extraSkyFunctions);
     this.pkgLocatorRef = builder.pkgLocatorRef;
     this.legacyGlobbingThreads = builder.legacyGlobbingThreads;
@@ -322,6 +324,9 @@ public Result loadPackages(Iterable<PackageIdentifier> pkgIds) throws Interrupte
       keys.add(PackageValue.key(pkgId));
     }
 
+    Reporter reporter = new Reporter(commonReporter);
+    StoredEventHandler storedEventHandler = new StoredEventHandler();
+    reporter.addHandler(storedEventHandler);
     EvaluationContext evaluationContext =
         EvaluationContext.newBuilder()
             .setKeepGoing(true)
@@ -344,7 +349,7 @@ public Result loadPackages(Iterable<PackageIdentifier> pkgIds) throws Interrupte
               : new PackageOrException(packageValue.getPackage(), null));
     }
 
-    return new Result(result.build());
+    return new Result(result.build(), storedEventHandler.getEvents());
   }
 
   public ConfiguredRuleClassProvider getRuleClassProvider() {

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

@@ -16,6 +16,7 @@ java_library(
     srcs = ["PackageLoader.java"],
     deps = [
         "//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/packages",
         "//third_party:guava",
         "//third_party:jsr305",

src/main/java/com/google/devtools/build/lib/skyframe/packages/PackageLoader.java

@@ -15,8 +15,10 @@
 
 import static com.google.common.base.Preconditions.checkState;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import javax.annotation.Nullable;
@@ -38,14 +40,23 @@ public interface PackageLoader {
   /** Contains the result of package loading. */
   class Result {
     private final ImmutableMap<PackageIdentifier, PackageOrException> loadedPackages;
+    private final ImmutableList<Event> events;
 
-    Result(ImmutableMap<PackageIdentifier, PackageOrException> loadedPackages) {
+    Result(
+        ImmutableMap<PackageIdentifier, PackageOrException> loadedPackages,
+        ImmutableList<Event> events) {
       this.loadedPackages = loadedPackages;
+      this.events = events;
     }
 
     public ImmutableMap<PackageIdentifier, PackageOrException> getLoadedPackages() {
       return loadedPackages;
     }
+
+    /** Returns all events generated while loading the requested packages. */
+    public ImmutableList<Event> getEvents() {
+      return events;
+    }
   }
 
   /** Contains a {@link Package} or the exception produced while loading it. */

src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java

@@ -59,15 +59,15 @@ public final void init() throws Exception {
   protected abstract AbstractPackageLoader.Builder newPackageLoaderBuilder(Root workspaceDir);
 
   private AbstractPackageLoader.Builder newPackageLoaderBuilder() {
-    return newPackageLoaderBuilder(root).useDefaultStarlarkSemantics().setReporter(reporter);
+    return newPackageLoaderBuilder(root).useDefaultStarlarkSemantics().setCommonReporter(reporter);
   }
 
   protected PackageLoader newPackageLoader() {
     return newPackageLoaderBuilder().build();
   }
 
   @Test
-  public void simpleNoPackage() throws Exception {
+  public void simpleNoPackage() {
     PackageLoader pkgLoader = newPackageLoader();
     PackageIdentifier pkgId = PackageIdentifier.createInMainRepo(PathFragment.create("nope"));
     NoSuchPackageException expected =
@@ -109,8 +109,10 @@ public void simpleMultipleGoodPackage() throws Exception {
     file("good2/BUILD", "sh_library(name = 'good2')");
     PackageIdentifier pkgId1 = PackageIdentifier.createInMainRepo(PathFragment.create("good1"));
     PackageIdentifier pkgId2 = PackageIdentifier.createInMainRepo(PathFragment.create("good2"));
+    PackageLoader.Result result = pkgLoader.loadPackages(ImmutableList.of(pkgId1, pkgId2));
     ImmutableMap<PackageIdentifier, PackageLoader.PackageOrException> pkgs =
-        pkgLoader.loadPackages(ImmutableList.of(pkgId1, pkgId2)).getLoadedPackages();
+        result.getLoadedPackages();
+
     assertThat(pkgs.get(pkgId1).get().containsErrors()).isFalse();
     assertThat(pkgs.get(pkgId2).get().containsErrors()).isFalse();
     assertThat(pkgs.get(pkgId1).get().getTarget("good1").getAssociatedRule().getRuleClass())
@@ -119,9 +121,38 @@ public void simpleMultipleGoodPackage() throws Exception {
         .isEqualTo("sh_library");
     assertNoEvents(pkgs.get(pkgId1).get().getEvents());
     assertNoEvents(pkgs.get(pkgId2).get().getEvents());
+    assertNoEvents(result.getEvents());
     assertNoEvents(handler.getEvents());
   }
 
+  @Test
+  public void testGoodAndBadAndMissingPackages() throws Exception {
+    PackageLoader pkgLoader = newPackageLoader();
+
+    file("bad/BUILD", "invalidBUILDsyntax");
+    PackageIdentifier badPkgId = PackageIdentifier.createInMainRepo(PathFragment.create("bad"));
+
+    file("good/BUILD", "sh_library(name = 'good')");
+    PackageIdentifier goodPkgId = PackageIdentifier.createInMainRepo(PathFragment.create("good"));
+
+    PackageIdentifier missingPkgId = PackageIdentifier.createInMainRepo("missing");
+
+    PackageLoader.Result result =
+        pkgLoader.loadPackages(ImmutableList.of(badPkgId, goodPkgId, missingPkgId));
+
+    Package goodPkg = result.getLoadedPackages().get(goodPkgId).get();
+    assertThat(goodPkg.containsErrors()).isFalse();
+
+    Package badPkg = result.getLoadedPackages().get(badPkgId).get();
+    assertThat(badPkg.containsErrors()).isTrue();
+
+    assertThrows(
+        NoSuchPackageException.class, () -> result.getLoadedPackages().get(missingPkgId).get());
+
+    assertContainsEvent(result.getEvents(), "invalidBUILDsyntax");
+    assertContainsEvent(handler.getEvents(), "invalidBUILDsyntax");
+  }
+
   @Test
   public void loadPackagesToleratesDuplicates() throws Exception {
     PackageLoader pkgLoader = newPackageLoader();
@@ -182,6 +213,18 @@ public void externalFile_AssumeNonExistentAndImmutable() throws Exception {
     assertThat(expected).hasMessageThat().contains("no such package 'foo': BUILD file not found");
   }
 
+  @Test
+  public void testNonPackageEventsReported() throws Exception {
+    PackageLoader pkgLoader = newPackageLoader();
+    path("foo").createDirectoryAndParents();
+    symlink("foo/infinitesymlinkpkg", path("foo"));
+
+    PackageIdentifier pkgId = PackageIdentifier.createInMainRepo("foo/infinitesymlinkpkg");
+    PackageLoader.Result result = pkgLoader.loadPackages(ImmutableList.of(pkgId));
+    assertThrows(NoSuchPackageException.class, () -> result.getLoadedPackages().get(pkgId).get());
+    assertContainsEvent(result.getEvents(), "infinite symlink expansion detected");
+  }
+
   protected Path path(String rootRelativePath) {
     return workspaceDir.getRelative(PathFragment.create(rootRelativePath));
   }