From 4f647e859203dc3a2f26ab088b95ef1a58c2e3ea Mon Sep 17 00:00:00 2001 From: ulfjack Date: Tue, 9 May 2017 07:45:40 -0400 Subject: Change FileContentsProxy to use ctime instead of mtime This gives us better reliability for detecting file changes; especially in cases where tools intentionally do not update mtime. Fixes #1525. PiperOrigin-RevId: 155490849 --- .../lib/pkgcache/BuildFileModificationTest.java | 216 +++++++++++++++++++++ .../build/lib/pkgcache/PackageCacheTest.java | 57 ------ .../build/lib/skyframe/FileContentsProxyTest.java | 100 ++++++++++ .../build/lib/skyframe/FileFunctionTest.java | 6 +- .../build/lib/skyframe/TreeArtifactBuildTest.java | 7 +- 5 files changed, 322 insertions(+), 64 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/pkgcache/BuildFileModificationTest.java create mode 100644 src/test/java/com/google/devtools/build/lib/skyframe/FileContentsProxyTest.java (limited to 'src/test/java') diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/BuildFileModificationTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/BuildFileModificationTest.java new file mode 100644 index 0000000000..91297b50b1 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/BuildFileModificationTest.java @@ -0,0 +1,216 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.pkgcache; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; +import com.google.devtools.build.lib.analysis.util.AnalysisMock; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.packages.NoSuchPackageException; +import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.skyframe.DiffAwareness; +import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; +import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName; +import com.google.devtools.build.lib.skyframe.PrecomputedValue; +import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor; +import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker; +import com.google.devtools.build.lib.skyframe.SkyframeExecutor; +import com.google.devtools.build.lib.syntax.SkylarkSemanticsOptions; +import com.google.devtools.build.lib.testutil.FoundationTestCase; +import com.google.devtools.build.lib.testutil.ManualClock; +import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.ModifiedFileSet; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.common.options.OptionsParser; +import java.nio.charset.StandardCharsets; +import java.util.UUID; +import java.util.logging.Level; +import java.util.logging.Logger; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Tests for package loading. + */ +@RunWith(JUnit4.class) +public class BuildFileModificationTest extends FoundationTestCase { + + private ManualClock clock = new ManualClock(); + private AnalysisMock analysisMock; + private ConfiguredRuleClassProvider ruleClassProvider; + private SkyframeExecutor skyframeExecutor; + + @Before + public final void disableLogging() throws Exception { + Logger.getLogger("com.google.devtools").setLevel(Level.SEVERE); + } + + @Before + public final void initializeSkyframeExecutor() throws Exception { + analysisMock = AnalysisMock.get(); + ruleClassProvider = analysisMock.createRuleClassProvider(); + BlazeDirectories directories = + new BlazeDirectories(outputBase, outputBase, rootDirectory, analysisMock.getProductName()); + skyframeExecutor = + SequencedSkyframeExecutor.create( + analysisMock + .getPackageFactoryForTesting() + .create(ruleClassProvider, scratch.getFileSystem()), + directories, + null, /* BinTools */ + null, /* workspaceStatusActionFactory */ + ruleClassProvider.getBuildInfoFactories(), + ImmutableList.of(), + Predicates.alwaysFalse(), + AnalysisMock.get().getSkyFunctions(), + ImmutableList.of(), + ImmutableList.of(), + analysisMock.getProductName(), + CrossRepositoryLabelViolationStrategy.ERROR, + ImmutableList.of(BuildFileName.BUILD_DOT_BAZEL, BuildFileName.BUILD)); + OptionsParser parser = OptionsParser.newOptionsParser( + PackageCacheOptions.class, SkylarkSemanticsOptions.class); + analysisMock.getInvocationPolicyEnforcer().enforce(parser); + setUpSkyframe( + parser.getOptions(PackageCacheOptions.class), + parser.getOptions(SkylarkSemanticsOptions.class)); + } + + private void setUpSkyframe( + PackageCacheOptions packageCacheOptions, + SkylarkSemanticsOptions skylarkSemanticsOptions) { + PathPackageLocator pkgLocator = PathPackageLocator.create( + null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); + packageCacheOptions.showLoadingProgress = true; + packageCacheOptions.globbingThreads = 7; + skyframeExecutor.preparePackageLoading( + pkgLocator, + packageCacheOptions, + skylarkSemanticsOptions, + analysisMock.getDefaultsPackageContent(), + UUID.randomUUID(), + ImmutableMap.of(), + ImmutableMap.of(), + new TimestampGranularityMonitor(clock)); + skyframeExecutor.setDeletedPackages( + ImmutableSet.copyOf(packageCacheOptions.getDeletedPackages())); + } + + @Override + protected FileSystem createFileSystem() { + return new InMemoryFileSystem(clock); + } + + private void invalidatePackages() throws InterruptedException { + skyframeExecutor.invalidateFilesUnderPathForTesting( + reporter, ModifiedFileSet.EVERYTHING_MODIFIED, rootDirectory); + } + + private Package getPackage(String packageName) + throws NoSuchPackageException, InterruptedException { + return skyframeExecutor.getPackageManager().getPackage(reporter, + PackageIdentifier.createInMainRepo(packageName)); + } + + @Test + public void testCTimeChangeDetectedWithError() throws Exception { + reporter.removeHandler(failFastHandler); + Path build = scratch.file( + "a/BUILD", "cc_library(name='a', feet='stinky')".getBytes(StandardCharsets.ISO_8859_1)); + Package a1 = getPackage("a"); + assertTrue(a1.containsErrors()); + assertContainsEvent("//a:a: no such attribute 'feet'"); + eventCollector.clear(); + // writeContent updates mtime and ctime. Note that we keep the content length exactly the same. + clock.advanceMillis(1); + FileSystemUtils.writeContent( + build, "cc_library(name='a', srcs=['a.cc'])".getBytes(StandardCharsets.ISO_8859_1)); + + invalidatePackages(); + Package a2 = getPackage("a"); + assertNotSame(a1, a2); + assertFalse(a2.containsErrors()); + assertNoEvents(); + } + + @Test + public void testCTimeChangeDetected() throws Exception { + Path path = scratch.file( + "pkg/BUILD", "cc_library(name = 'foo')\n".getBytes(StandardCharsets.ISO_8859_1)); + Package oldPkg = getPackage("pkg"); + + // Note that the content has exactly the same length as before. + clock.advanceMillis(1); + FileSystemUtils.writeContent( + path, "cc_library(name = 'bar')\n".getBytes(StandardCharsets.ISO_8859_1)); + assertSame(oldPkg, getPackage("pkg")); // Change only becomes visible after invalidatePackages. + + invalidatePackages(); + + Package newPkg = getPackage("pkg"); + assertNotSame(oldPkg, newPkg); + assertNotNull(newPkg.getTarget("bar")); + } + + @Test + public void testLengthChangeDetected() throws Exception { + reporter.removeHandler(failFastHandler); + Path build = scratch.file( + "a/BUILD", "cc_library(name='a', srcs=['a.cc'])".getBytes(StandardCharsets.ISO_8859_1)); + Package a1 = getPackage("a"); + eventCollector.clear(); + // Note that we didn't advance the clock, so ctime/mtime is the same as before. + // However, the file contents are one byte longer. + FileSystemUtils.writeContent( + build, "cc_library(name='ab', srcs=['a.cc'])".getBytes(StandardCharsets.ISO_8859_1)); + + invalidatePackages(); + Package a2 = getPackage("a"); + assertNotSame(a1, a2); + assertNoEvents(); + } + + @Test + public void testTouchedBuildFileCausesReloadAfterSync() throws Exception { + Path path = scratch.file("pkg/BUILD", + "cc_library(name = 'foo')"); + + Package oldPkg = getPackage("pkg"); + // Change ctime to 1. + clock.advanceMillis(1); + path.setLastModifiedTime(1001); + assertSame(oldPkg, getPackage("pkg")); // change not yet visible + + invalidatePackages(); + + Package newPkg = getPackage("pkg"); + assertNotSame(oldPkg, newPkg); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java index 165db96d5c..d31fa524c9 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.pkgcache; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; @@ -313,62 +312,6 @@ public class PackageCacheTest extends FoundationTestCase { assertNoEvents(); } - @Test - public void testPackageInErrorReloadedWhenFixed() throws Exception { - reporter.removeHandler(failFastHandler); - Path build = scratch.file("a/BUILD", "cc_library(name='a', feet='stinky')"); - build.setLastModifiedTime(1); - Package a1 = getPackage("a"); - assertTrue(a1.containsErrors()); - assertContainsEvent("//a:a: no such attribute 'feet'"); - - eventCollector.clear(); - build.delete(); - build = scratch.file("a/BUILD", "cc_library(name='a', srcs=['a.cc'])"); - build.setLastModifiedTime(2); - invalidatePackages(); - Package a2 = getPackage("a"); - assertNotSame(a1, a2); - assertFalse(a2.containsErrors()); - assertNoEvents(); - } - - @Test - public void testModifiedBuildFileCausesReloadAfterSync() throws Exception { - Path path = scratch.file("pkg/BUILD", - "cc_library(name = 'foo')"); - path.setLastModifiedTime(1000); - - Package oldPkg = getPackage("pkg"); - // modify BUILD file (and change its timestamp) - path.delete(); - scratch.file("pkg/BUILD", "cc_library(name = 'bar')"); - path.setLastModifiedTime(999); // earlier; mtime doesn't have to advance - assertSame(oldPkg, getPackage("pkg")); // change not yet visible - - invalidatePackages(); - - Package newPkg = getPackage("pkg"); - assertNotSame(oldPkg, newPkg); - assertNotNull(newPkg.getTarget("bar")); - } - - @Test - public void testTouchedBuildFileCausesReloadAfterSync() throws Exception { - Path path = scratch.file("pkg/BUILD", - "cc_library(name = 'foo')"); - path.setLastModifiedTime(1000); - - Package oldPkg = getPackage("pkg"); - path.setLastModifiedTime(1001); - assertSame(oldPkg, getPackage("pkg")); // change not yet visible - - invalidatePackages(); - - Package newPkg = getPackage("pkg"); - assertNotSame(oldPkg, newPkg); - } - @Test public void testMovedBuildFileCausesReloadAfterSync() throws Exception { Path buildFile1 = scratch.file("pkg/BUILD", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileContentsProxyTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileContentsProxyTest.java new file mode 100644 index 0000000000..0ca3da5f80 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileContentsProxyTest.java @@ -0,0 +1,100 @@ +// Copyright 2017 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.testing.EqualsTester; +import com.google.devtools.build.lib.vfs.FileStatus; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link FileContentsProxy}. */ +@RunWith(JUnit4.class) +public class FileContentsProxyTest { + /** A simple implementation of FileStatus for testing. */ + public static final class InjectedStat implements FileStatus { + private final long mtime; + private final long ctime; + private final long size; + private final long nodeId; + + public InjectedStat(long mtime, long ctime, long size, long nodeId) { + this.mtime = mtime; + this.ctime = ctime; + this.size = size; + this.nodeId = nodeId; + } + + @Override + public boolean isFile() { + return true; + } + + @Override + public boolean isSpecialFile() { + return false; + } + + @Override + public boolean isDirectory() { + return false; + } + + @Override + public boolean isSymbolicLink() { + return false; + } + + @Override + public long getSize() { + return size; + } + + @Override + public long getLastModifiedTime() { + return mtime; + } + + @Override + public long getLastChangeTime() { + return ctime; + } + + @Override + public long getNodeId() { + return nodeId; + } + } + + @Test + public void equalsAndHashCode() { + new EqualsTester() + .addEqualityGroup(new FileContentsProxy(1L, 2L), new FileContentsProxy(1L, 2L)) + .addEqualityGroup(new FileContentsProxy(1L, 4L)) + .addEqualityGroup(new FileContentsProxy(3L, 4L)) + .addEqualityGroup(new FileContentsProxy(-1L, -1L)) + .testEquals(); + } + + @Test + public void fromStat() throws Exception { + FileContentsProxy p1 = + FileContentsProxy.create( + new InjectedStat(/*mtime=*/1, /*ctime=*/2, /*size=*/3, /*nodeId=*/4)); + assertThat(p1.getCTime()).isEqualTo(2); + assertThat(p1.getNodeId()).isEqualTo(4); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index 830b91c786..8e1ff34c53 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java @@ -558,13 +558,15 @@ public class FileFunctionTest { } @Test - public void testSymlinkTargetContentsChangeModTime() throws Exception { + public void testSymlinkTargetContentsChangeCTime() throws Exception { fastDigest = false; Path fooPath = file("foo"); FileSystemUtils.writeContentAsLatin1(fooPath, "foo"); Path p = symlink("symlink", "foo"); FileValue a = valueForPath(p); - fooPath.setLastModifiedTime(88); + manualClock.advanceMillis(1); + fooPath.chmod(0555); + manualClock.advanceMillis(1); FileValue b = valueForPath(p); assertThat(b).isNotEqualTo(a); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index c682340282..a3853e6540 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -42,7 +42,6 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.actions.Root; -import com.google.devtools.build.lib.actions.cache.InjectedStat; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.TestAction; @@ -687,13 +686,11 @@ public class TreeArtifactBuildTest extends TimestampBuilderTestCase { MetadataHandler md = actionExecutionContext.getMetadataHandler(); FileStatus stat = outOneFileOne.getPath().stat(Symlinks.NOFOLLOW); md.injectDigest(outOneFileOne, - new InjectedStat(stat.getLastModifiedTime(), stat.getSize(), stat.getNodeId()), - Hashing.md5().hashString("one", Charset.forName("UTF-8")).asBytes()); + stat, Hashing.md5().hashString("one", Charset.forName("UTF-8")).asBytes()); stat = outOneFileTwo.getPath().stat(Symlinks.NOFOLLOW); md.injectDigest(outOneFileTwo, - new InjectedStat(stat.getLastModifiedTime(), stat.getSize(), stat.getNodeId()), - Hashing.md5().hashString("two", Charset.forName("UTF-8")).asBytes()); + stat, Hashing.md5().hashString("two", Charset.forName("UTF-8")).asBytes()); } catch (Exception e) { throw new RuntimeException(e); } -- cgit v1.2.3