Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add watch parameter to {module,repo}_ctx.path #23386

Closed
wants to merge 2 commits into from
Closed

Add watch parameter to {module,repo}_ctx.path #23386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4131ab8
Add `watch` parameter to `module_ctx.path`
fmeum Aug 22, 2024
6b85315
Add comment
fmeum Aug 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 49 additions & 26 deletions .../com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ public Object download(
checksumValidation = e;
}

StarlarkPath outputPath = getPath("download()", output);
StarlarkPath outputPath = path(output);
WorkspaceRuleEvent w =
WorkspaceRuleEvent.newDownloadEvent(
urls,
Expand Down Expand Up @@ -983,7 +983,7 @@ public StructImpl downloadAndExtract(
identifyingStringForLogging,
thread.getCallerLocation());

StarlarkPath outputPath = getPath("download_and_extract()", output);
StarlarkPath outputPath = path(output);
checkInOutputDirectory("write", outputPath);
createDirectory(outputPath.getPath());

Expand Down Expand Up @@ -1130,7 +1130,7 @@ public StructImpl downloadAndExtract(
+ "the <a href=\"#watch\"><code>watch()</code></a> 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 <code>watch()</code> docs for more "
+ "information."),
+ "information)."),
})
public void extract(
Object archive,
Expand All @@ -1140,7 +1140,7 @@ public void extract(
String watchArchive,
StarlarkThread thread)
throws RepositoryFunctionException, InterruptedException, EvalException {
StarlarkPath archivePath = getPath("extract()", archive);
StarlarkPath archivePath = path(archive);

if (!archivePath.exists()) {
throw new RepositoryFunctionException(
Expand All @@ -1151,7 +1151,7 @@ public void extract(
}
maybeWatch(archivePath, ShouldWatch.fromString(watchArchive));

StarlarkPath outputPath = getPath("extract()", output);
StarlarkPath outputPath = path(output);
checkInOutputDirectory("write", outputPath);

Map<String, String> renameFilesMap =
Expand Down Expand Up @@ -1252,7 +1252,7 @@ public boolean isFinished() {
public void createFile(
Object path, String content, Boolean executable, Boolean legacyUtf8, StarlarkThread thread)
throws RepositoryFunctionException, EvalException, InterruptedException {
StarlarkPath p = getPath("file()", path);
StarlarkPath p = path(path);
byte[] contentBytes;
if (legacyUtf8) {
contentBytes = content.getBytes(UTF_8);
Expand Down Expand Up @@ -1349,24 +1349,42 @@ during the analysis phase and thus cannot depends on a target result (the \
},
doc =
"<code>string</code>, <code>Label</code> or <code>path</code> from which to create"
+ " a path from.")
+ " a path from."),
@Param(
name = "watch",
defaultValue = "'auto'",
positional = false,
named = true,
doc =
"""
Whether to <a href="#watch">watch</a> the path. Can be the string 'yes', 'no', \
or 'auto'. Passing 'yes' is equivalent to immediately invoking the \
<a href="#watch"><code>watch()</code></a> method; passing 'no' does not \
attempt to watch the path; passing 'auto' will only attempt to watch the \
path when it is a <code>Label</code> and legal to do so (see <code>watch()</code> \
docs for more information).
""")
})
public StarlarkPath path(Object path) throws EvalException, InterruptedException {
return getPath("path()", path);
public StarlarkPath path(Object path, String watch)
throws EvalException, InterruptedException, RepositoryFunctionException {
var shouldWatch = ShouldWatch.fromString(watch);
var result =
switch (path) {
case String s -> new StarlarkPath(this, workingDirectory.getRelative(s));
case Label label -> getPathFromLabel(label, shouldWatch);
case StarlarkPath starlarkPath -> starlarkPath;
// This can never happen because we check it in the Starlark interpreter.
default -> throw new IllegalArgumentException("expected string or label for path");
};
if (shouldWatch == ShouldWatch.YES) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just maybeWatch(result, shouldWatch) here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment, this would be backwards incompatible (caught by one of the new tests).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry for the delay)

You're talking about testPath_watchAuto_starlarkPathAndAbsolutePathNotWatched, right? I agree that's a change in behavior, but I don't know if it counts as backwards-incompatible. Didn't we do the same behavior change for repo rules?

Plus, we already have the condition where watch = 'auto' means that paths outside the workspace aren't watched (https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java;drc=6052ad65e4185f19944e5138e9b70f6a4eaf9e76;l=1457). Isn't that enough?

Copy link
Collaborator Author

@fmeum fmeum Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title was misleading, it adds this parameter to both repo and module context, so we don't have prior art for this function to refer to.

I agree that the change is minor, but I think that it would end up breaking the code in https://github.com/bazelbuild/rules_python/pull/2135/files#diff-ca74c876e894b0bd25547ca666e562281e5c8fb2abc4a51446f410371224aea2 by reintroducing a dependency into the lockfile. Depending on a sibling label, manipulating the path and then obtaining a StarlarkPath for it via this method is the kind of existing "pattern" that changing this would break.

I'm not in love with that edge case and we could decide to point users to bazel_features instead, but it would still be breaking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I see. Could you add some more details in the PR description about the motivation of this change? It got lost in the commotion.

I could see an API where auto (or whatever the default value for [mr]ctx.path(watch=) ends up being) means "don't watch". Or just make it default to no. Or actually, maybe just make [mr]ctx.watch return the path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I remember now -- so rctx.path(label) used to watch, and rctx.path(path) and rctx.path(string) used to not watch.

This PR retains this old rctx.path(X) watch behavior (which is based on the type of X, not the actual location of X or whether it's rctx or mctx). It just allows the caller to be explicit about watching or not by specifying watch='yes' or watch='no'. The awkward part is that watch='auto' means a different thing for rctx.path() than rctx.read(), rctx.extract(), etc.

I wonder if we'd want rctx.path(X) to never watch (just use rctx.watch() instead). That's obviously backwards-incompatible because some people rely on rctx.path(label) watching, but maybe it's worth introducing an incompatible flag flip for? Undecided.

Also, the fact that rctx.read(label, watch='no') actually still watches is a bit of a subtle hiccup. I'll need to think about that too...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I've thought more about it, I think that I would prefer the following changes:

  • ctx.path never watches and doesn't have a watch parameter. It only ever watched because that was the only way to watch anything at all. It doesn't make much sense that resolving a label to a path is linked to caring about the content at that path. ctx.watch can replace it.
  • ctx.read(label, watch = "no") shouldn't watch.

The latter clearly violates the intent, I would consider changing that behavior a bug fix and don't think it would break anyone.
The former is more impactful to change as it has been the only way to watch for a long time. rules_go would definitely need to change. Luckily ctx.watch can be trivially feature detected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree. Just a bit nervous about not having an incompatible flag for it. (so maybe not so "fully" 😅) Adding an incompatible flag for this one isn't super bad, right? It's just a single if in StarlarkBaseExternalContext so shouldn't be a lot of maintenance burden. And we can make it no-op in Bazel 9.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree the change for path should come with a flag. But since it will mostly require fixing in rulesets, not user projects, we would need to look into that regardless.

maybeWatch(result, ShouldWatch.YES);
}
return result;
}

protected StarlarkPath getPath(String method, Object path)
throws EvalException, InterruptedException {
if (path instanceof String) {
return new StarlarkPath(this, workingDirectory.getRelative(path.toString()));
} else if (path instanceof Label label) {
return getPathFromLabel(label);
} else if (path instanceof StarlarkPath starlarkPath) {
return starlarkPath;
} else {
// This can never happen because we check it in the Starlark interpreter.
throw new IllegalArgumentException("expected string or label for path");
}
protected StarlarkPath path(Object path)
throws EvalException, InterruptedException, RepositoryFunctionException {
return path(path, "auto");
}

@StarlarkMethod(
Expand Down Expand Up @@ -1394,12 +1412,12 @@ protected StarlarkPath getPath(String method, Object path)
<a href="#watch"><code>watch()</code></a> 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 <code>watch()</code> docs for more \
information.
information).
""")
})
public String readFile(Object path, String watch, StarlarkThread thread)
throws RepositoryFunctionException, EvalException, InterruptedException {
StarlarkPath p = getPath("read()", path);
StarlarkPath p = path(path);
WorkspaceRuleEvent w =
WorkspaceRuleEvent.newReadEvent(
p.toString(), identifyingStringForLogging, thread.getCallerLocation());
Expand Down Expand Up @@ -1552,7 +1570,7 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch)
})
public void watchForStarlark(Object path)
throws RepositoryFunctionException, EvalException, InterruptedException {
maybeWatch(getPath("watch()", path), ShouldWatch.YES);
maybeWatch(path(path), ShouldWatch.YES);
}

// Create parent directories for the given path
Expand Down Expand Up @@ -1835,7 +1853,7 @@ public StarlarkExecutionResult execute(

Path workingDirectoryPath;
if (overrideWorkingDirectory != null && !overrideWorkingDirectory.isEmpty()) {
workingDirectoryPath = getPath("execute()", overrideWorkingDirectory).getPath();
workingDirectoryPath = path(overrideWorkingDirectory).getPath();
} else {
workingDirectoryPath = workingDirectory;
}
Expand Down Expand Up @@ -1919,10 +1937,15 @@ private StarlarkPath findCommandOnPath(String program) throws IOException {

// Resolve the label given by value into a file path.
protected StarlarkPath getPathFromLabel(Label label) throws EvalException, InterruptedException {
return getPathFromLabel(label, ShouldWatch.AUTO);
}

protected StarlarkPath getPathFromLabel(Label label, ShouldWatch shouldWatch)
throws EvalException, InterruptedException {
RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env);
StarlarkPath starlarkPath = new StarlarkPath(this, rootedPath.asPath());
try {
maybeWatch(starlarkPath, ShouldWatch.AUTO);
maybeWatch(starlarkPath, shouldWatch);
} catch (RepositoryFunctionException e) {
throw Starlark.errorf("%s", e.getCause().getMessage());
}
Expand Down
20 changes: 10 additions & 10 deletions ...va/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,8 @@ public StructImpl getAttr() {
}

private StarlarkPath externalPath(String method, Object pathObject)
throws EvalException, InterruptedException {
StarlarkPath starlarkPath = getPath(method, pathObject);
throws EvalException, InterruptedException, RepositoryFunctionException {
StarlarkPath starlarkPath = path(pathObject);
Path path = starlarkPath.getPath();
if (packageLocator.getPathEntries().stream().noneMatch(root -> path.startsWith(root.asPath()))
|| path.startsWith(workingDirectory)) {
Expand Down Expand Up @@ -212,8 +212,8 @@ private StarlarkPath externalPath(String method, Object pathObject)
})
public void symlink(Object target, Object linkName, StarlarkThread thread)
throws RepositoryFunctionException, EvalException, InterruptedException {
StarlarkPath targetPath = getPath("symlink()", target);
StarlarkPath linkPath = getPath("symlink()", linkName);
StarlarkPath targetPath = path(target);
StarlarkPath linkPath = path(linkName);
WorkspaceRuleEvent w =
WorkspaceRuleEvent.newSymlinkEvent(
targetPath.toString(),
Expand Down Expand Up @@ -293,7 +293,7 @@ public void symlink(Object target, Object linkName, StarlarkThread thread)
the <a href="#watch"><code>watch()</code></a> 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 <code>watch()</code> docs for more \
information.
information).
"""),
})
public void createFileFromTemplate(
Expand All @@ -304,8 +304,8 @@ public void createFileFromTemplate(
String watchTemplate,
StarlarkThread thread)
throws RepositoryFunctionException, EvalException, InterruptedException {
StarlarkPath p = getPath("template()", path);
StarlarkPath t = getPath("template()", template);
StarlarkPath p = path(path);
StarlarkPath t = path(template);
Map<String, String> substitutionMap =
Dict.cast(substitutions, String.class, String.class, "substitutions");
WorkspaceRuleEvent w =
Expand Down Expand Up @@ -436,13 +436,13 @@ public boolean delete(Object pathObject, StarlarkThread thread)
the <a href="#watch"><code>watch()</code></a> 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 <code>watch()</code> docs for more \
information.
information).
"""),
})
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);
StarlarkPath starlarkPath = path(patchFile);
WorkspaceRuleEvent w =
WorkspaceRuleEvent.newPatchEvent(
starlarkPath.toString(),
Expand Down Expand Up @@ -487,7 +487,7 @@ public void patch(Object patchFile, StarlarkInt stripI, String watchPatch, Starl
})
public void watchTree(Object path)
throws EvalException, InterruptedException, RepositoryFunctionException {
StarlarkPath p = getPath("watch_tree()", path);
StarlarkPath p = path(path);
if (!p.isDir()) {
throw Starlark.errorf("can't call watch_tree() on non-directory %s", p);
}
Expand Down
35 changes: 2 additions & 33 deletions src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ java_library(
srcs = glob(["*.java"]),
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
Expand All @@ -31,6 +32,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
Expand All @@ -54,7 +56,6 @@ java_library(
java_test(
name = "StarlarkTests",
testonly = 1,
srcs = glob(["*.java"]),
Wyverald marked this conversation as resolved.
Show resolved Hide resolved
tags = [
"rules",
],
Expand All @@ -63,36 +64,4 @@ java_test(
":StarlarkTests_lib",
"//src/test/java/com/google/devtools/build/lib:test_runner",
],
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
"//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
"//src/main/java/com/google/devtools/build/lib/bazel/repository/starlark",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//src/test/java/com/google/devtools/build/lib/analysis/util",
"//src/test/java/com/google/devtools/build/lib/starlark/util",
"//src/test/java/com/google/devtools/build/lib/testutil",
"//third_party:guava",
"//third_party:jsr305",
"//third_party:junit4",
"//third_party:mockito",
"//third_party:truth",
],
)
Loading