new_local_repo: fix correctness bug with glob()
In Skyframe, the DirectoryListingStateValue (DLS)
of an external repository's path will now depend
on the RepositoryDirectoryValue (RD).
Example: the DLS of [output_root]/external/X now
depends on the RD of `@X`.
This is relevant for `glob()` expressions in the
top-level package of the repository. The GlobValue
depends on the DLS in order to get notified about
added/removed files. In the root of external
directories, those files are added/removed by the
repository function. Hence the DLS should depend
on the evaluation of the repository function, i.e.
the creation of the RD. And that is exactly what
this commit now does -- it ensures this dependency
is declared.
The bugfix is specific to Windows. On Linux,
GlobValue depends on the DLS, but it also depends
on symlinks in the directory (to get notified
about a directory symlink getting replaced by a
file with the same name, which would affect
recursive globbing). And the symlinks depend on
the RD, so the GlobValue always depended on the
RD.
However on Windows the files in the root of the
repository are copies, not symlinks, and the Glob
does not depend on files (as a means of
optimization), so on Windows the GlobValue didn't
depend on the RD, which caused issue #6351.
More info: https://github.com/bazelbuild/bazel/issues/6351#issuecomment-465488344
Fixes https://github.com/bazelbuild/bazel/issues/6351
Closes #7473.
PiperOrigin-RevId: 234938677
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 ee2141d..f37ca6e 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 @@
* 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,9 @@
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 49a6aaa..c3d3ce9 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 @@
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 aa7f036..abc7577 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
@@ -197,15 +197,15 @@
}
/**
- * If this instance is configured with
- * {@link ExternalFileAction#DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS} and
- * {@code rootedPath} isn't under a package root then this adds a dependency on the //external
- * package. If the action is
+ * If this instance is configured with {@link
+ * ExternalFileAction#DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS} and {@code rootedPath} isn't
+ * under a package root then this adds a dependency on the //external package. If the action is
* {@link ExternalFileAction#ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS}, it will throw
* 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 +225,6 @@
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 15b8763..940b2d7 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 @@
RootedPath rootedPath = (RootedPath) skyKey.argument();
try {
- externalFilesHelper.maybeHandleExternalFile(rootedPath, env);
+ externalFilesHelper.maybeHandleExternalFile(rootedPath, false, env);
if (env.valuesMissing()) {
return null;
}
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index ec18516..885836b 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -816,3 +816,10 @@
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 100755
index 0000000..c41f65b
--- /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: g.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"
