From 2436a358f464ea75b6308dd530c5cce0adde2ebc Mon Sep 17 00:00:00 2001 From: michajlo Date: Tue, 14 Jul 2020 06:13:48 -0700 Subject: [PATCH] Report workspace file events as they happen This is closer to how events for proper packages are reported (immediately after they're loaded), and it makes it less likely that we forget to report events. It also lets us stop retaining Posts in Packages and gets us close to being able to stop retaining Events as well. Also cleans up a few test bugs this exposed ("WORKSPACE" as first line of test workspace files). PiperOrigin-RevId: 321144813 --- .../devtools/build/lib/packages/Package.java | 6 -- .../build/lib/packages/PackageFactory.java | 28 +++++---- .../build/lib/packages/WorkspaceFactory.java | 2 - .../lib/repository/ExternalPackageHelper.java | 8 +-- .../LocalRepositoryLookupFunction.java | 5 -- .../build/lib/skyframe/PackageFunction.java | 5 -- .../lib/skyframe/WorkspaceFileFunction.java | 59 +++++++++++-------- .../skyframe/WorkspaceFileFunctionTest.java | 36 +++++++---- 8 files changed, 76 insertions(+), 73 deletions(-) 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.",