bazel packages: simplify WorkspaceFactory.execute
- remove parseForTesting method
- remove unnecessary StoredEventHandler and call
to handler.clear(). It is now local.
In WFTestHelper: ("helper" is OOP jargon for "junk")
- skip writing WORKSPACE to file system
- remove unused methods and fields.
PiperOrigin-RevId: 331595501
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index dadcacd..fcb462f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -14,12 +14,9 @@
package com.google.devtools.build.lib.packages;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedSet;
-import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
@@ -32,7 +29,6 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Mutability;
-import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Program;
import com.google.devtools.build.lib.syntax.Starlark;
@@ -44,8 +40,6 @@
import com.google.devtools.build.lib.syntax.Tuple;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.Root;
-import com.google.devtools.build.lib.vfs.RootedPath;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
@@ -109,50 +103,13 @@
allowOverride, ruleFactory, this.workspaceGlobals, starlarkSemantics);
}
- // TODO(adonovan): move this into the test. It doesn't need privileged access,
- // and it's called exactly once (!).
- @VisibleForTesting
- void parseForTesting(ParserInput source, @Nullable StoredEventHandler localReporter)
- throws BuildFileContainsErrorsException, InterruptedException {
- if (localReporter == null) {
- localReporter = new StoredEventHandler();
- }
-
- StarlarkFile file = StarlarkFile.parse(source); // use default options in tests
- if (!file.ok()) {
- Event.replayEventsOn(localReporter, file.errors());
- throw new BuildFileContainsErrorsException(
- LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER, "Failed to parse " + source.getFile());
- }
- execute(
- file,
- /* additionalLoadedModules= */ ImmutableMap.of(),
- localReporter,
- WorkspaceFileValue.key(
- RootedPath.toRootedPath(
- Root.fromPath(workspaceDir), PathFragment.create(source.getFile()))));
- }
-
/**
* Actually runs through the AST, calling the functions in the WORKSPACE file and adding rules to
* the //external package.
*/
public void execute(
StarlarkFile file, // becomes resolved as a side effect
- Map<String, Module> loadedModules,
- WorkspaceFileValue.WorkspaceFileKey workspaceFileKey)
- throws InterruptedException {
- Preconditions.checkNotNull(file);
- Preconditions.checkNotNull(loadedModules);
- // TODO(adonovan): What's up with the transient StoredEventHandler?
- // Doesn't this discard events, including print statements?
- execute(file, loadedModules, new StoredEventHandler(), workspaceFileKey);
- }
-
- private void execute(
- StarlarkFile file, // becomes resolved as a side effect
Map<String, Module> additionalLoadedModules,
- StoredEventHandler localReporter,
WorkspaceFileValue.WorkspaceFileKey workspaceFileKey)
throws InterruptedException {
loadedModules.putAll(additionalLoadedModules);
@@ -163,6 +120,7 @@
predeclared.putAll(bindings); // (may shadow bindings in default environment)
Module module = Module.withPredeclared(starlarkSemantics, predeclared);
+ StoredEventHandler localReporter = new StoredEventHandler();
try {
// compile
Program prog = Program.compileFile(file, module);
@@ -215,7 +173,6 @@
if (localReporter.hasErrors()) {
builder.setContainsErrors();
}
- localReporter.clear();
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BUILD b/src/test/java/com/google/devtools/build/lib/packages/BUILD
index fa3605c..ae38003 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/packages/BUILD
@@ -23,9 +23,7 @@
"//src/main/java/com/google/devtools/build/lib:syntax",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
- "//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/vfs",
- "//src/main/java/com/google/devtools/common/options",
"//src/test/java/com/google/devtools/build/lib/testutil",
"//third_party:guava",
"//third_party:junit4",
diff --git a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java
index 6a80fd3..5a862d6 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/WorkspaceFactoryTestHelper.java
@@ -17,29 +17,26 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.fail;
+import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Package.Builder.DefaultPackageSettings;
-import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.syntax.Mutability;
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.TestRuleClassProvider;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
-import com.google.devtools.common.options.OptionsParser;
-import java.io.IOException;
import java.util.List;
/** Parses a WORKSPACE file with the given content. */
-class WorkspaceFactoryTestHelper {
+// TODO(adonovan): delete this junk class.
+final class WorkspaceFactoryTestHelper {
private final Root root;
private Package.Builder builder;
- private Exception exception;
- private ImmutableList<Event> events;
private StarlarkSemantics starlarkSemantics;
private final boolean allowOverride;
@@ -50,20 +47,21 @@
WorkspaceFactoryTestHelper(boolean allowOverride, Root root) {
this.root = root;
- this.exception = null;
- this.events = null;
this.allowOverride = allowOverride;
this.starlarkSemantics = StarlarkSemantics.DEFAULT;
}
void parse(String... args) {
+ // parse
Path workspaceFilePath = root.getRelative("WORKSPACE");
- try {
- FileSystemUtils.writeIsoLatin1(workspaceFilePath, args);
- } catch (IOException e) {
- fail("Shouldn't happen: " + e.getMessage());
+ StarlarkFile file =
+ StarlarkFile.parse(ParserInput.fromString(Joiner.on("\n").join(args), "WORKSPACE"));
+ if (!file.ok()) {
+ fail("parse failed: " + file.errors());
+ return;
}
- StoredEventHandler eventHandler = new StoredEventHandler();
+
+ // execute
builder =
Package.newExternalPackageBuilder(
DefaultPackageSettings.INSTANCE,
@@ -81,50 +79,23 @@
root.asPath(),
/* defaultSystemJavabaseDir= */ null,
starlarkSemantics);
- Exception exception = null;
try {
- byte[] bytes =
- FileSystemUtils.readWithKnownFileSize(workspaceFilePath, workspaceFilePath.getFileSize());
- factory.parseForTesting(
- ParserInput.fromLatin1(bytes, workspaceFilePath.toString()), eventHandler);
- } catch (BuildFileContainsErrorsException e) {
- exception = e;
- } catch (IOException | InterruptedException e) {
+ factory.execute(
+ file,
+ /* additionalLoadedModules= */ ImmutableMap.of(),
+ WorkspaceFileValue.key(RootedPath.toRootedPath(root, workspaceFilePath)));
+ } catch (InterruptedException e) {
fail("Shouldn't happen: " + e.getMessage());
}
- this.events = eventHandler.getEvents();
- this.exception = exception;
}
- public Package getPackage() throws InterruptedException, NoSuchPackageException {
+ Package getPackage() throws InterruptedException, NoSuchPackageException {
return builder.build();
}
- public void assertLexingExceptionThrown() {
- assertThat(exception).isNotNull();
- assertThat(exception).hasMessageThat().contains("Failed to parse /WORKSPACE");
- }
-
- public String getLexerError() {
- assertThat(events).hasSize(1);
- return events.get(0).getMessage();
- }
-
- public String getParserError() {
+ String getParserError() {
List<Event> events = builder.getEvents();
assertThat(events.size()).isGreaterThan(0);
return events.get(0).getMessage();
}
-
- protected void setStarlarkSemantics(String... options) throws Exception {
- starlarkSemantics = parseBuildLanguageOptions(options);
- }
-
- private static StarlarkSemantics parseBuildLanguageOptions(String... options) throws Exception {
- OptionsParser parser =
- OptionsParser.builder().optionsClasses(BuildLanguageOptions.class).build();
- parser.parse(options);
- return parser.getOptions(BuildLanguageOptions.class).toStarlarkSemantics();
- }
-
}
