Skip to content

Commit

Permalink
Fix watching paths in undefined repos in repo rules
Browse files Browse the repository at this point in the history 
Watching a path in an undefined repo (eg. `$output_base/external/i_am_undefined/blah`) currently causes an NPE because we expect it to need a skyframe restart when it actually doesn't. The underlying reason is rather convoluted, but the gist is that we request `RepositoryDirectoryValue.Key(@@i_am_undefined)` explicitly, when we don't actually have to -- we can simply use `FileValue.Key($output_base/external/i_am_undefined/blah` directly, and rely on the fact that `FileFunction` knows to request the corresponding `RepositoryDirectoryValue` and behave appropriately when the repo is undefined. (This is due to logic in `ExternalFilesHelper.maybeHandleExternalFile`.)

Additionally, this PR removes the `rctx.symlink(watch_target=)` parameter. The symlink target's existence and its contents should not affect the creation of the symlink at all, so there's no reason to ever watch it.

Fixes #21483.

Closes #21562.

PiperOrigin-RevId: 612898526
Change-Id: I30deb9d9b2d19e0869517f4b6b126078446745b4
  • Loading branch information
@Wyverald @copybara-github
Wyverald authored and copybara-github committed Mar 5, 2024
1 parent 8bb0bb3 commit aba1186
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 102 deletions.
21 changes: 6 additions & 15 deletions .../com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult;
import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
import com.google.devtools.build.lib.util.OsUtils;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand Down Expand Up @@ -1280,20 +1279,16 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch)
if (repoCacheFriendlyPath == null) {
return;
}
RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories);
if (rootedPath == null) {
throw new NeedsSkyframeRestartException();
}
try {
var recordedInput = new RepoRecordedInput.File(repoCacheFriendlyPath);
FileValue fileValue =
(FileValue) env.getValueOrThrow(FileValue.key(rootedPath), IOException.class);
(FileValue) env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class);
if (fileValue == null) {
throw new NeedsSkyframeRestartException();
}

recordedFileInputs.put(
new RepoRecordedInput.File(repoCacheFriendlyPath),
RepoRecordedInput.File.fileValueToMarkerValue(fileValue));
recordedInput, RepoRecordedInput.File.fileValueToMarkerValue(fileValue));
} catch (IOException e) {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
Expand All @@ -1305,17 +1300,13 @@ protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch)
if (repoCacheFriendlyPath == null) {
return;
}
RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories);
if (env.valuesMissing()) {
throw new NeedsSkyframeRestartException();
}
if (env.getValue(DirectoryListingValue.key(rootedPath)) == null) {
var recordedInput = new RepoRecordedInput.Dirents(repoCacheFriendlyPath);
if (env.getValue(recordedInput.getSkyKey(directories)) == null) {
throw new NeedsSkyframeRestartException();
}
try {
recordedDirentsInputs.put(
new RepoRecordedInput.Dirents(repoCacheFriendlyPath),
RepoRecordedInput.Dirents.getDirentsMarkerValue(path));
recordedInput, RepoRecordedInput.Dirents.getDirentsMarkerValue(path));
} catch (IOException e) {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
Expand Down
26 changes: 4 additions & 22 deletions ...va/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
Expand Down Expand Up @@ -209,20 +208,8 @@ private StarlarkPath externalPath(String method, Object pathObject)
@ParamType(type = StarlarkPath.class)
},
doc = "The path of the symlink to create."),
@Param(
name = "watch_target",
defaultValue = "'auto'",
positional = false,
named = true,
doc =
"whether to <a href=\"#watch\">watch</a> the symlink target. 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 file when it is legal to do so (see <code>watch()</code> docs for more "
+ "information."),
})
public void symlink(Object target, Object linkName, String watchTarget, StarlarkThread thread)
public void symlink(Object target, Object linkName, StarlarkThread thread)
throws RepositoryFunctionException, EvalException, InterruptedException {
StarlarkPath targetPath = getPath("symlink()", target);
StarlarkPath linkPath = getPath("symlink()", linkName);
Expand All @@ -233,7 +220,6 @@ public void symlink(Object target, Object linkName, String watchTarget, Starlark
getIdentifyingStringForLogging(),
thread.getCallerLocation());
env.getListener().post(w);
maybeWatch(targetPath, ShouldWatch.fromString(watchTarget));
try {
checkInOutputDirectory("write", linkPath);
makeDirectories(linkPath.getPath());
Expand Down Expand Up @@ -605,20 +591,16 @@ public void watchTree(Object path)
if (repoCacheFriendlyPath == null) {
return;
}
RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories);
if (rootedPath == null) {
throw new NeedsSkyframeRestartException();
}
try {
var recordedInput = new RepoRecordedInput.DirTree(repoCacheFriendlyPath);
DirectoryTreeDigestValue digestValue =
(DirectoryTreeDigestValue)
env.getValueOrThrow(DirectoryTreeDigestValue.key(rootedPath), IOException.class);
env.getValueOrThrow(recordedInput.getSkyKey(directories), IOException.class);
if (digestValue == null) {
throw new NeedsSkyframeRestartException();
}

recordedDirTreeInputs.put(
new RepoRecordedInput.DirTree(repoCacheFriendlyPath), digestValue.hexDigest());
recordedDirTreeInputs.put(recordedInput, digestValue.hexDigest());
} catch (IOException e) {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
Expand Down
99 changes: 38 additions & 61 deletions src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.LabelValidator;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
Expand Down Expand Up @@ -115,8 +116,7 @@ public static boolean areAllValuesUpToDate(
throws InterruptedException {
env.getValuesAndExceptions(
recordedInputValues.keySet().stream()
.map(RepoRecordedInput::getSkyKey)
.filter(Objects::nonNull)
.map(rri -> rri.getSkyKey(directories))
.collect(toImmutableSet()));
if (env.valuesMissing()) {
return false;
Expand Down Expand Up @@ -157,18 +157,14 @@ public int compareTo(RepoRecordedInput o) {
/** Returns the parser object for this type of recorded inputs. */
public abstract Parser getParser();

/**
* Returns the {@link SkyKey} that is necessary to determine {@link #isUpToDate}. Can be null if
* no SkyKey is needed.
*/
@Nullable
public abstract SkyKey getSkyKey();
/** Returns the {@link SkyKey} that is necessary to determine {@link #isUpToDate}. */
public abstract SkyKey getSkyKey(BlazeDirectories directories);

/**
* Returns whether the given {@code oldValue} is still up-to-date for this recorded input. This
* method can assume that {@link #getSkyKey()} is already evaluated; it can request further
* Skyframe evaluations, and if any values are missing, this method can return any value (doesn't
* matter what) and will be reinvoked after a Skyframe restart.
* method can assume that {@link #getSkyKey(BlazeDirectories)} is already evaluated; it can
* request further Skyframe evaluations, and if any values are missing, this method can return any
* value (doesn't matter what) and will be reinvoked after a Skyframe restart.
*/
public abstract boolean isUpToDate(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
Expand Down Expand Up @@ -226,32 +222,25 @@ public static RepoCacheFriendlyPath parse(String s) {
return createOutsideWorkspace(PathFragment.create(s));
}

/**
* If resolving this path requires getting a {@link RepositoryDirectoryValue}, this method
* returns the appropriate key; otherwise it returns null.
*/
@Nullable
public final RepositoryDirectoryValue.Key getRepoDirSkyKeyOrNull() {
if (repoName().isEmpty() || repoName().get().isMain()) {
return null;
}
return RepositoryDirectoryValue.key(repoName().get());
}

public final RootedPath getRootedPath(Environment env, BlazeDirectories directories)
throws InterruptedException {
/** Returns the rooted path corresponding to this "repo-friendly path". */
public final RootedPath getRootedPath(BlazeDirectories directories) {
Root root;
if (repoName().isEmpty()) {
root = Root.absoluteRoot(directories.getOutputBase().getFileSystem());
} else if (repoName().get().isMain()) {
root = Root.fromPath(directories.getWorkspace());
} else {
RepositoryDirectoryValue repoDirValue =
(RepositoryDirectoryValue) env.getValue(getRepoDirSkyKeyOrNull());
if (repoDirValue == null || !repoDirValue.repositoryExists()) {
return null;
}
root = Root.fromPath(repoDirValue.getPath());
// This path is from an external repo. We just directly fabricate the path here instead of
// requesting the appropriate RepositoryDirectoryValue, since we can rely on the various
// other SkyFunctions (such as FileStateFunction and DirectoryListingStateFunction) to do
// that for us instead. This also sidesteps an awkward situation when the external repo in
// question is not defined.
root =
Root.fromPath(
directories
.getOutputBase()
.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION)
.getRelative(repoName().get().getName()));
}
return RootedPath.toRootedPath(root, path());
}
Expand Down Expand Up @@ -331,23 +320,18 @@ public static String fileValueToMarkerValue(FileValue fileValue) throws IOExcept
return BaseEncoding.base16().lowerCase().encode(digest);
}

@Override
@Nullable
public SkyKey getSkyKey() {
return path.getRepoDirSkyKeyOrNull();
public SkyKey getSkyKey(BlazeDirectories directories) {
return FileValue.key(path.getRootedPath(directories));
}

@Override
public boolean isUpToDate(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
RootedPath rootedPath = path.getRootedPath(env, directories);
if (rootedPath == null) {
return false;
}
SkyKey fileKey = FileValue.key(rootedPath);
try {
FileValue fileValue = (FileValue) env.getValueOrThrow(fileKey, IOException.class);
FileValue fileValue =
(FileValue) env.getValueOrThrow(getSkyKey(directories), IOException.class);
if (fileValue == null) {
return false;
}
Expand Down Expand Up @@ -406,25 +390,22 @@ public Parser getParser() {
return PARSER;
}

@Nullable
@Override
public SkyKey getSkyKey() {
return path.getRepoDirSkyKeyOrNull();
public SkyKey getSkyKey(BlazeDirectories directories) {
return DirectoryListingValue.key(path.getRootedPath(directories));
}

@Override
public boolean isUpToDate(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
RootedPath rootedPath = path.getRootedPath(env, directories);
if (rootedPath == null) {
return false;
}
if (env.getValue(DirectoryListingValue.key(rootedPath)) == null) {
SkyKey skyKey = getSkyKey(directories);
if (env.getValue(skyKey) == null) {
return false;
}
try {
return oldValue.equals(getDirentsMarkerValue(rootedPath.asPath()));
return oldValue.equals(
getDirentsMarkerValue(((DirectoryListingValue.Key) skyKey).argument().asPath()));
} catch (IOException e) {
return false;
}
Expand Down Expand Up @@ -493,22 +474,17 @@ public Parser getParser() {
return PARSER;
}

@Nullable
@Override
public SkyKey getSkyKey() {
return path.getRepoDirSkyKeyOrNull();
public SkyKey getSkyKey(BlazeDirectories directories) {
return DirectoryTreeDigestValue.key(path.getRootedPath(directories));
}

@Override
public boolean isUpToDate(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
RootedPath rootedPath = path.getRootedPath(env, directories);
if (rootedPath == null) {
return false;
}
DirectoryTreeDigestValue value =
(DirectoryTreeDigestValue) env.getValue(DirectoryTreeDigestValue.key(rootedPath));
(DirectoryTreeDigestValue) env.getValue(getSkyKey(directories));
if (value == null) {
return false;
}
Expand Down Expand Up @@ -565,7 +541,7 @@ public String toStringInternal() {
}

@Override
public SkyKey getSkyKey() {
public SkyKey getSkyKey(BlazeDirectories directories) {
return ActionEnvironmentFunction.key(name);
}

Expand All @@ -575,7 +551,7 @@ public boolean isUpToDate(
throws InterruptedException {
String v = PrecomputedValue.REPO_ENV.get(env).get(name);
if (v == null) {
v = ((ClientEnvironmentValue) env.getValue(getSkyKey())).getValue();
v = ((ClientEnvironmentValue) env.getValue(getSkyKey(directories))).getValue();
}
// Note that `oldValue` can be null if the env var was not set.
return Objects.equals(oldValue, v);
Expand Down Expand Up @@ -636,7 +612,7 @@ public String toStringInternal() {
}

@Override
public SkyKey getSkyKey() {
public SkyKey getSkyKey(BlazeDirectories directories) {
// Since we only record repo mapping entries for repos defined in Bzlmod, we can request the
// WORKSPACE-less version of the main repo mapping (as no repos defined in Bzlmod can see
// stuff from WORKSPACE).
Expand All @@ -649,7 +625,8 @@ public SkyKey getSkyKey() {
public boolean isUpToDate(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
RepositoryMappingValue repoMappingValue = (RepositoryMappingValue) env.getValue(getSkyKey());
RepositoryMappingValue repoMappingValue =
(RepositoryMappingValue) env.getValue(getSkyKey(directories));
return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE
&& RepositoryName.createUnvalidated(oldValue)
.equals(repoMappingValue.getRepositoryMapping().get(apparentName));
Expand Down
2 changes: 1 addition & 1 deletion ...om/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,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"), "auto", thread);
context.symlink(context.path("foo"), context.path("bar"), thread);
testOutputFile(outputDirectory.getChild("bar"), "foobar");

assertThat(context.path("bar").realpath()).isEqualTo(context.path("foo"));
Expand Down
Loading

0 comments on commit aba1186

Please sign in to comment.