Automated rollback of commit 23f171e2208700dc2f2f65f5ccb32bacd7aec87d.
*** Reason for rollback ***
b/195550666: internal breakage
*** Original change description ***
Avoid touching the filesystem to see what the BUILD file for a Starlark extension was: we already have the package, so we can just read it out.
Genquery remains buggy here.
haxorz@ observed in http://b/123795023#comment6 that we only need to read these paths to determine if the package was defined in a BUILD or BUILD.bazel file. Since the package already knows that, we just ask it.
PiperOrigin-RevId: 389642637
diff --git a/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java b/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java
index 6983afe..008b893 100644
--- a/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.events;
-import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.List;
@@ -70,13 +69,4 @@
posts.clear();
hasErrors = false;
}
-
- @Override
- public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("events", events)
- .add("posts", posts)
- .add("hasErrors", hasErrors)
- .toString();
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java b/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java
index 6aa8a38..d485394 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java
@@ -16,7 +16,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.Path;
-import javax.annotation.Nullable;
/**
* CachingPackageLocator implementations locate a package by its name.
@@ -29,26 +28,19 @@
* pieces of legacy code while still updating the Skyframe node graph.
*/
public interface CachingPackageLocator {
+
/**
* Returns path of BUILD file for specified package iff the specified package exists, null
* otherwise (e.g. invalid package name, no build file, or package has been deleted via
* --deleted_packages)..
*
- * <p>The package's root directory may be computed by calling getParentFile().
+ * <p> The package's root directory may be computed by calling getParentFile().
*
- * <p>Instances of this interface are required to cache the results.
+ * <p> Instances of this interface are required to cache the results.
*
- * <p>This method must be thread-safe.
+ * <p> This method must be thread-safe.
*/
@ThreadSafe
- @Nullable
Path getBuildFileForPackage(PackageIdentifier packageName);
- /**
- * Returns base name of BUILD file for specified package (typically 'BUILD' or 'BUILD.bazel').
- * {@code packageName} must be an already loaded package. Implementations that do not have full
- * access to the set of loaded packages may return null here.
- */
- @Nullable
- String getBaseNameForLoadedPackage(PackageIdentifier packageName);
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java
index 8fe671b..31aaae2 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java
@@ -55,6 +55,7 @@
import com.google.devtools.build.lib.query2.engine.SkyframeRestartQueryException;
import com.google.devtools.build.lib.query2.engine.ThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.query2.engine.Uniquifier;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.ArrayList;
@@ -437,14 +438,14 @@
// Also add the BUILD file of the extension.
if (buildFiles) {
- // Can be null in genquery: see http://b/123795023#comment6.
- String baseName =
- cachingPackageLocator.getBaseNameForLoadedPackage(
+ Path buildFileForLoad =
+ cachingPackageLocator.getBuildFileForPackage(
loadTarget.getLabel().getLabel().getPackageIdentifier());
- if (baseName != null) {
+ if (buildFileForLoad != null) {
Label buildFileLabel =
Label.createUnvalidated(
- loadTarget.getLabel().getLabel().getPackageIdentifier(), baseName);
+ loadTarget.getLabel().getLabel().getPackageIdentifier(),
+ buildFileForLoad.getBaseName());
addIfUniqueLabel(
getNode(new FakeLoadTarget(buildFileLabel, pkg)), seenLabels, dependentFiles);
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java
index e5c218f..d531b6a 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java
@@ -45,6 +45,7 @@
import com.google.devtools.build.lib.query2.engine.QueryUtil.UniquifierImpl;
import com.google.devtools.build.lib.query2.engine.SkyframeRestartQueryException;
import com.google.devtools.build.lib.query2.engine.Uniquifier;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.Collection;
@@ -437,13 +438,13 @@
// Also add the BUILD file of the extension.
if (buildFiles) {
- // Can be null in genquery: see http://b/123795023#comment6.
- String baseName =
- cachingPackageLocator.getBaseNameForLoadedPackage(
+ Path buildFileForLoad =
+ cachingPackageLocator.getBuildFileForPackage(
loadTarget.getLabel().getPackageIdentifier());
- if (baseName != null) {
+ if (buildFileForLoad != null) {
Label buildFileLabel =
- Label.createUnvalidated(loadTarget.getLabel().getPackageIdentifier(), baseName);
+ Label.createUnvalidated(
+ loadTarget.getLabel().getPackageIdentifier(), buildFileForLoad.getBaseName());
dependentFiles.add(new FakeLoadTarget(buildFileLabel, pkg));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index 2830a2e..e94c860 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -617,14 +617,6 @@
}
return pkg.getBuildFile().getPath();
}
-
- @Override
- public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
- // TODO(b/123795023): we should have the data here but we don't have all packages for Starlark
- // loads present here.
- Package pkg = pkgMap.get(packageName);
- return pkg == null ? null : pkg.getBuildFileLabel().getName();
- }
}
private static class BrokenQueryScopeException extends Exception {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index fbf2e7b..c988797 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -13,14 +13,11 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import static com.google.common.base.Preconditions.checkNotNull;
-
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.CachingPackageLocator;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
@@ -110,22 +107,6 @@
}
@Override
- public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
- StoredEventHandler eventHandler = new StoredEventHandler();
- try {
- Package pkg =
- checkNotNull(
- packageLoader.getPackage(eventHandler, packageName),
- "Already loaded package not present: %s",
- eventHandler);
- return pkg.getBuildFileLabel().getName();
- } catch (InterruptedException | NoSuchPackageException e) {
- throw new IllegalStateException(
- "Can't have error for loaded " + packageName + ", " + eventHandler, e);
- }
- }
-
- @Override
public PathPackageLocator getPackagePath() {
return pkgLocator.get();
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 11818be..2eb0449 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -460,12 +460,6 @@
public Path getBuildFileForPackage(PackageIdentifier packageName) {
return pkgLocatorRef.get().getPackageBuildFileNullable(packageName, syscallCacheRef);
}
-
- @Override
- public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
- Path buildFileForPackage = getBuildFileForPackage(packageName);
- return buildFileForPackage == null ? null : buildFileForPackage.getBaseName();
- }
};
ImmutableMap.Builder<SkyFunctionName, SkyFunction> builder = ImmutableMap.builder();
builder
diff --git a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
index f1b693f..d90c55c 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
@@ -110,12 +110,6 @@
return null;
}
}
-
- @Override
- public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
- Path buildFileForPackage = getBuildFileForPackage(packageName);
- return buildFileForPackage == null ? null : buildFileForPackage.getBaseName();
- }
},
null,
TestUtils.getPool(),
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 1710588..21141f1 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1251,11 +1251,6 @@
public Path getBuildFileForPackage(PackageIdentifier packageName) {
return null;
}
-
- @Override
- public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
- return null;
- }
},
null,
TestUtils.getPool(),
