Treat existence of managed directories as a part of repository dirtiness. - If managed directory is declared for the external repository and does not exist (probably was deleted by user), make external repository dirty and re-fetch it. - Add tests to demonstrate the added behavior to ManagedDirectoriesBlackBoxTest. - Closes #8487. Closes #8564. PiperOrigin-RevId: 251882207
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 961a95a..5c94b2e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java
@@ -1,4 +1,4 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. +// Copyright 2019 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. @@ -11,6 +11,7 @@ // 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.bazel; @@ -173,7 +174,8 @@ builder.setManagedDirectoriesKnowledge(managedDirectoriesKnowledge); RepositoryDirectoryDirtinessChecker customDirtinessChecker = - new RepositoryDirectoryDirtinessChecker(managedDirectoriesKnowledge); + new RepositoryDirectoryDirtinessChecker( + directories.getWorkspace(), managedDirectoriesKnowledge); builder.addCustomDirtinessChecker(customDirtinessChecker); // Create the repository function everything flows through. builder.addSkyFunction(SkyFunctions.REPOSITORY, new RepositoryLoaderFunction());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 7d3ddcf..8e77200 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -1,4 +1,4 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. +// Copyright 2019 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. @@ -11,9 +11,12 @@ // 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.rules.repository; +import static com.google.devtools.build.lib.rules.repository.RepositoryDirectoryDirtinessChecker.managedDirectoriesExist; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.base.Preconditions; @@ -178,11 +181,13 @@ // generally fast and they do not depend on non-local data, so it does not make much sense to // try to cache them from across server instances. boolean fetchLocalRepositoryAlways = isFetch.get() && handler.isLocal(rule); - if (!fetchLocalRepositoryAlways) { + if (!fetchLocalRepositoryAlways + && managedDirectoriesExist(directories.getWorkspace(), managedDirectories)) { // For the non-local repositories, check if they are already up-to-date: // 1) unconditional fetching is not enabled, AND // 2) repository directory exists, AND - // 3) marker file correctly describes the current repository state + // 3) marker file correctly describes the current repository state, AND + // 4) managed directories, mapped to the repository, exist if (doNotFetchUnconditionally && repoRoot.exists()) { byte[] markerHash = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env); if (env.valuesMissing()) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryDirtinessChecker.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryDirtinessChecker.java index bbc0eaf..309b51d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryDirtinessChecker.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryDirtinessChecker.java
@@ -11,14 +11,18 @@ // 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.rules.repository; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.Objects; @@ -31,15 +35,17 @@ * <li>Only dirties {@link RepositoryDirectoryValue}s, if they were produced in a {@code * --nofetch} build, so that they are re-created on subsequent {@code --fetch} builds. The * alternative solution would be to reify the value of the flag as a Skyframe value. - * <li>Dirties repositories, if their managed directories were changed. + * <li>Dirties repositories, if their managed directories were changed or do not exist. * </ul> */ @VisibleForTesting public class RepositoryDirectoryDirtinessChecker extends SkyValueDirtinessChecker { + private final Path workspaceRoot; private final ManagedDirectoriesKnowledge managedDirectoriesKnowledge; public RepositoryDirectoryDirtinessChecker( - ManagedDirectoriesKnowledge managedDirectoriesKnowledge) { + Path workspaceRoot, ManagedDirectoriesKnowledge managedDirectoriesKnowledge) { + this.workspaceRoot = workspaceRoot; this.managedDirectoriesKnowledge = managedDirectoriesKnowledge; } @@ -71,6 +77,15 @@ repositoryValue.getManagedDirectories())) { return DirtyResult.dirty(skyValue); } + + if (!managedDirectoriesExist(workspaceRoot, repositoryValue.getManagedDirectories())) { + return DirtyResult.dirty(skyValue); + } return DirtyResult.notDirty(skyValue); } + + static boolean managedDirectoriesExist( + Path workspaceRoot, ImmutableSet<PathFragment> managedDirectories) { + return managedDirectories.stream().allMatch(pf -> workspaceRoot.getRelative(pf).exists()); + } }
diff --git a/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java b/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java index c5677c9..2cbbf74 100644 --- a/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java +++ b/src/test/java/com/google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java
@@ -4,13 +4,14 @@ // 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 +// 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.blackbox.tests.manageddirs; @@ -50,6 +51,39 @@ } @Test + public void testNodeModulesDeleted() throws Exception { + generateProject(); + buildExpectRepositoryRuleCalled(); + checkProjectFiles(); + + Path nodeModules = context().getWorkDir().resolve("node_modules"); + assertThat(nodeModules.toFile().isDirectory()).isTrue(); + PathUtils.deleteTree(nodeModules); + + buildExpectRepositoryRuleCalled(); + checkProjectFiles(); + } + + @Test + public void testNodeModulesDeletedAndRecreated() throws Exception { + generateProject(); + buildExpectRepositoryRuleCalled(); + checkProjectFiles(); + + Path nodeModules = context().getWorkDir().resolve("node_modules"); + assertThat(nodeModules.toFile().isDirectory()).isTrue(); + + Path nodeModulesBackup = context().getWorkDir().resolve("node_modules_backup"); + PathUtils.copyTree(nodeModules, nodeModulesBackup); + PathUtils.deleteTree(nodeModules); + + PathUtils.copyTree(nodeModulesBackup, nodeModules); + + buildExpectRepositoryRuleNotCalled(); + checkProjectFiles(); + } + + @Test public void testBuildProjectFetchNotRecalled() throws Exception { generateProject(); buildExpectRepositoryRuleCalled();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index f693bea..39cea3f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
@@ -1,4 +1,4 @@ -// Copyright 2017 The Bazel Authors. All rights reserved. +// Copyright 2019 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. @@ -11,6 +11,7 @@ // 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.rules.repository; @@ -77,9 +78,11 @@ import com.google.devtools.build.skyframe.SequentialBuildDriver; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunction.Environment; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.io.IOException; import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -115,7 +118,8 @@ managedDirectoriesKnowledge = new TestManagedDirectoriesKnowledge(); HttpDownloader downloader = Mockito.mock(HttpDownloader.class); RepositoryFunction localRepositoryFunction = new LocalRepositoryFunction(); - testSkylarkRepositoryFunction = new TestSkylarkRepositoryFunction(downloader); + testSkylarkRepositoryFunction = + new TestSkylarkRepositoryFunction(rootPath, downloader, managedDirectoriesKnowledge); ImmutableMap<String, RepositoryFunction> repositoryHandlers = ImmutableMap.of(LocalRepositoryRule.NAME, localRepositoryFunction); delegatorFunction = @@ -243,7 +247,7 @@ TestManagedDirectoriesKnowledge knowledge = new TestManagedDirectoriesKnowledge(); RepositoryDirectoryDirtinessChecker checker = - new RepositoryDirectoryDirtinessChecker(knowledge); + new RepositoryDirectoryDirtinessChecker(rootPath, knowledge); RepositoryName repositoryName = RepositoryName.create("@repo"); RepositoryDirectoryValue.Key key = RepositoryDirectoryValue.key(repositoryName); @@ -272,12 +276,18 @@ assertThat(checker.check(key, withManagedDirectories, tsgm).isDirty()).isTrue(); + Path managedDirectoryM = rootPath.getRelative("m"); + assertThat(managedDirectoryM.createDirectory()).isTrue(); + knowledge.setManagedDirectories( ImmutableMap.of(PathFragment.create("m"), RepositoryName.create("@other"))); assertThat(checker.check(key, withManagedDirectories, tsgm).isDirty()).isTrue(); knowledge.setManagedDirectories(ImmutableMap.of(PathFragment.create("m"), repositoryName)); assertThat(checker.check(key, withManagedDirectories, tsgm).isDirty()).isFalse(); + + managedDirectoryM.deleteTree(); + assertThat(checker.check(key, withManagedDirectories, tsgm).isDirty()).isTrue(); } @Test @@ -313,9 +323,23 @@ testSkylarkRepositoryFunction.reset(); loadRepo("repo1"); + // Nothing changed, fetch does not happen. assertThat(testSkylarkRepositoryFunction.isFetchCalled()).isFalse(); testSkylarkRepositoryFunction.reset(); + // Delete managed directory, fetch should happen again. + Path managedDirectory = rootPath.getRelative("dir1"); + managedDirectory.deleteTree(); + loadRepo("repo1"); + assertThat(testSkylarkRepositoryFunction.isFetchCalled()).isTrue(); + testSkylarkRepositoryFunction.reset(); + + // Change managed directories declaration, fetch should happen. + // NB: we are making sure that managed directories exist to check only the declaration changes + // were percepted. + rootPath.getRelative("dir1").createDirectory(); + rootPath.getRelative("dir2").createDirectory(); + managedDirectoriesKnowledge.setManagedDirectories( ImmutableMap.of( PathFragment.create("dir1"), @@ -354,9 +378,16 @@ private static class TestSkylarkRepositoryFunction extends SkylarkRepositoryFunction { private boolean fetchCalled = false; + private final Path workspaceRoot; + private final TestManagedDirectoriesKnowledge managedDirectoriesKnowledge; - private TestSkylarkRepositoryFunction(HttpDownloader downloader) { + private TestSkylarkRepositoryFunction( + Path workspaceRoot, + HttpDownloader downloader, + TestManagedDirectoriesKnowledge managedDirectoriesKnowledge) { super(downloader); + this.workspaceRoot = workspaceRoot; + this.managedDirectoriesKnowledge = managedDirectoriesKnowledge; } public void reset() { @@ -378,7 +409,18 @@ SkyKey key) throws RepositoryFunctionException, InterruptedException { fetchCalled = true; - return super.fetch(rule, outputDirectory, directories, env, markerData, key); + RepositoryDirectoryValue.Builder builder = + super.fetch(rule, outputDirectory, directories, env, markerData, key); + ImmutableSet<PathFragment> managedDirectories = + managedDirectoriesKnowledge.getManagedDirectories((RepositoryName) key.argument()); + try { + for (PathFragment managedDirectory : managedDirectories) { + workspaceRoot.getRelative(managedDirectory).createDirectory(); + } + } catch (IOException e) { + throw new RepositoryFunctionException(e, Transience.PERSISTENT); + } + return builder; } }