diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java index 4d1af807045492..ee1b77a16e2aa6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java @@ -1185,6 +1185,9 @@ public String readFile(Object path, String watch, StarlarkThread thread) p.toString(), getIdentifyingStringForLogging(), thread.getCallerLocation()); env.getListener().post(w); maybeWatch(p, ShouldWatch.fromString(watch)); + if (p.isDir()) { + throw Starlark.errorf("attempting to read() a directory: %s", p); + } try { return FileSystemUtils.readContent(p.getPath(), ISO_8859_1); } catch (IOException e) { @@ -1270,9 +1273,6 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) if (fileValue == null) { throw new NeedsSkyframeRestartException(); } - if (!fileValue.isFile() || fileValue.isSpecialFile()) { - throw Starlark.errorf("Not a regular file: %s", pair.second.asPath().getPathString()); - } recordedFileInputs.put(pair.first, RepoRecordedInput.File.fileValueToMarkerValue(fileValue)); } catch (IOException e) { @@ -1283,12 +1283,19 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch) @StarlarkMethod( name = "watch", doc = - "Tells Bazel to watch for changes to the given file. Any changes to the file will " + "Tells Bazel to watch for changes to the given path, whether or not it exists, or " + + "whether it's a file or a directory. Any changes to the file or directory will " + "invalidate this repository or module extension, and cause it to be refetched or " - + "re-evaluated next time.

Note that attempting to watch files inside the repo " - + "currently being fetched, or inside the working directory of the current module " - + "extension, will result in an error. A module extension attempting to watch a file " - + "outside the current Bazel workspace will also result in an error.", + + "re-evaluated next time.

\"Changes\" include changes to the contents of the file " + + "(if the path is a file); if the path was a file but is now a directory, or vice " + + "versa; and if the path starts or stops existing. Notably, this does not " + + "include changes to any files under the directory if the path is a directory. For " + // TODO: add `watch_dir()` + + "that, use watch_dir() instead.

Note " + + "that attempting to watch paths inside the repo currently being fetched, or inside " + + "the working directory of the current module extension, will result in an error. A " + + "module extension attempting to watch a path outside the current Bazel workspace " + + "will also result in an error.", parameters = { @Param( name = "path", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java index 8f0193410bbb4d..4823c294c1188a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkPath.java @@ -77,7 +77,6 @@ public String getBasename() { @StarlarkMethod( name = "readdir", - structField = false, doc = "The list of entries in the directory denoted by this path.") public ImmutableList readdir() throws IOException { ImmutableList.Builder builder = ImmutableList.builder(); @@ -118,11 +117,27 @@ public StarlarkPath getChild(Tuple relativePaths) throws EvalException { @StarlarkMethod( name = "exists", structField = true, - doc = "Returns true if the file denoted by this path exists.") + doc = + "Returns true if the file or directory denoted by this path exists.

Note that " + + "accessing this field does not cause the path to be watched. If you'd " + + "like the repo rule or module extension to be sensitive to the path's existence, " + + "use the watch() method on the context object.") public boolean exists() { return path.exists(); } + @StarlarkMethod( + name = "is_dir", + structField = true, + doc = + "Returns true if this path points to a directory.

Note that accessing this field does " + + "not cause the path to be watched. If you'd like the repo rule or module " + + "extension to be sensitive to whether the path is a directory or a file, use the " + + "watch() method on the context object.") + public boolean isDir() { + return path.isDirectory(); + } + @StarlarkMethod( name = "realpath", structField = true, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java index 8e051c7981f0dd..6d69087374669d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java @@ -198,9 +198,21 @@ private StarlarkPath externalPath(String method, Object pathObject) @ParamType(type = Label.class), @ParamType(type = StarlarkPath.class) }, - doc = "The path of the symlink to create, relative to the repository directory."), + doc = "The path of the symlink to create."), + @Param( + name = "watch_target", + defaultValue = "'auto'", + positional = false, + named = true, + doc = + "whether to watch the symlink target. Can be the string " + + "'yes', 'no', or 'auto'. Passing 'yes' is equivalent to immediately invoking " + + "the watch() method; passing 'no' does " + + "not attempt to watch the path; passing 'auto' will only attempt to watch " + + "the file when it is legal to do so (see watch() docs for more " + + "information."), }) - public void symlink(Object target, Object linkName, StarlarkThread thread) + public void symlink(Object target, Object linkName, String watchTarget, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { StarlarkPath targetPath = getPath("symlink()", target); StarlarkPath linkPath = getPath("symlink()", linkName); @@ -211,6 +223,7 @@ public void symlink(Object target, Object linkName, StarlarkThread thread) getIdentifyingStringForLogging(), thread.getCallerLocation()); env.getListener().post(w); + maybeWatch(targetPath, ShouldWatch.fromString(watchTarget)); try { checkInOutputDirectory("write", linkPath); makeDirectories(linkPath.getPath()); @@ -269,12 +282,25 @@ public void symlink(Object target, Object linkName, StarlarkThread thread) defaultValue = "True", named = true, doc = "set the executable flag on the created file, true by default."), + @Param( + name = "watch_template", + defaultValue = "'auto'", + positional = false, + named = true, + doc = + "whether to watch the template file. Can be the string " + + "'yes', 'no', or 'auto'. Passing 'yes' is equivalent to immediately invoking " + + "the watch() method; passing 'no' does " + + "not attempt to watch the file; passing 'auto' will only attempt to watch " + + "the file when it is legal to do so (see watch() docs for more " + + "information."), }) public void createFileFromTemplate( Object path, Object template, Dict substitutions, // expected Boolean executable, + String watchTemplate, StarlarkThread thread) throws RepositoryFunctionException, EvalException, InterruptedException { StarlarkPath p = getPath("template()", path); @@ -290,6 +316,10 @@ public void createFileFromTemplate( getIdentifyingStringForLogging(), thread.getCallerLocation()); env.getListener().post(w); + if (t.isDir()) { + throw Starlark.errorf("attempting to use a directory as template: %s", t); + } + maybeWatch(t, ShouldWatch.fromString(watchTemplate)); try { checkInOutputDirectory("write", p); makeDirectories(p.getPath()); @@ -385,8 +415,20 @@ public boolean delete(Object pathObject, StarlarkThread thread) named = true, defaultValue = "0", doc = "strip the specified number of leading components from file names."), + @Param( + name = "watch_patch", + defaultValue = "'auto'", + positional = false, + named = true, + doc = + "whether to watch the patch file. Can be the string " + + "'yes', 'no', or 'auto'. Passing 'yes' is equivalent to immediately invoking " + + "the watch() method; passing 'no' does " + + "not attempt to watch the file; passing 'auto' will only attempt to watch " + + "the file when it is legal to do so (see watch() docs for more " + + "information."), }) - public void patch(Object patchFile, StarlarkInt stripI, StarlarkThread thread) + public void patch(Object patchFile, StarlarkInt stripI, String watchPatch, StarlarkThread thread) throws EvalException, RepositoryFunctionException, InterruptedException { int strip = Starlark.toInt(stripI, "strip"); StarlarkPath starlarkPath = getPath("patch()", patchFile); @@ -397,6 +439,10 @@ public void patch(Object patchFile, StarlarkInt stripI, StarlarkThread thread) getIdentifyingStringForLogging(), thread.getCallerLocation()); env.getListener().post(w); + if (starlarkPath.isDir()) { + throw Starlark.errorf("attempting to use a directory as patch file: %s", starlarkPath); + } + maybeWatch(starlarkPath, ShouldWatch.fromString(watchPatch)); try { PatchUtil.apply(starlarkPath.getPath(), strip, workingDirectory); } catch (PatchFailedException e) { @@ -457,12 +503,25 @@ public void patch(Object patchFile, StarlarkInt stripI, StarlarkThread thread) + " any directory prefix adjustment. This can be used to extract archives that" + " contain non-Unicode filenames, or which have files that would extract to" + " the same path on case-insensitive filesystems."), + @Param( + name = "watch_archive", + defaultValue = "'auto'", + positional = false, + named = true, + doc = + "whether to watch the archive file. Can be the string " + + "'yes', 'no', or 'auto'. Passing 'yes' is equivalent to immediately invoking " + + "the watch() method; passing 'no' does " + + "not attempt to watch the file; passing 'auto' will only attempt to watch " + + "the file when it is legal to do so (see watch() docs for more " + + "information."), }) public void extract( Object archive, Object output, String stripPrefix, Dict renameFiles, // expected + String watchArchive, StarlarkThread thread) throws RepositoryFunctionException, InterruptedException, EvalException { StarlarkPath archivePath = getPath("extract()", archive); @@ -471,6 +530,10 @@ public void extract( throw new RepositoryFunctionException( Starlark.errorf("Archive path '%s' does not exist.", archivePath), Transience.TRANSIENT); } + if (archivePath.isDir()) { + throw Starlark.errorf("attempting to extract a directory: %s", archivePath); + } + maybeWatch(archivePath, ShouldWatch.fromString(watchArchive)); StarlarkPath outputPath = getPath("extract()", output); checkInOutputDirectory("write", outputPath); diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java index 2168b163068a8a..78ee43978d31de 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java @@ -194,12 +194,16 @@ public Parser getParser() { /** * Convert to a {@link com.google.devtools.build.lib.actions.FileValue} to a String appropriate - * for placing in a repository marker file. - * - * @param fileValue The value to convert. It must correspond to a regular file. + * for placing in a repository marker file. The file need not exist, and can be a file or a + * directory. */ public static String fileValueToMarkerValue(FileValue fileValue) throws IOException { - Preconditions.checkArgument(fileValue.isFile() && !fileValue.isSpecialFile()); + if (fileValue.isDirectory()) { + return "DIR"; + } + if (!fileValue.exists()) { + return "ENOENT"; + } // Return the file content digest in hex. fileValue may or may not have the digest available. byte[] digest = fileValue.realFileStateValue().getDigest(); if (digest == null) { 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 4bea03aa2f3ae1..5f3e59621b2533 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 @@ -93,7 +93,7 @@ public final class RepositoryDelegatorFunction implements SkyFunction { // The marker file version is inject in the rule key digest so the rule key is always different // when we decide to update the format. - private static final int MARKER_FILE_VERSION = 5; + private static final int MARKER_FILE_VERSION = 6; // Mapping of rule class name to RepositoryFunction. private final ImmutableMap handlers; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java index b3539d3b6bd4f7..26870de15b3792 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java @@ -324,7 +324,7 @@ public void testPatch() throws Exception { StarlarkPath patchFile = context.path("my.patch"); context.createFile( context.path("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread); - context.patch(patchFile, StarlarkInt.of(0), thread); + context.patch(patchFile, StarlarkInt.of(0), "auto", thread); testOutputFile(foo.getPath(), "line one\nline two\n"); } @@ -335,7 +335,7 @@ public void testCannotFindFileToPatch() throws Exception { context.createFile( context.path("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread); try { - context.patch(patchFile, StarlarkInt.of(0), thread); + context.patch(patchFile, StarlarkInt.of(0), "auto", thread); fail("Expected RepositoryFunctionException"); } catch (RepositoryFunctionException ex) { assertThat(ex) @@ -358,7 +358,7 @@ public void testPatchOutsideOfExternalRepository() throws Exception { true, thread); try { - context.patch(patchFile, StarlarkInt.of(0), thread); + context.patch(patchFile, StarlarkInt.of(0), "auto", thread); fail("Expected RepositoryFunctionException"); } catch (RepositoryFunctionException ex) { assertThat(ex) @@ -379,7 +379,7 @@ public void testPatchErrorWasThrown() throws Exception { context.createFile( context.path("my.patch"), "--- foo\n+++ foo\n" + ONE_LINE_PATCH, false, true, thread); try { - context.patch(patchFile, StarlarkInt.of(0), thread); + context.patch(patchFile, StarlarkInt.of(0), "auto", thread); fail("Expected RepositoryFunctionException"); } catch (RepositoryFunctionException ex) { assertThat(ex) @@ -456,7 +456,7 @@ public void testSymlink() throws Exception { setUpContextForRule("test"); context.createFile(context.path("foo"), "foobar", true, true, thread); - context.symlink(context.path("foo"), context.path("bar"), thread); + context.symlink(context.path("foo"), context.path("bar"), "auto", thread); testOutputFile(outputDirectory.getChild("bar"), "foobar"); assertThat(context.path("bar").realpath()).isEqualTo(context.path("foo")); diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index ea4a6e310eb9bf..5a1e521a0f9758 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -501,7 +501,7 @@ function test_starlark_repository_environ() { def _impl(repository_ctx): print(repository_ctx.os.environ["FOO"]) # Symlink so a repository is created - repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path("")) + repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path(""), watch_target="no") repo = repository_rule(implementation=_impl, local=False) EOF @@ -544,7 +544,7 @@ EOF def _impl(repository_ctx): print(repository_ctx.os.environ["BAR"]) # Symlink so a repository is created - repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path("")) + repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path(""), watch_target="no") repo = repository_rule(implementation=_impl, local=True) EOF BAR=BEZ bazel build @foo//:bar >& $TEST_log || fail "Failed to build" @@ -556,7 +556,7 @@ EOF def _impl(repository_ctx): print(repository_ctx.os.environ["BAZ"]) # Symlink so a repository is created - repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path("")) + repository_ctx.symlink(repository_ctx.path("$repo2"), repository_ctx.path(""), watch_target="no") repo = repository_rule(implementation=_impl, local=True) EOF BAZ=BOZ bazel build @foo//:bar >& $TEST_log || fail "Failed to build" @@ -2788,7 +2788,7 @@ EOF function test_file_watching_outside_workspace() { # when reading a file outside the Bazel workspace, we should watch it. - local outside_dir="${TEST_TMPDIR}/outside_dir" + local outside_dir=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX") mkdir -p "${outside_dir}" echo nothing > ${outside_dir}/data.txt @@ -2887,4 +2887,150 @@ EOF expect_log "Circular definition of repositories" } +function test_watch_file_status_change() { + local outside_dir=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX") + mkdir -p "${outside_dir}" + echo something > ${outside_dir}/data.txt + + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: something" + + # test that all kinds of transitions between file, dir, and noent are watched + + rm ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see nothing" + + mkdir ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see a directory" + + rm -r ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see nothing" + + echo something again > ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: something again" + + rm ${outside_dir}/data.txt + mkdir ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see a directory" + + rm -r ${outside_dir}/data.txt + echo something yet again > ${outside_dir}/data.txt + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: something yet again" +} + +function test_watch_file_status_change_dangling_symlink() { + if "$is_windows"; then + # symlinks on Windows... annoying + return + fi + local outside_dir=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX") + mkdir -p "${outside_dir}" + ln -s ${outside_dir}/pointee ${outside_dir}/pointer + + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see nothing" + + echo haha > ${outside_dir}/pointee + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: haha" + + rm ${outside_dir}/pointee + mkdir ${outside_dir}/pointee + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see a directory" +} + +function test_watch_file_status_change_symlink_parent() { + if "$is_windows"; then + # symlinks on Windows... annoying + return + fi + local outside_dir=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX") + mkdir -p "${outside_dir}/a" + + create_new_workspace + cat > MODULE.bazel < r.bzl <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see nothing" + + mkdir -p ${outside_dir}/a/b + echo blah > ${outside_dir}/a/b/c + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: blah" + + rm -rf ${outside_dir}/a/b + ln -s ${outside_dir}/d ${outside_dir}/a/b + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see nothing" + + mkdir ${outside_dir}/d + echo bleh > ${outside_dir}/d/c + bazel build @r >& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: bleh" +} + run_suite "local repository tests"