commit eb97f8647a6dcc4dd4705d156fca64f44226fe72
author nharmata <nharmata@google.com> Wed Mar 18 11:11:38 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Mar 18 11:13:12 2020 -0700
tree 0e955ee450bd5539d23a4d3347b1dd2d8ef9da3f
parent 1932d0f460681820d10c6501a3b582e319d3d7a1

Have PackageFactory#afterDoneLoadingPackage call PackageValidator.validate before it calls Package.Builder.Helper#onLoadingComplete.

This fixes a hypothetical bug where Package.Builder.Helper#onLoadingComplete implementations if rightfully assuming the Package had successfully been loaded when it fact it hadn't.

While I'm here, now that this hypothetical bug has been fixed, I renamed #onLoadingComplete to #onLoadingCompleteAndSuccessful to emphasize that the assumption mentioned above is a legit one to make.

RELNOTES:
PiperOrigin-RevId: 301627063

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

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 9736d0f..fd2d66f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -779,8 +779,8 @@
       Package createFreshPackage(PackageIdentifier packageId, String runfilesPrefix);
 
       /**
-       * Called after {@link com.google.devtools.build.lib.skyframe.PackageFunction} is completely
-       * done loading the given {@link Package}.
+       * Called after {@link com.google.devtools.build.lib.skyframe.PackageFunction} has
+       * successfully loaded the given {@link Package}.
        *
        * @param pkg the loaded {@link Package}
        * @param starlarkSemantics are the semantics used to load the package
@@ -790,7 +790,8 @@
        *     and parse the package's BUILD file, nor the time to read, parse, or evaluate any of the
        *     transitively loaded .bzl files.
        */
-      void onLoadingComplete(Package pkg, StarlarkSemantics starlarkSemantics, long loadTimeNanos);
+      void onLoadingCompleteAndSuccessful(
+          Package pkg, StarlarkSemantics starlarkSemantics, long loadTimeNanos);
     }
 
     /** {@link Helper} that simply calls the {@link Package} constructor. */
@@ -806,7 +807,7 @@
       }
 
       @Override
-      public void onLoadingComplete(
+      public void onLoadingCompleteAndSuccessful(
           Package pkg, StarlarkSemantics starlarkSemantics, long loadTimeMs) {}
     }
 

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

diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 4a90723..b59ab85 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -664,8 +664,8 @@
       long loadTimeNanos,
       ExtendedEventHandler eventHandler)
       throws InvalidPackageException {
-    packageBuilderHelper.onLoadingComplete(pkg, starlarkSemantics, loadTimeNanos);
     packageValidator.validate(pkg, eventHandler);
+    packageBuilderHelper.onLoadingCompleteAndSuccessful(pkg, starlarkSemantics, loadTimeNanos);
   }
 
   /**

src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java

diff --git a/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java b/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java
index 908c713..4ea0fb5 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/BazelPackageBuilderHelperForTesting.java
@@ -49,7 +49,7 @@
   }
 
   @Override
-  public void onLoadingComplete(
+  public void onLoadingCompleteAndSuccessful(
       Package pkg, StarlarkSemantics starlarkSemantics, long loadTimeNanos) {
     sanityCheckBazelPackageLoader(pkg, ruleClassProvider, starlarkSemantics);
   }