Repository: bazelbuild/bazel
Issue: Windows, external repositories: correctness bug with Starlark rule in external repo
Labels: type: bug
P1
area-Windows
team-OSS

### Description of the problem / feature request:

On Windows, if `//src:foo` depends on `@repo2//:gen`, which is a Starlark rule whose definition is in `@repo1//:gen_cc.bzl`, then changing `gen_cc.bzl` does not trigger a rebuild of `//src:foo` even though it should.

On Linux, the same thing works correctly.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

https://github.com/laszlocsomor/projects/tree/81325c41c015dfe02d7639ab8940f52d72da8093/bazel/starlark-rule-change-no-picked-up-from-repo

### What operating system are you running Bazel on?

Windows 10

### What's the output of `bazel info release`?

`release 0.21.0`

Issue comments:
Workaround: call `bazel shutdown` then `bazel run //src:foo` again.
I bet this is some symlink-related issue. I suspect Bazel on Windows copies files in the roots of external repositories instead of symlinking them, and that's why it doesn't notice the change.

I'll proceed to check this theory.
My theory is correct:
- moving `gen_cc.bzl` to a subdirectory `sub` in under `repo1` will result in correct behavior, because `<output_base>/external/repo1/sub` is a junction pointing to the real `sub` directory
- adding a file to or removing a file from the root of repo1 between builds is picked up, but modifying a file is not; to see this, check the contents of `<output_base>/external/repo1`
- changing `new_local_repository` to `local_repository` and adding a BUILD file to `repo1` will result in correct behavior, even when the bzl file is in repo1's root; therefore this is a bug with `new_local_repository`, ~and this bug might actually be a duplicate of #3908~
Could be the same bug as https://github.com/bazelbuild/bazel/issues/6351

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
index 5c164c7d807ddc..9c4c4aa3c325ce 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.repository;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.actions.InconsistentFilesystemException;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -24,6 +25,7 @@
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
@@ -32,6 +34,7 @@
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
 import java.util.Map;
 
@@ -90,10 +93,19 @@ public RepositoryDirectoryValue.Builder fetch(Rule rule, Path outputDirectory,
       throw new RepositoryFunctionException(e, Transience.PERSISTENT);
     }
 
-    // fetch() creates symlinks to each child under 'path' and DiffAwareness handles checking all
-    // of these files and directories for changes. However, if a new file/directory is added
-    // directly under 'path', Bazel doesn't know that this has to be symlinked in. Thus, this
-    // creates a dependency on the contents of the 'path' directory.
+    // fetch() creates symlinks to each child under 'path'.
+    //
+    // On Linux/macOS (i.e. where file symlinks are supported), DiffAwareness handles checking all
+    // of these files and directories for changes.
+    //
+    // On Windows (when file symlinks are disabled), the supposed symlinks are actually copies and
+    // DiffAwareness doesn't pick up changes of the "symlink" targets. To rectify that, we request
+    // FileValues for each child of 'path'.
+    // See https://github.com/bazelbuild/bazel/issues/7063
+    //
+    // Furthermore, if a new file/directory is added directly under 'path', Bazel doesn't know that
+    // this has to be symlinked in. So we also create a dependency on the contents of the 'path'
+    // directory.
     SkyKey dirKey = DirectoryListingValue.key(dirPath);
     DirectoryListingValue directoryValue;
     try {
@@ -106,6 +118,20 @@ public RepositoryDirectoryValue.Builder fetch(Rule rule, Path outputDirectory,
       return null;
     }
 
+    Map<SkyKey, SkyValue> fileValues =
+        env.getValues(
+            Iterables.transform(
+                directoryValue.getDirents(),
+                e -> (SkyKey) FileValue.key(
+                         RootedPath.toRootedPath(
+                             dirPath.getRoot(),
+                             dirPath.getRootRelativePath().getRelative(e.getName())))));
+    for (Map.Entry<?, ?> m : fileValues.entrySet()) {
+      if (m.getValue() == null) {
+        return null;
+      }
+    }
+
     // Link x/y/z to /some/path/to/y/z.
     if (!symlinkLocalRepositoryContents(outputDirectory, path)) {
       return null;
@@ -114,7 +140,8 @@ public RepositoryDirectoryValue.Builder fetch(Rule rule, Path outputDirectory,
     fileHandler.finishFile(rule, outputDirectory, markerData);
     env.getListener().post(resolve(rule, directories));
 
-    return RepositoryDirectoryValue.builder().setPath(outputDirectory).setSourceDir(directoryValue);
+    return RepositoryDirectoryValue.builder().setPath(outputDirectory).setSourceDir(directoryValue)
+               .setFileValues(fileValues);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java
index c2ce139864e83c..68323a517e57e1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java
@@ -17,6 +17,7 @@
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Interner;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
@@ -25,8 +26,10 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.AbstractSkyKey;
 import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Arrays;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /** A local view of an external repository. */
@@ -53,13 +56,16 @@ public static final class SuccessfulRepositoryDirectoryValue extends RepositoryD
     private final boolean fetchingDelayed;
     @Nullable private final byte[] digest;
     @Nullable private final DirectoryListingValue sourceDir;
+    private final ImmutableMap<SkyKey, SkyValue> fileValues;
 
     private SuccessfulRepositoryDirectoryValue(
-        Path path, boolean fetchingDelayed, DirectoryListingValue sourceDir, byte[] digest) {
+        Path path, boolean fetchingDelayed, DirectoryListingValue sourceDir, byte[] digest,
+        ImmutableMap<SkyKey, SkyValue> fileValues) {
       this.path = path;
       this.fetchingDelayed = fetchingDelayed;
       this.sourceDir = sourceDir;
       this.digest = digest;
+      this.fileValues = fileValues;
     }
 
     @Override
@@ -87,14 +93,15 @@ public boolean equals(Object other) {
         SuccessfulRepositoryDirectoryValue otherValue = (SuccessfulRepositoryDirectoryValue) other;
         return Objects.equal(path, otherValue.path)
             && Objects.equal(sourceDir, otherValue.sourceDir)
-            && Arrays.equals(digest, otherValue.digest);
+            && Arrays.equals(digest, otherValue.digest)
+            && Objects.equal(fileValues, otherValue.fileValues);
       }
       return false;
     }
 
     @Override
     public int hashCode() {
-      return Objects.hashCode(path, sourceDir, Arrays.hashCode(digest));
+      return Objects.hashCode(path, sourceDir, Arrays.hashCode(digest), fileValues);
     }
 
     @Override
@@ -162,6 +169,7 @@ public static class Builder {
     private boolean fetchingDelayed = false;
     private byte[] digest = null;
     private DirectoryListingValue sourceDir = null;
+    private Map<SkyKey, SkyValue> fileValues = ImmutableMap.of();
 
     private Builder() {}
 
@@ -185,13 +193,19 @@ public Builder setSourceDir(DirectoryListingValue sourceDir) {
       return this;
     }
 
+    public Builder setFileValues(Map<SkyKey, SkyValue> fileValues) {
+      this.fileValues = fileValues;
+      return this;
+    }
+
     public SuccessfulRepositoryDirectoryValue build() {
       Preconditions.checkNotNull(path, "Repository path must be specified!");
       // Only if fetching is delayed then we are allowed to have a null digest.
       if (!this.fetchingDelayed) {
         Preconditions.checkNotNull(digest, "Repository marker digest must be specified!");
       }
-      return new SuccessfulRepositoryDirectoryValue(path, fetchingDelayed, sourceDir, digest);
+      return new SuccessfulRepositoryDirectoryValue(path, fetchingDelayed, sourceDir, digest,
+          ImmutableMap.copyOf(fileValues));
     }
   }
 }


Test patch:
diff --git a/src/test/py/bazel/bazel_external_repository_test.py b/src/test/py/bazel/bazel_external_repository_test.py
index 3ed67f9b264802..2950ade954ea82 100644
--- a/src/test/py/bazel/bazel_external_repository_test.py
+++ b/src/test/py/bazel/bazel_external_repository_test.py
@@ -131,5 +131,50 @@ def testNewHttpArchiveWithSymlinks(self):
     ])
     self.assertEqual(exit_code, 0, os.linesep.join(stderr))
 
+  def _CreatePyWritingStarlarkRule(self, print_string):
+    self.ScratchFile('repo/foo.bzl', [
+        "def _impl(ctx):",
+        "  ctx.actions.write(",
+        "      output = ctx.outputs.out,",
+        "      content = \"\"\"from __future__ import print_function",
+        "print('%s')\"\"\"," % print_string,
+        "  )",
+        "  return [DefaultInfo(files = depset(direct = [ctx.outputs.out]))]",
+        "",
+        "gen_py = rule(",
+        "    implementation = _impl,",
+        "    outputs = {'out': '%{name}.py'},",
+        ")",
+    ])
+
+  def testNewLocalRepositoryNoticesFileChangeInRepoRoot(self):
+    """Regression test for https://github.com/bazelbuild/bazel/issues/7063."""
+    self.ScratchFile('WORKSPACE', [
+        'new_local_repository(',
+        '    name = "r",',
+        '    path = "./repo",',
+        '    build_file_content = "exports_files([\'foo.bzl\'])",',
+        ')',
+    ])
+    self.ScratchFile('repo/WORKSPACE')
+    self._CreatePyWritingStarlarkRule('hello!')
+    self.ScratchFile('BUILD', [
+        'load("@r//:foo.bzl", "gen_py")',
+        'gen_py(name = "gen")',
+        'py_binary(name = "bin", srcs = [":gen"], main = "gen.py")',
+    ])
+
+    exit_code, stdout, stderr = self.RunBazel(['run', '//:bin'])
+    self.assertEqual(exit_code, 0, os.linesep.join(stderr))
+    self.assertIn('hello!', os.linesep.join(stdout))
+
+    # Modify the definition of the Starlark rule in the external repository.
+    # The py_binary rule should notice this and rebuild.
+    self._CreatePyWritingStarlarkRule('world')
+    exit_code, stdout, stderr = self.RunBazel(['run', '//:bin'])
+    self.assertEqual(exit_code, 0, os.linesep.join(stderr))
+    self.assertNotIn('hello!', os.linesep.join(stdout))
+    self.assertIn('world', os.linesep.join(stdout))
+
 if __name__ == '__main__':
   unittest.main()