From 6efc5b787ad3164cc2fb779c73377695032b4524 Mon Sep 17 00:00:00 2001 From: ichern Date: Thu, 6 Jun 2019 10:49:59 -0700 Subject: [PATCH] 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 --- .../lib/bazel/BazelRepositoryModule.java | 6 ++- .../RepositoryDelegatorFunction.java | 11 ++-- .../RepositoryDirectoryDirtinessChecker.java | 19 ++++++- .../ManagedDirectoriesBlackBoxTest.java | 36 ++++++++++++- .../repository/RepositoryDelegatorTest.java | 52 +++++++++++++++++-- 5 files changed, 111 insertions(+), 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 index 961a95a48182b0..5c94b2e835418d 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 @@ 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/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 7d3ddcfdc47675..8e772005cd5bb8 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 @@ 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/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 bbc0eaf894b15f..309b51dea42cbf 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 @@ *
  • 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. - *
  • Dirties repositories, if their managed directories were changed. + *
  • Dirties repositories, if their managed directories were changed or do not exist. * */ @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 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 c5677c9c4e12f9..2cbbf740544a38 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; @@ -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 index f693bead1a177d..39cea3fc35f983 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 @@ 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 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 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; } }