bazel packages: delete testCreatePackageIsolatedFromOuterErrors
This test, from CL 6198296 in 2008, ensured that an error injected
before the return of PackageFactory.parseBuildFile would not cause
PackageFactoryApparatus.createPackage to fail with an exception.
Although enthusiastically approved at the time by someone with the
same login name as me, this is not a style of test we want to
encourage, as it drills holes into lib.packages to support artificial
scenarios. It also requires a lot of support code.
Tests should be expressed in terms of stable interfaces.
Also:
- inline and delete PackageFactory.parseBuildFile.
Enable option.recordScope in tests.
- delete ParsingTracker and ErrorReporter (part of above test,
but artificially distanced from it by the awful Test/TestBase split).
- delete getEnvironmentExtensions() abstract method (always empty)
and parameter of PackageFactoryApparatus
- delete assertOutputFileForRule method (dead code)
- delete getPathPrefix() (always empty)
RELNOTES: N/A
PiperOrigin-RevId: 303802864
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 d9d752a..d503774 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
@@ -63,7 +63,6 @@
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
-import com.google.devtools.build.lib.syntax.Statement;
import com.google.devtools.build.lib.syntax.StringLiteral;
import com.google.devtools.build.lib.syntax.Tuple;
import com.google.devtools.build.lib.syntax.ValidationEnvironment;
@@ -81,7 +80,6 @@
import java.util.Set;
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.atomic.AtomicReference;
-import java.util.logging.Logger;
import javax.annotation.Nullable;
/**
@@ -93,10 +91,7 @@
*/
public final class PackageFactory {
- // Used only for a single test (that looks long obsolete).
- private static final Logger logger = Logger.getLogger(PackageFactory.class.getName());
-
- private static final GoogleLogger glogger = GoogleLogger.forEnclosingClass();
+ private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
/** An extension to the global namespace of the BUILD language. */
// TODO(bazel-team): this is largely unrelated to syntax.StarlarkThread.Extension,
@@ -400,30 +395,6 @@
}
// Exposed to skyframe.PackageFunction.
- public static StarlarkFile parseBuildFile(
- PackageIdentifier packageId,
- ParserInput input,
- List<Statement> preludeStatements,
- StarlarkSemantics semantics) {
- // Options for processing BUILD files.
- FileOptions options =
- FileOptions.builder()
- .recordScope(false) // don't mutate BUILD syntax trees due to shared prelude
- .requireLoadStatementsFirst(false)
- .allowToplevelRebinding(true)
- .restrictStringEscapes(semantics.incompatibleRestrictStringEscapes())
- .build();
-
- // Log messages are expected as signs of progress by a single very old test:
- // testCreatePackageIsolatedFromOuterErrors, see CL 6198296.
- // Removing them will cause it to time out. TODO(adonovan): clean this up.
- logger.fine("Starting to parse " + packageId);
- StarlarkFile file = StarlarkFile.parseWithPrelude(input, preludeStatements, options);
- logger.fine("Finished parsing of " + packageId);
- return file;
- }
-
- // Exposed to skyframe.PackageFunction.
public Package.Builder createPackageFromAst(
String workspaceName,
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping,
@@ -527,9 +498,14 @@
ParserInput input =
ParserInput.create(
FileSystemUtils.convertFromLatin1(buildFileBytes), buildFile.asPath().toString());
- StarlarkFile file =
- parseBuildFile(
- packageId, input, /*preludeStatements=*/ ImmutableList.<Statement>of(), semantics);
+ // Options for processing BUILD files. (No prelude, so recordScope(true) is safe.)
+ FileOptions options =
+ FileOptions.builder()
+ .requireLoadStatementsFirst(false)
+ .allowToplevelRebinding(true)
+ .restrictStringEscapes(semantics.incompatibleRestrictStringEscapes())
+ .build();
+ StarlarkFile file = StarlarkFile.parse(input, options);
Package result =
createPackageFromAst(
externalPkg.getWorkspaceName(),
@@ -689,7 +665,7 @@
// Write a log message for BUILD files with more than 1e6 steps
// (approximately the top 1% in Google's code base).
if (steps > 1_000_000) {
- glogger.atInfo().log(
+ logger.atInfo().log(
"%s: BUILD file computation took %d steps", pkg.getPackageIdentifier(), steps);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 9d91d8a..6394e48 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -58,6 +58,7 @@
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException;
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupValue.SkylarkImportLookupKey;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.FileOptions;
import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
@@ -1207,8 +1208,16 @@
// See the javadoc for ActionOnIOExceptionReadingBuildFile.
}
input = ParserInput.create(buildFileBytes, inputFile.toString());
- file =
- PackageFactory.parseBuildFile(packageId, input, preludeStatements, starlarkSemantics);
+
+ // Options for processing BUILD files.
+ FileOptions options =
+ FileOptions.builder()
+ .recordScope(false) // don't mutate BUILD syntax tree due to shared prelude
+ .requireLoadStatementsFirst(false)
+ .allowToplevelRebinding(true)
+ .restrictStringEscapes(starlarkSemantics.incompatibleRestrictStringEscapes())
+ .build();
+ file = StarlarkFile.parseWithPrelude(input, preludeStatements, options);
fileSyntaxCache.put(packageId, file);
}
SkylarkImportResult importResult;
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 d5c46c0..d3fc968 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
@@ -21,19 +21,15 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
-import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.events.StoredEventHandler;
-import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException;
import com.google.devtools.build.lib.packages.util.PackageFactoryTestBase;
import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -43,12 +39,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Set;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.Semaphore;
-import java.util.concurrent.TimeUnit;
-import java.util.logging.Level;
-import java.util.logging.Logger;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -59,11 +49,6 @@
@RunWith(JUnit4.class)
public class PackageFactoryTest extends PackageFactoryTestBase {
- @Override
- public List<EnvironmentExtension> getEnvironmentExtensions() {
- return ImmutableList.of();
- }
-
@Test
public void testCreatePackage() throws Exception {
Path buildFile = scratch.file("/pkgname/BUILD", "# empty build file ");
@@ -73,31 +58,6 @@
}
@Test
- public void testCreatePackageIsolatedFromOuterErrors() throws Exception {
- ExecutorService e = Executors.newCachedThreadPool();
-
- final Semaphore beforeError = new Semaphore(0);
- final Semaphore afterError = new Semaphore(0);
- Reporter reporter = new Reporter(new EventBus());
- ParsingTracker parser = new ParsingTracker(beforeError, afterError, reporter);
- final Logger log = Logger.getLogger(PackageFactory.class.getName());
- log.addHandler(parser);
- Level originalLevel = log.getLevel();
- log.setLevel(Level.FINE);
-
- e.execute(new ErrorReporter(reporter, beforeError, afterError));
- e.execute(parser);
-
- // wait for all to finish
- e.shutdown();
- assertThat(e.awaitTermination(TestUtils.WAIT_TIMEOUT_MILLISECONDS, TimeUnit.MILLISECONDS))
- .isTrue();
- log.removeHandler(parser);
- log.setLevel(originalLevel);
- assertThat(parser.hasParsed()).isTrue();
- }
-
- @Test
public void testBadRuleName() throws Exception {
events.setFailFast(false);
@@ -1220,11 +1180,6 @@
"package(default_restricted_to=['//foo', '//bar', '//foo'])");
}
- @Override
- protected String getPathPrefix() {
- return "";
- }
-
@Test
public void testGlobPatternExtractor() {
StarlarkFile file =
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
index 8d61e5f..859b4de 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -28,7 +28,6 @@
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.PackageFactory;
-import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
import com.google.devtools.build.lib.packages.PackageValidator;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
@@ -44,7 +43,6 @@
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.common.options.OptionsParser;
import java.io.IOException;
-import java.util.List;
/**
* An apparatus that creates / maintains a {@link PackageFactory}.
@@ -55,22 +53,17 @@
private final PackageFactory factory;
public PackageFactoryApparatus(ExtendedEventHandler eventHandler) {
- this(
- eventHandler,
- /*environmentExtensions=*/ ImmutableList.of(),
- PackageValidator.NOOP_VALIDATOR);
+ this(eventHandler, PackageValidator.NOOP_VALIDATOR);
}
public PackageFactoryApparatus(
- ExtendedEventHandler eventHandler,
- List<EnvironmentExtension> environmentExtensions,
- PackageValidator packageValidator) {
+ ExtendedEventHandler eventHandler, PackageValidator packageValidator) {
this.eventHandler = eventHandler;
RuleClassProvider ruleClassProvider = TestRuleClassProvider.getRuleClassProvider();
factory =
new PackageFactory(
ruleClassProvider,
- ImmutableList.copyOf(environmentExtensions),
+ /*environmentExtensions=*/ ImmutableList.of(),
"test",
Package.Builder.DefaultHelper.INSTANCE,
packageValidator);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
index f32edbc..c829ae7 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
@@ -20,18 +20,13 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
-import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.GlobCache;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
-import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Package;
-import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
import com.google.devtools.build.lib.packages.PackageValidator;
import com.google.devtools.build.lib.packages.RawAttributeMapper;
import com.google.devtools.build.lib.packages.Rule;
@@ -51,25 +46,21 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
-import java.util.concurrent.Semaphore;
-import java.util.logging.Handler;
-import java.util.logging.LogRecord;
import org.junit.Before;
-/**
- * Base class for PackageFactory tests.
- */
+/** Base class for PackageFactory tests. */
+// TODO(adonovan): merge this down into PackageFactory---one small step towards ending
+// a history abuse of the "extends" keyword.
public abstract class PackageFactoryTestBase {
protected Scratch scratch;
protected EventCollectionApparatus events = new EventCollectionApparatus();
protected DummyPackageValidator dummyPackageValidator = new DummyPackageValidator();
protected PackageFactoryApparatus packages =
- new PackageFactoryApparatus(
- events.reporter(), getEnvironmentExtensions(), dummyPackageValidator);
+ new PackageFactoryApparatus(events.reporter(), dummyPackageValidator);
protected Root root;
- protected com.google.devtools.build.lib.packages.Package expectEvalSuccess(String... content)
+ protected Package expectEvalSuccess(String... content)
throws InterruptedException, IOException, NoSuchPackageException {
Path file = scratch.file("pkg/BUILD", content);
Package pkg = packages.eval("pkg", RootedPath.toRootedPath(root, file));
@@ -87,26 +78,12 @@
events.assertContainsError(expectedError);
}
- protected abstract List<EnvironmentExtension> getEnvironmentExtensions();
-
protected Path throwOnReaddir = null;
protected static AttributeMap attributes(Rule rule) {
return RawAttributeMapper.of(rule);
}
- protected static void assertOutputFileForRule(Package pkg, Collection<String> outNames, Rule rule)
- throws Exception {
- for (String outName : outNames) {
- OutputFile out = (OutputFile) pkg.getTarget(outName);
- assertThat(rule.getOutputFiles()).contains(out);
- assertThat(out.getGeneratingRule()).isSameInstanceAs(rule);
- assertThat(out.getName()).isEqualTo(outName);
- assertThat(out.getTargetKind()).isEqualTo("generated file");
- }
- assertThat(rule.getOutputFiles()).hasSize(outNames.size());
- }
-
protected static void assertEvaluates(Package pkg, List<String> expected, String... include)
throws Exception {
assertEvaluates(pkg, expected, ImmutableList.copyOf(include), Collections.<String>emptyList());
@@ -146,7 +123,7 @@
}
protected Path emptyBuildFile(String packageName) {
- return emptyFile(getPathPrefix() + "/" + packageName + "/BUILD");
+ return emptyFile("/" + packageName + "/BUILD");
}
protected Path emptyFile(String path) {
@@ -159,8 +136,7 @@
protected boolean isValidPackageName(String packageName) throws Exception {
// Write a license decl just in case it's a third_party package:
- Path buildFile = scratch.file(
- getPathPrefix() + "/" + packageName + "/BUILD", "licenses(['notice'])");
+ Path buildFile = scratch.file("/" + packageName + "/BUILD", "licenses(['notice'])");
Package pkg = packages.createPackage(packageName, RootedPath.toRootedPath(root, buildFile));
return !pkg.containsErrors();
}
@@ -271,107 +247,9 @@
assertThat(foundError).isEqualTo(errorExpected);
}
- /** Runnable that asks for parsing of build file and synchronizes it with
- * ErrorReporter. It consumes log messages from PackageFactory to release
- * first semaphore when parsing is started and waits for second semaphore
- * before it ends.
+ /**
+ * {@link PackageValidator} whose functionality can be swapped out on demand via {@link #setImpl}.
*/
- protected class ParsingTracker extends Handler implements Runnable {
- private final Semaphore parsingStarted;
- private final Semaphore errorReported;
- private final ExtendedEventHandler eventHandler;
- private boolean first = true;
- private boolean parsedOK;
-
- public ParsingTracker(Semaphore first, Semaphore second, ExtendedEventHandler eventHandler) {
- this.eventHandler = eventHandler;
- parsingStarted = first;
- errorReported = second;
- }
-
- @Override
- public void run() {
- try {
- Path buildFile =
- scratch.file(
- getPathPrefix() + "/isolated/BUILD",
- "# -*- python -*-",
- "",
- "java_library(name = 'mylib',",
- " srcs = 'java/A.java')");
- packages.createPackage(
- PackageIdentifier.createInMainRepo("isolated"),
- RootedPath.toRootedPath(root, buildFile),
- eventHandler);
- parsedOK = true;
- } catch (Exception e) {
- throw new IllegalStateException(e);
- }
- }
-
- public boolean hasParsed() {
- return parsedOK;
- }
-
- @Override
- public void close() throws SecurityException {}
-
- @Override
- public void flush() {}
-
- @Override
- public void publish(LogRecord record) {
- if (!record.getMessage().contains("isolated")) {
- return;
- }
-
- if (first) {
- parsingStarted.release();
- first = false;
- } else {
- try {
- errorReported.acquire();
- } catch (InterruptedException e) {
- e.printStackTrace();
- fail("parsing thread interrupted");
- }
- }
- }
- }
-
- protected abstract String getPathPrefix();
-
- /** Process interfering with parsing of build files.
- * It waits until parsing of some BUILD file is started and then reports
- * arbitrary error. It signals that error was submitted so the parsing can be
- * finished at the end.
- */
- protected class ErrorReporter implements Runnable {
- private final EventHandler eventHandler;
- private final Semaphore parsingStarted;
- private final Semaphore errorReported;
-
- public ErrorReporter(EventHandler eventHandler, Semaphore first, Semaphore second) {
- this.eventHandler = eventHandler;
- parsingStarted = first;
- errorReported = second;
- }
-
- @Override
- public void run() {
- try {
- parsingStarted.acquire();
- eventHandler.handle(
- Event.error(Location.fromFile("dummy"), "Error from other " + "thread"));
- errorReported.release();
- } catch (InterruptedException e) {
- e.printStackTrace();
- fail("ErrorReporter thread interrupted");
- }
- }
- }
-
- /** {@PackageValidator} whose functionality can be swapped out on demand via {@link #setImpl}. */
protected static class DummyPackageValidator implements PackageValidator {
private PackageValidator underlying = PackageValidator.NOOP_VALIDATOR;
