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 9a0e684eb0b76e..1bc7e29a306a61 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 @@ -207,7 +207,6 @@ private NameConflictException(String message) { private ImmutableSet features; private ImmutableList events; - private ImmutableList posts; private ImmutableList registeredExecutionPlatforms; private ImmutableList registeredToolchains; @@ -439,7 +438,6 @@ private void finishInit(Builder builder) { this.defaultDistributionSet = builder.defaultDistributionSet; this.features = ImmutableSortedSet.copyOf(builder.features); this.events = ImmutableList.copyOf(builder.events); - this.posts = ImmutableList.copyOf(builder.posts); this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms); this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains); this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping); @@ -534,10 +532,6 @@ public boolean containsErrors() { return containsErrors; } - public List getPosts() { - return posts; - } - public List getEvents() { return events; } 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 4a386e50941653..7ee684390eb361 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 @@ -533,22 +533,24 @@ public Package createPackageForTesting( .restrictStringEscapes(semantics.incompatibleRestrictStringEscapes()) .build(); StarlarkFile file = StarlarkFile.parse(input, options); - Package result = + Package.Builder packageBuilder = createPackageFromAst( - externalPkg.getWorkspaceName(), - /*repositoryMapping=*/ ImmutableMap.of(), - packageId, - buildFile, - file, - /*loadedModules=*/ ImmutableMap.of(), - /*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC, - semantics, - globber) - .build(); - for (Postable post : result.getPosts()) { + externalPkg.getWorkspaceName(), + /*repositoryMapping=*/ ImmutableMap.of(), + packageId, + buildFile, + file, + /*loadedModules=*/ ImmutableMap.of(), + /*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC, + semantics, + globber); + Package result = packageBuilder.build(); + + for (Postable post : packageBuilder.getPosts()) { eventHandler.post(post); } - Event.replayEventsOn(eventHandler, result.getEvents()); + Event.replayEventsOn(eventHandler, packageBuilder.getEvents()); + return result; } 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 0892e414436c07..340c6bc5f7b330 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 @@ -225,8 +225,6 @@ public void setParent( this.loadedModules.putAll(loadedModules); builder.setWorkspaceName(aPackage.getWorkspaceName()); // Transmit the content of the parent package to the new package builder. - builder.addPosts(aPackage.getPosts()); - builder.addEvents(aPackage.getEvents()); if (aPackage.containsErrors()) { builder.setContainsErrors(); } diff --git a/src/main/java/com/google/devtools/build/lib/repository/ExternalPackageHelper.java b/src/main/java/com/google/devtools/build/lib/repository/ExternalPackageHelper.java index b5d916c70010c0..fe8a8dd88a0ba2 100644 --- a/src/main/java/com/google/devtools/build/lib/repository/ExternalPackageHelper.java +++ b/src/main/java/com/google/devtools/build/lib/repository/ExternalPackageHelper.java @@ -20,7 +20,6 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.cmdline.LabelConstants; -import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.BuildFileName; import com.google.devtools.build.lib.packages.Package; @@ -74,7 +73,7 @@ public ImmutableSortedSet getNotSymlinkedInExecrootDirectories(Environme public Rule getRuleByName(String ruleName, Environment env) throws ExternalPackageException, InterruptedException { - ExternalPackageRuleExtractor extractor = new ExternalPackageRuleExtractor(env, ruleName); + ExternalPackageRuleExtractor extractor = new ExternalPackageRuleExtractor(ruleName); if (!iterateWorkspaceFragments(env, extractor)) { // Values missing return null; @@ -145,13 +144,11 @@ private boolean iterateWorkspaceFragments(Environment env, WorkspaceFileValuePro } private static class ExternalPackageRuleExtractor implements WorkspaceFileValueProcessor { - private final Environment env; private final String ruleName; private ExternalPackageException exception; private Rule rule; - private ExternalPackageRuleExtractor(Environment env, String ruleName) { - this.env = env; + private ExternalPackageRuleExtractor(String ruleName) { this.ruleName = ruleName; } @@ -159,7 +156,6 @@ private ExternalPackageRuleExtractor(Environment env, String ruleName) { public boolean processAndShouldContinue(WorkspaceFileValue workspaceFileValue) { Package externalPackage = workspaceFileValue.getPackage(); if (externalPackage.containsErrors()) { - Event.replayEventsOn(env.getListener(), externalPackage.getEvents()); exception = new ExternalPackageException( new BuildFileContainsErrorsException( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java index ea32186f9aa78d..a625ec9dc0efc3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/LocalRepositoryLookupFunction.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.actions.InconsistentFilesystemException; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.AggregatingAttributeMapper; import com.google.devtools.build.lib.packages.ErrorDeterminingRepositoryException; import com.google.devtools.build.lib.packages.Package; @@ -171,10 +170,6 @@ private Optional maybeCheckWorkspaceForRepository( } Package externalPackage = value.getPackage(); - if (externalPackage.containsErrors()) { - Event.replayEventsOn(env.getListener(), externalPackage.getEvents()); - } - // Find all local_repository rules in the WORKSPACE, and check if any have a "path" attribute // the same as the requested directory. Iterable localRepositories = 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 04b61ec0b46735..507b591af36a9d 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 @@ -341,11 +341,6 @@ private SkyValue getExternalPackage(Environment env) } Package pkg = workspace.getPackage(); - Event.replayEventsOn(env.getListener(), pkg.getEvents()); - for (Postable post : pkg.getPosts()) { - env.getListener().post(post); - } - if (packageFactory != null) { try { packageFactory.afterDoneLoadingPackage( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java index 65ba07fa059840..d27a8ca7d1f365 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java @@ -18,6 +18,8 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Package.NameConflictException; @@ -79,20 +81,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) workspaceFile, ruleClassProvider.getRunfilesPrefix(), starlarkSemantics); if (workspaceASTValue.getASTs().isEmpty()) { - try { - return new WorkspaceFileValue( - /* pkg = */ builder.build(), - /* loadedModules = */ ImmutableMap.of(), - /* loadToChunkMap = */ ImmutableMap.of(), - /* bindings = */ ImmutableMap.of(), - workspaceFile, - /* idx = */ 0, // first fragment - /* hasNext = */ false, - ImmutableMap.of(), - ImmutableSortedSet.of()); - } catch (NoSuchPackageException e) { - throw new WorkspaceFileFunctionException(e, Transience.TRANSIENT); - } + return new WorkspaceFileValue( + buildAndReportEvents(builder, env), + /* loadedModules = */ ImmutableMap.of(), + /* loadToChunkMap = */ ImmutableMap.of(), + /* bindings = */ ImmutableMap.of(), + workspaceFile, + /* idx = */ 0, // first fragment + /* hasNext = */ false, + ImmutableMap.of(), + ImmutableSortedSet.of()); } WorkspaceFactory parser; WorkspaceFileValue prevValue = null; @@ -141,20 +139,33 @@ public SkyValue compute(SkyKey skyKey, Environment env) throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT); } + return new WorkspaceFileValue( + buildAndReportEvents(builder, env), + parser.getLoadedModules(), + createLoadToChunkMap(prevValue, parser, key), + parser.getVariableBindings(), + workspaceFile, + key.getIndex(), + key.getIndex() < workspaceASTValue.getASTs().size() - 1, + ImmutableMap.copyOf(parser.getManagedDirectories()), + parser.getDoNotSymlinkInExecrootPaths()); + } + + private static Package buildAndReportEvents(Package.Builder pkgBuilder, Environment env) + throws WorkspaceFileFunctionException { + Package result; try { - return new WorkspaceFileValue( - builder.build(), - parser.getLoadedModules(), - createLoadToChunkMap(prevValue, parser, key), - parser.getVariableBindings(), - workspaceFile, - key.getIndex(), - key.getIndex() < workspaceASTValue.getASTs().size() - 1, - ImmutableMap.copyOf(parser.getManagedDirectories()), - parser.getDoNotSymlinkInExecrootPaths()); + result = pkgBuilder.build(); } catch (NoSuchPackageException e) { throw new WorkspaceFileFunctionException(e, Transience.TRANSIENT); } + + Event.replayEventsOn(env.getListener(), pkgBuilder.getEvents()); + for (Postable postable : pkgBuilder.getPosts()) { + env.getListener().post(postable); + } + + return result; } /** diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java index 98da46cba756dd..bca37d0aa5bd2f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java @@ -35,7 +35,6 @@ import com.google.devtools.build.lib.rules.repository.ManagedDirectoriesKnowledgeImpl.ManagedDirectoriesListener; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.syntax.StarlarkSemantics; -import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -122,7 +121,6 @@ public void testLoadToChunkMapSimple() throws Exception { scratch.file("BUILD", ""); RootedPath workspace = createWorkspaceFile( - "WORKSPACE", "workspace(name = 'good')", "load('//:a.bzl', 'a')", "x = 1 #for chunk break", @@ -145,7 +143,6 @@ public void testLoadToChunkMapDoesNotOverrideDuplicate() throws Exception { scratch.file("BUILD", ""); RootedPath workspace = createWorkspaceFile( - "WORKSPACE", "workspace(name = 'good')", "load('//:a.bzl', 'a')", "x = 1 #for chunk break", @@ -261,6 +258,9 @@ public void testManagedDirectories() throws Exception { createWorkspaceFileValueForTest(); + // Test intentionally introduces errors. + reporter.removeHandler(failFastHandler); + assertManagedDirectoriesParsingError( "{'@repo1': 'dir1', '@repo2': ['dir3']}", "managed_directories attribute value should be of the type attr.string_list_dict()," @@ -340,7 +340,7 @@ private WorkspaceFileValue parseWorkspaceFileValue(String... lines) throws Excep Package pkg = workspaceFileValue.getPackage(); if (pkg.containsErrors()) { throw new RuntimeException( - Preconditions.checkNotNull(Iterables.getFirst(pkg.getEvents(), null)).getMessage()); + Preconditions.checkNotNull(Iterables.getFirst(eventCollector, null)).getMessage()); } return workspaceFileValue; } @@ -350,7 +350,7 @@ private void parseWorkspaceFileValueWithError(String expectedError, String... li WorkspaceFileValue workspaceFileValue = parseWorkspaceFileValueImpl(lines); Package pkg = workspaceFileValue.getPackage(); assertThat(pkg.containsErrors()).isTrue(); - MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedError); + assertContainsEvent(expectedError); } private WorkspaceFileValue parseWorkspaceFileValueImpl(String[] lines) @@ -363,12 +363,15 @@ private WorkspaceFileValue parseWorkspaceFileValueImpl(String[] lines) @Test public void testInvalidRepo() throws Exception { + // Test intentionally introduces errors. + reporter.removeHandler(failFastHandler); + RootedPath workspacePath = createWorkspaceFile("workspace(name = 'foo$')"); SkyKey key = ExternalPackageFunction.key(workspacePath); EvaluationResult evaluationResult = eval(key); Package pkg = evaluationResult.get(key).getPackage(); assertThat(pkg.containsErrors()).isTrue(); - MoreAsserts.assertContainsEvent(pkg.getEvents(), "foo$ is not a legal workspace name"); + assertContainsEvent("foo$ is not a legal workspace name"); } @Test @@ -381,7 +384,7 @@ public void testBindFunction() throws Exception { Package pkg = evaluationResult.get(key).getPackage(); assertThat(getLabelMapping(pkg, "foo/bar")) .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of())); - MoreAsserts.assertNoEvents(pkg.getEvents()); + assertNoEvents(); } @Test @@ -394,11 +397,14 @@ public void testBindArgsReversed() throws Exception { Package pkg = evaluationResult.get(key).getPackage(); assertThat(getLabelMapping(pkg, "foo/bar")) .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of())); - MoreAsserts.assertNoEvents(pkg.getEvents()); + assertNoEvents(); } @Test public void testNonExternalBinding() throws Exception { + // Test intentionally introduces errors. + reporter.removeHandler(failFastHandler); + // name must be a valid label name. String[] lines = {"bind(name = 'foo:bar', actual = '//bar/baz')"}; RootedPath workspacePath = createWorkspaceFile(lines); @@ -407,11 +413,14 @@ public void testNonExternalBinding() throws Exception { EvaluationResult evaluationResult = eval(key); Package pkg = evaluationResult.get(key).getPackage(); assertThat(pkg.containsErrors()).isTrue(); - MoreAsserts.assertContainsEvent(pkg.getEvents(), "target names may not contain ':'"); + assertContainsEvent("target names may not contain ':'"); } @Test public void testWorkspaceFileParsingError() throws Exception { + // Test intentionally introduces errors. + reporter.removeHandler(failFastHandler); + // //external:bar:baz is not a legal package. String[] lines = {"bind(name = 'foo/bar', actual = '//external:bar:baz')"}; RootedPath workspacePath = createWorkspaceFile(lines); @@ -420,7 +429,7 @@ public void testWorkspaceFileParsingError() throws Exception { EvaluationResult evaluationResult = eval(key); Package pkg = evaluationResult.get(key).getPackage(); assertThat(pkg.containsErrors()).isTrue(); - MoreAsserts.assertContainsEvent(pkg.getEvents(), "target names may not contain ':'"); + assertContainsEvent("target names may not contain ':'"); } @Test @@ -433,7 +442,7 @@ public void testNoWorkspaceFile() throws Exception { EvaluationResult evaluationResult = eval(key); Package pkg = evaluationResult.get(key).getPackage(); assertThat(pkg.containsErrors()).isFalse(); - MoreAsserts.assertNoEvents(pkg.getEvents()); + assertNoEvents(); } @Test @@ -448,7 +457,7 @@ public void testListBindFunction() throws Exception { Package pkg = evaluationResult.get(key).getPackage(); assertThat(getLabelMapping(pkg, "foo/bar")) .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of())); - MoreAsserts.assertNoEvents(pkg.getEvents()); + assertNoEvents(); } @Test @@ -496,6 +505,9 @@ public void testDoNotSymlinkInExecroot() throws Exception { .getDoNotSymlinkInExecrootPaths()) .isEmpty(); + // Test now intentionally introduces errors. + reporter.removeHandler(failFastHandler); + parseWorkspaceFileValueWithError( "toplevel_output_directories should not " + "contain duplicate values: \"out\" is specified more then once.",