Repository: bazelbuild/bazel
Issue: Windows: new_local_repository doesn't reevaluate glob
Labels: type: bug
P1
area-Windows
team-OSS

### Description of the problem / feature request:

If a "new_local_repository" rule with "build_file_content" creates a rule with a glob, and a rule in the main repo depends on this one, then Bazel on Windows won't detect changes of the glob. (It does on Linux.)

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

1. `git clone https://github.com/laszlocsomor/projects`
1. modify the "new_local_repository" rule's "path" in "bazel/examples/repo_rule_glob_bug/main_repo/WORKSPACE" to use the correct path on your system
1. `cd bazel/examples/repo_rule_glob_bug/main_repo`
1. `bazel build //foo:all`

List the contents of "bazel-genfiles/foo/x.txt" and "bazel-genfiles/foo/y.txt": they will contain "external/external_repo/a.txt" and "foo/foo.txt" respectively, as they should.

Now create "b.txt" in the external_repo directory (next to "a.txt") and "foo/bar.txt" (next to "foo.txt"), and rebuild "//foo:all".

If you now list the contents of the output files, "bazel-genfiles/foo/x.txt" is stale, it sill contains only "external/external_repo/a.txt", whereas "bazel-genfiles/foo/y.txt" correctly contains "foo/foo.txt" and "foo/bar.txt".

The problem doesn't seem to manifest when using a "local_repository" rule and having a BUILD file in the external_repo directory.

### What operating system are you running Bazel on?

Windows 10

### What's the output of `bazel info release`?
```
release 0.17.2
```


Issue comments:
@laszlocsomor there should be only one team label. Is that better for your team or for External dependencies?
I cannot tell. We have the Windows expertise, the Ext.Deps team has the rule logic expertise.
I spent some time with #7063 so I'm also investigating this bug now.
More observation:
- When I add a new file to the external repo, Bazel *does* notice this, as proven by logging I added. Bazel creates a new `RegularDirectoryListingValue` SkyValue, however the downstream action is not re-executed.
- If I re-run the build once more without any change, Bazel creates yet another `RegularDirectoryListingValue` SkyValue, whose hashcode is different from that in the previous bullet point, and this time Bazel also re-executes the downstream genrule.
Found the culprit. It's complex, so I'll write it down in a document and paste here later.
## Repro

Let's have two directories, "A" and "B". "A" contains a WORKSPACE and BUILD file, while "B" contains just a dummy file:

/src/A/WORKSPACE:
```
new_local_repository(
    name = "B",
    build_file_content = """
filegroup(
    name = "F",
    srcs = glob(["*.txt"]),
    visibility = ["//visibility:public"],
)
""",
    path = "/src/B",
)
```

/src/A/BUILD:
```
genrule(
    name = "G",
    srcs = ["@B//:F"],
    outs = ["g.txt"],
    cmd = "echo $(SRCS) > $@",
)
```

/src/B/a.txt:
```
dummy
```

## Bug

If we build "//:G" inside /src/A, then g.txt correctly lists external/B/a.txt and nothing else.

If we add a new file /src/B/b.txt and rebuild :G, then g.txt should list external/B/a.txt and external/B/b.txt -- and it does on Linux but not on Windows.

If we build a third time on Windows without doing anything, g.txt will contain the correct output.

## Analysis

new_local_repository creates symlinks to every file and directory in its root, e.g. external/B/a.txt is a symlink to /src/B/a.txt. This is done as part of computing the REPOSITORY_DIRECTORY SkyValue, which is done by evaluating the [RepositoryDelegatorFunction](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java#L179), which calls into [NewLocalRepositoryFunction.fetch](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java#L100), which creates these symlinks. On Linux these are all symlinks indeed, but on Windows only the directory symlinks are really symlinks, the files are copies (because we cannot create file symlinks).

On both Linux and Windows, FILE_STATEs and DIRECTORY_LISTING_STATEs of paths under the external directory have special handling -- they all depend on the corresponding REPOSITORY_DIRECTORY. This dependency is established [here](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java#L48) and [here](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java#L54), by the [ExternalFilesHelper.maybeHandleExternalFile()](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java#L208) --&gt; [RepositoryFunction.addExternalFilesDependencies()](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java#L510) call chain, and it makes perfect sense, because it is the repository's SkyFunction that populates the external/B directory and creates the symlinks (or files) under it. Furthermore, the REPOSITORY_DIRECTORY depends on the DIRECTORY_LISTING_STATE of /src/B, so on both platforms Skyframe recomputes the REPOSITORY_DIRECTORY after we added /src/B/b.txt and correctly creates the new symlink (copy) under external/B/. This is all good.

The difference is that on Linux the GLOB depends on external/B/a.txt while on Windows it doesn't, because on Linux it's a symlink but on Windows it's a regular file. The reason is a performance optimization: GLOB only depends on FILE_STATEs of symlinks and directories but not files, to avoid recomputing the glob when a matched file's contents change, but to dirty the glob when a directory (or symlink to one) is replaced by a file with the same name (which affects recursive globbing).

On both Linux and Windows, the GLOB depends on the DIRECTORY_LISTING_STATE of external/B, and on Linux the GLOB also depends on the FILE_STATE of external/B/a.txt (because it's a symlink).
On both platforms, the FILE_STATE of external/B/a.txt depends on the REPOSITORY_DIRECTORY of @B, because the file is under external/. When we add the new file to B and rebuild, Skyframe's invalidation phase dirties the DIRECTORY_LISTING_STATE of /src/B, which dirties the REPOSITORY_DIRECTORY, which dirties the FILE_STATE of external/B/a.txt and on Linux this also dirties the GLOB.

This is a big difference: on Linux the GLOB is dirty but on Windows it's not. Upon closer inspection though, we notice that on Linux the GLOB is not recomputed but revalidated in the second build, and its SkyValue does not contain the newly added file -- however, the PACKAGE for "@B//" is recomputed and it does have the new glob contents and there's a new TARGET for @B//:b.txt -- what's going on?!

Turns out there's some package reloading logic [in PackageFunction](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java#L1188) that [uses Skyframe's dirtiness information](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java#L908) to decide which GLOBs to recompute, but doesn't actually recompute the GlobValue. On Linux the GLOB is dirtied in the second build but on Windows it's not, so on Linux we re-glob (and find external/B/b.txt) but on Windows we don't (even though external/B/b.txt is there).

Why doesn't the GLOB depend on the DIRECTORY_LISTING_STATE of external/B though? [It actually does](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java#L135). So why isn't this DIRECTORY_LISTING_STATE dirtied on Windows? It actually is! The problem is that the DIRECTORY_LISTING_STATE of external/B does not depend on the REPOSITORY_DIRECTORY of @B, because the [RepositoryFunction.addExternalFilesDependencies() call doesn't add this dependency if the path is the same as the repository's root path, in order to avoid a dependency cycle](https://github.com/bazelbuild/bazel/blob/d3d90630fa53d7d99e7e3de0a88bc0dfebefd50d/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java#L534-L551). Which is cool for the FILE_STATE of /src/B, but unnecessary for DIRECTORY_LISTING_STATE of it -- that's the bug.

If we change RepositoryFunction.addExternalFilesDependencies() to depend on the REPOSITORY_DIRECTORY for DIRECTORY_LISTING_STATEs, even of external/B itself, then everything works correctly on Windows. Becaues then adding the new file to /src/B will dirty the DIRECTORY_LISTING_STATE of /src/B, dirtying the REPOSITORY_DIRECTORY, dirtying the DIRECTORY_LISTING_STATE of /external/B, dirtying the GLOB, so the partial package reloading logic will re-compute that GLOB, even on Windows.

Ironically the bug was there on Linux too, just never manifested… because on Linux, external/B always contains symlinks, so the GLOB depends on their FILE_STATEs, and thus depends on the REPOSITORY_DIRECTORY.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index ee2141dce465bc..f037d79339f5e8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -508,7 +508,7 @@ protected static Path getExternalRepositoryDirectory(BlazeDirectories directorie
    * encourage nor optimize for since it is not common. So the set of external files is small.
    */
   public static void addExternalFilesDependencies(
-      RootedPath rootedPath, BlazeDirectories directories, Environment env)
+      RootedPath rootedPath, boolean isDirectory, BlazeDirectories directories, Environment env)
       throws IOException, InterruptedException {
     Path externalRepoDir = getExternalRepositoryDirectory(directories);
     PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir);
@@ -531,8 +531,8 @@ public static void addExternalFilesDependencies(
         return;
       }
 
-      if (repositoryPath.segmentCount() > 1) {
-        if (rule.getRuleClass().equals(LocalRepositoryRule.NAME)
+      if (isDirectory || repositoryPath.segmentCount() > 1) {
+        if (!isDirectory && rule.getRuleClass().equals(LocalRepositoryRule.NAME)
             && repositoryPath.endsWith(BuildFileName.WORKSPACE.getFilenameFragment())) {
           // Ignore this, there is a dependency from LocalRepositoryFunction->WORKSPACE file already
           return;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
index 49a6aaa2f2117c..c3d3ce9db9166b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
@@ -51,7 +51,7 @@ public DirectoryListingStateValue compute(SkyKey skyKey, Environment env)
     RootedPath dirRootedPath = (RootedPath) skyKey.argument();
 
     try {
-      externalFilesHelper.maybeHandleExternalFile(dirRootedPath, env);
+      externalFilesHelper.maybeHandleExternalFile(dirRootedPath, true, env);
       if (env.valuesMissing()) {
         return null;
       }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
index aa7f0362840109..37581eb6fe134e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
@@ -205,7 +205,8 @@ FileType getAndNoteFileType(RootedPath rootedPath) {
    * a {@link NonexistentImmutableExternalFileException} instead.
    */
   @ThreadSafe
-  void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
+  void maybeHandleExternalFile(
+      RootedPath rootedPath, boolean isDirectory, SkyFunction.Environment env)
       throws NonexistentImmutableExternalFileException, IOException, InterruptedException {
     FileType fileType = getAndNoteFileType(rootedPath);
     if (fileType == FileType.INTERNAL) {
@@ -225,6 +226,6 @@ void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
     Preconditions.checkState(
         externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
         externalFileAction);
-    RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env);
+    RepositoryFunction.addExternalFilesDependencies(rootedPath, isDirectory, directories, env);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
index 15b876351093b6..940b2d72429640 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java
@@ -45,7 +45,7 @@ public FileStateValue compute(SkyKey skyKey, Environment env)
     RootedPath rootedPath = (RootedPath) skyKey.argument();
 
     try {
-      externalFilesHelper.maybeHandleExternalFile(rootedPath, env);
+      externalFilesHelper.maybeHandleExternalFile(rootedPath, false, env);
       if (env.valuesMissing()) {
         return null;
       }


Test patch:
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index dc9f3033f44de2..547c888b35ec9b 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -806,3 +806,10 @@ sh_test(
     data = [":test-deps"],
     deps = ["@bazel_tools//tools/bash/runfiles"],
 )
+
+sh_test(
+    name = "new_local_repo_test",
+    srcs = ["new_local_repo_test.sh"],
+    data = [":test-deps"],
+    deps = ["@bazel_tools//tools/bash/runfiles"],
+)
diff --git a/src/test/shell/bazel/new_local_repo_test.sh b/src/test/shell/bazel/new_local_repo_test.sh
new file mode 100644
index 00000000000000..ede0ed014d166d
--- /dev/null
+++ b/src/test/shell/bazel/new_local_repo_test.sh
@@ -0,0 +1,113 @@
+#!/bin/bash
+#
+# 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.
+# 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.
+
+# --- begin runfiles.bash initialization ---
+# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash).
+set -euo pipefail
+if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+  if [[ -f "$0.runfiles_manifest" ]]; then
+    export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest"
+  elif [[ -f "$0.runfiles/MANIFEST" ]]; then
+    export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST"
+  elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+    export RUNFILES_DIR="$0.runfiles"
+  fi
+fi
+if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+  source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash"
+elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+  source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \
+            "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)"
+else
+  echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash"
+  exit 1
+fi
+# --- end runfiles.bash initialization ---
+
+source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+# `uname` returns the current platform, e.g "MSYS_NT-10.0" or "Linux".
+# `tr` converts all upper case letters to lower case.
+# `case` matches the result if the `uname | tr` expression to string prefixes
+# that use the same wildcards as names do in Bash, i.e. "msys*" matches strings
+# starting with "msys", and "*" matches everything (it's the default case).
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*)
+  # As of 2019-02-20, Bazel on Windows only supports MSYS Bash.
+  declare -r is_windows=true
+  ;;
+*)
+  declare -r is_windows=false
+  ;;
+esac
+
+if "$is_windows"; then
+  # Disable MSYS path conversion that converts path-looking command arguments to
+  # Windows paths (even if they arguments are not in fact paths).
+  export MSYS_NO_PATHCONV=1
+  export MSYS2_ARG_CONV_EXCL="*"
+fi
+
+# Regression test for GitHub issue #6351, see
+# https://github.com/bazelbuild/bazel/issues/6351#issuecomment-465488344
+function test_glob_in_synthesized_build_file() {
+  local -r pkg=${FUNCNAME[0]}
+  mkdir $pkg || fail "mkdir $pkg"
+  mkdir $pkg/A || fail "mkdir $pkg/A"
+  mkdir $pkg/B || fail "mkdir $pkg/B"
+
+  cat >$pkg/A/WORKSPACE <<'eof'
+new_local_repository(
+    name = "B",
+    build_file_content = """
+filegroup(
+    name = "F",
+    srcs = glob(["*.txt"]),
+    visibility = ["//visibility:public"],
+)
+""",
+    path = "../B",
+)
+eof
+
+  cat >$pkg/A/BUILD <<'eof'
+genrule(
+    name = "G",
+    srcs = ["@B//:F"],
+    outs = ["g.txt"],
+    cmd = "echo $(SRCS) > $@",
+)
+eof
+
+  echo "dummy" >$pkg/B/a.txt
+
+  # Build 1: x.txt should contain external/B/a.txt
+  cd $pkg/A
+  bazel build //:G || fail "build failed"
+  cat bazel-genfiles/g.txt >$TEST_log
+  expect_log "external/B/a.txt"
+  expect_not_log "external/B/b.txt"
+
+  # Build 2: add B/b.txt and see if the glob picks it up.
+  echo "dummy" > ../B/b.txt
+  bazel build //:G || fail "build failed"
+  cat bazel-genfiles/g.txt >$TEST_log
+  expect_log "external/B/a.txt"
+  expect_log "external/B/b.txt"
+}
+
+run_suite "new_local_repository correctness tests"