Treat existence of managed directories as a part of repository dirtin…

…ess. - 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 bazelbuild#8487. Closes bazelbuild#8564. PiperOrigin-RevId: 251882207
buchgr · Jun 6, 2019 · 6efc5b7 · 6efc5b7
1 parent d97392f
commit 6efc5b7
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 13 deletions.
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
@@ -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 @@ public void workspaceInit(
     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/...main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/...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 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     // 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/...a/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryDirtinessChecker.java b/...a/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 @@ public DirtyResult check(
         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/.../google/devtools/build/lib/blackbox/tests/manageddirs/ManagedDirectoriesBlackBoxTest.java b/.../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;
 
@@ -49,6 +50,39 @@ public void testBuildProject() throws Exception {
     checkProjectFiles();
   }
 
+  @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();

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
@@ -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 @@ public void setupDelegator() throws Exception {
     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 @@ public void testRepositoryDirtinessChecker() throws Exception {
     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 @@ public void testRepositoryDirtinessChecker() throws Exception {
 
     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 @@ public void testManagedDirectoriesCauseRepositoryReFetches() throws Exception {
     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 void loadRepo(String strippedRepoName) throws InterruptedException {
 
   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 @@ public RepositoryDirectoryValue.Builder fetch(
         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;
     }
   }