diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 8fb687040404ae..08f5fa150c9196 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.CommandLineExpansionException; +import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.AnalysisProtos.ActionGraphContainer; @@ -38,6 +39,7 @@ import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.concurrent.Uninterruptibles; import com.google.devtools.build.lib.events.Event; @@ -52,6 +54,8 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions; +import com.google.devtools.build.lib.packages.WorkspaceFileValue; +import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey; import com.google.devtools.build.lib.pkgcache.PackageCacheOptions; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.profiler.Profiler; @@ -70,6 +74,7 @@ import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; import com.google.devtools.build.lib.skyframe.actiongraph.ActionGraphDump; import com.google.devtools.build.lib.util.AbruptExitException; +import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.ResourceUsage; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; @@ -78,6 +83,7 @@ import com.google.devtools.build.lib.vfs.ModifiedFileSet; 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 com.google.devtools.build.skyframe.BuildDriver; import com.google.devtools.build.skyframe.Differencer; import com.google.devtools.build.skyframe.EvaluationContext; @@ -93,6 +99,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.common.options.OptionsProvider; +import java.io.IOException; import java.io.PrintStream; import java.time.Duration; import java.util.ArrayList; @@ -102,6 +109,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.concurrent.Callable; @@ -141,6 +149,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor skyframeExecutorConsumerOnInit, EvaluatorSupplier evaluatorSupplier, @@ -159,7 +169,8 @@ private SequencedSkyframeExecutor( List buildFilesByPriority, ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile, BuildOptions defaultBuildOptions, - MutableArtifactFactorySupplier mutableArtifactFactorySupplier) { + MutableArtifactFactorySupplier mutableArtifactFactorySupplier, + @Nullable WorkspaceFileHeaderListener workspaceFileHeaderListener) { super( skyframeExecutorConsumerOnInit, evaluatorSupplier, @@ -185,6 +196,7 @@ private SequencedSkyframeExecutor( /*nonexistentFileReceiver=*/ null); this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories); this.customDirtinessCheckers = customDirtinessCheckers; + this.workspaceFileHeaderListener = workspaceFileHeaderListener; } @Override @@ -301,7 +313,8 @@ public void setDeletedPackages(Iterable pkgs) { /** Uses diff awareness on all the package paths to invalidate changed files. */ @VisibleForTesting - public void handleDiffsForTesting(ExtendedEventHandler eventHandler) throws InterruptedException { + public void handleDiffsForTesting(ExtendedEventHandler eventHandler) + throws InterruptedException, AbruptExitException { if (super.lastAnalysisDiscarded) { // Values were cleared last build, but they couldn't be deleted because they were needed for // the execution phase. We can delete them now. @@ -313,9 +326,14 @@ public void handleDiffsForTesting(ExtendedEventHandler eventHandler) throws Inte private void handleDiffs( ExtendedEventHandler eventHandler, boolean checkOutputFiles, OptionsProvider options) - throws InterruptedException { + throws InterruptedException, AbruptExitException { TimestampGranularityMonitor tsgm = this.tsgm.get(); modifiedFiles = 0; + + if (workspaceFileHeaderListener != null) { + refreshWorkspaceHeader(eventHandler); + } + Map modifiedFilesByPathEntry = Maps.newHashMap(); Set> @@ -803,6 +821,68 @@ public void dumpPackages(PrintStream out) { } } + /** + * Calculate the new value of the WORKSPACE file header (WorkspaceFileValue with the index = 0), + * and call the listener, if the value has changed. Needed for incremental update of user-owned + * directories by repository rules. + */ + private void refreshWorkspaceHeader(ExtendedEventHandler eventHandler) + throws InterruptedException, AbruptExitException { + Preconditions.checkNotNull(workspaceFileHeaderListener); + + Root workspaceRoot = Root.fromPath(directories.getWorkspace()); + RootedPath workspacePath = + RootedPath.toRootedPath(workspaceRoot, LabelConstants.WORKSPACE_FILE_NAME); + WorkspaceFileKey workspaceFileKey = WorkspaceFileValue.key(workspacePath); + + WorkspaceFileValue oldValue = + (WorkspaceFileValue) memoizingEvaluator.getExistingValue(workspaceFileKey); + maybeInvalidateWorkspaceFileStateValue(workspacePath); + WorkspaceFileValue newValue = + (WorkspaceFileValue) evaluateSingleValue(workspaceFileKey, eventHandler); + if (!Objects.equals(newValue, oldValue)) { + workspaceFileHeaderListener.workspaceHeaderChanged(newValue); + } + } + + // We only check the FileStateValue of the WORKSPACE file; we do not support the case + // when the WORKSPACE file is a symlink. + private void maybeInvalidateWorkspaceFileStateValue(RootedPath workspacePath) + throws InterruptedException, AbruptExitException { + SkyKey workspaceFileStateKey = FileStateValue.key(workspacePath); + SkyValue oldWorkspaceFileState = memoizingEvaluator.getExistingValue(workspaceFileStateKey); + if (oldWorkspaceFileState == null) { + // no need to invalidate if not cached + return; + } + FileStateValue newWorkspaceFileState; + try { + newWorkspaceFileState = FileStateValue.create(workspacePath, tsgm.get()); + if (FileStateType.SYMLINK.equals(newWorkspaceFileState.getType())) { + throw new AbruptExitException( + "WORKSPACE file can not be a symlink if incrementally" + + " updated directories feature is enabled.", + ExitCode.PARSING_FAILURE); + } + } catch (IOException e) { + throw new AbruptExitException("Can not read WORKSPACE file.", ExitCode.PARSING_FAILURE, e); + } + if (!oldWorkspaceFileState.equals(newWorkspaceFileState)) { + recordingDiffer.invalidate(ImmutableSet.of(workspaceFileStateKey)); + } + } + + private SkyValue evaluateSingleValue(SkyKey key, ExtendedEventHandler eventHandler) + throws InterruptedException { + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(DEFAULT_THREAD_COUNT) + .setEventHander(eventHandler) + .build(); + return buildDriver.evaluate(ImmutableSet.of(key), evaluationContext).get(key); + } + public static Builder builder() { return new Builder(); } @@ -827,6 +907,7 @@ public static final class Builder { private CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy; private List buildFilesByPriority; private ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile; + private WorkspaceFileHeaderListener workspaceFileHeaderListener; // Fields with default values. private ImmutableMap extraSkyFunctions = ImmutableMap.of(); @@ -872,7 +953,8 @@ public SequencedSkyframeExecutor build() { buildFilesByPriority, actionOnIOExceptionReadingBuildFile, defaultBuildOptions, - mutableArtifactFactorySupplier); + mutableArtifactFactorySupplier, + workspaceFileHeaderListener); skyframeExecutor.init(); return skyframeExecutor; } @@ -975,5 +1057,21 @@ public Builder setSkyframeExecutorConsumerOnInit( this.skyframeExecutorConsumerOnInit = skyframeExecutorConsumerOnInit; return this; } + + public Builder setWorkspaceFileHeaderListener( + WorkspaceFileHeaderListener workspaceFileHeaderListener) { + this.workspaceFileHeaderListener = workspaceFileHeaderListener; + return this; + } + } + + /** + * Listener class to subscribe for WORKSPACE file header changes. + * + *

Changes to WORKSPACE file header are computed before the files difference is computed in + * {@link #handleDiffs(ExtendedEventHandler, boolean, OptionsProvider)} + */ + public interface WorkspaceFileHeaderListener { + void workspaceHeaderChanged(@Nullable WorkspaceFileValue newValue); } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index a8ee477102a27a..9df4bda1d3f617 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -140,6 +140,7 @@ import com.google.devtools.build.lib.skyframe.PackageRootsNoSymlinkCreation; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor; +import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor.WorkspaceFileHeaderListener; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue; import com.google.devtools.build.lib.syntax.StarlarkSemantics; @@ -279,6 +280,7 @@ public void initializeSkyframeExecutor(boolean doPackageLoadingChecks) throws Ex DefaultBuildOptionsForTesting.getDefaultBuildOptionsForTest(ruleClassProvider)) .setWorkspaceStatusActionFactory(workspaceStatusActionFactory) .setExtraSkyFunctions(analysisMock.getSkyFunctions(directories)) + .setWorkspaceFileHeaderListener(getWorkspaceFileListener()) .build(); TestConstants.processSkyframeExecutorForTesting(skyframeExecutor); skyframeExecutor.injectExtraPrecomputedValues(extraPrecomputedValues); @@ -319,6 +321,10 @@ protected ConfiguredRuleClassProvider getRuleClassProvider() { return getAnalysisMock().createRuleClassProvider(); } + protected WorkspaceFileHeaderListener getWorkspaceFileListener() { + return null; + } + protected PackageFactory getPackageFactory() { return pkgFactory; } diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java index 1e392e2c9f98e5..620a77e006d135 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java @@ -48,6 +48,7 @@ import com.google.devtools.build.lib.skyframe.SkyframeExecutor; import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileStatus; @@ -581,7 +582,7 @@ private ModifiedFileSet getModifiedFileSet() { return builder.build(); } - void sync() throws InterruptedException { + void sync() throws InterruptedException, AbruptExitException { clock.advanceMillis(1); modifiedFileSet = getModifiedFileSet(); 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 540f760a072231..e8754f758ac4c9 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 @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.NullEventHandler; import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.PackageFactory; @@ -32,6 +33,7 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.WorkspaceFileValue; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; +import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor.WorkspaceFileHeaderListener; 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; @@ -47,6 +49,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; +import javax.annotation.Nullable; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.junit.Before; @@ -68,6 +71,7 @@ public class WorkspaceFileFunctionTest extends BuildViewTestCase { private ExternalPackageFunction externalSkyFunc; private WorkspaceASTFunction astSkyFunc; private FakeFileValue fakeWorkspaceFileValue; + private TestWorkspaceFileListener testWorkspaceFileListener; static class FakeFileValue extends FileValue { private boolean exists; @@ -128,6 +132,12 @@ public final void setUp() throws Exception { fakeWorkspaceFileValue = new FakeFileValue(); } + @Override + protected WorkspaceFileHeaderListener getWorkspaceFileListener() { + testWorkspaceFileListener = new TestWorkspaceFileListener(); + return testWorkspaceFileListener; + } + @Override protected Iterable getEnvironmentExtensions() { return ImmutableList.of(new PackageFactory.EmptyEnvironmentExtension()); @@ -389,4 +399,42 @@ public void testListBindFunction() throws Exception { .isEqualTo(Label.parseAbsolute("//foo:bar", ImmutableMap.of())); MoreAsserts.assertNoEvents(pkg.getEvents()); } + + @Test + public void testWorkspaceFileValueListener() throws Exception { + // Normally, syscalls cache is reset in the sync() method of the SkyframeExecutor, before + // diffing. + // But here we are calling only actual diffing part, exposed for testing: + // handleDiffsForTesting(), so we better turn off the syscalls cache. + skyframeExecutor.turnOffSyscallCacheForTesting(); + + createWorkspaceFile("workspace(name = 'old')"); + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + assertThat(testWorkspaceFileListener.getLastWorkspaceName()).isEqualTo("old"); + assertThat(testWorkspaceFileListener.getCnt()).isEqualTo(1); + + createWorkspaceFile("workspace(name = 'changed')"); + skyframeExecutor.handleDiffsForTesting(NullEventHandler.INSTANCE); + assertThat(testWorkspaceFileListener.getLastWorkspaceName()).isEqualTo("changed"); + assertThat(testWorkspaceFileListener.getCnt()).isEqualTo(2); + } + + private static class TestWorkspaceFileListener implements WorkspaceFileHeaderListener { + private String lastWorkspaceName; + private int cnt = 0; + + @Override + public void workspaceHeaderChanged(@Nullable WorkspaceFileValue newValue) { + ++cnt; + lastWorkspaceName = newValue != null ? newValue.getPackage().getWorkspaceName() : null; + } + + private String getLastWorkspaceName() { + return lastWorkspaceName; + } + + private int getCnt() { + return cnt; + } + } }