Skip to content

Commit

Permalink
Fix up dependency between RepositoryDirectoryValue and FileStateValue
Browse files Browse the repository at this point in the history 
In `RepositoryFunction#addExternalFilesDependencies`, we introduce a dependency from `FileStateValue($outputBase/external/foo/$SOME_PATH)` to `RepositoryDirectoryValue(@foo)`, _except_ when `$SOME_PATH` is empty (i.e. the "foo" directory itself), or when `@foo` is a local_repository and $SOME_PATH is "WORKSPACE".

These two exceptions are a bit strange (why would we expect anyone to mess with the external directory? and why should that result in a re-fetch?) and exist only because there is a dependency in the reverse direction (that is, `RepositoryDirectoryValue(@foo)` depends on `FileStateValue($outputBase/external/foo)`, and additionally on `FileStateValue($outputBase/external/foo/WORKSPACE)` if `@foo` is a local_repository).

This CL removes these exceptions. This means that the dependency is always from FileStateValue to RepositoryDirectoryValue. Meaning that if the repo definition changes in any way, we'll refetch the files in the external directory; but if the files in the external directory somehow change from under us, we don't trigger a refetch.

This change will also make our lives easier in the future with the external deps overhaul.

PiperOrigin-RevId: 374830398
  • Loading branch information
@Wyverald @copybara-github
Wyverald authored and copybara-github committed May 20, 2021
1 parent 4fc24bc commit 9732d23
Show file tree
Hide file tree
Showing 12 changed files with 98 additions and 190 deletions.
39 changes: 3 additions & 36 deletions src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,15 @@
package com.google.devtools.build.lib.rules.repository;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.events.ExtendedEventHandler.ResolvedEvent;
import com.google.devtools.build.lib.packages.Rule;
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.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import java.io.IOException;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.Starlark;

/**
Expand All @@ -51,10 +46,10 @@ public RepositoryDirectoryValue.Builder fetch(
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
String userDefinedPath = RepositoryFunction.getPathAttr(rule);
PathFragment pathFragment =
RepositoryFunction.getTargetPath(userDefinedPath, directories.getWorkspace());
Path targetPath = directories.getWorkspace().getRelative(userDefinedPath);
RepositoryDirectoryValue.Builder result =
RepositoryDelegatorFunction.symlink(outputDirectory, pathFragment, userDefinedPath, env);
RepositoryDelegatorFunction.symlinkRepoRoot(
outputDirectory, targetPath, userDefinedPath, env);
if (result != null) {
env.getListener().post(resolve(rule, directories));
}
Expand All @@ -66,34 +61,6 @@ public Class<? extends RuleDefinition> getRuleDefinition() {
return LocalRepositoryRule.class;
}

@Nullable
protected static FileValue getWorkspaceFile(RootedPath directory, Environment env)
throws RepositoryFunctionException, InterruptedException {
RootedPath workspaceRootedFile;
try {
workspaceRootedFile = WorkspaceFileHelper.getWorkspaceRootedFile(directory, env);
if (workspaceRootedFile == null) {
return null;
}
} catch (IOException e) {
throw new RepositoryFunctionException(
new IOException(
"Could not determine workspace file (\"WORKSPACE.bazel\" or \"WORKSPACE\"): "
+ e.getMessage()),
Transience.PERSISTENT);
}
SkyKey workspaceFileKey = FileValue.key(workspaceRootedFile);
FileValue value;
try {
value = (FileValue) env.getValueOrThrow(workspaceFileKey, IOException.class);
} catch (IOException e) {
throw new RepositoryFunctionException(
new IOException("Could not access " + workspaceRootedFile + ": " + e.getMessage()),
Transience.PERSISTENT);
}
return value;
}

private static ResolvedEvent resolve(Rule rule, BlazeDirectories directories) {
String name = rule.getName();
Object pathObj = rule.getAttr("path");
Expand Down
83 changes: 57 additions & 26 deletions ...main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@
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.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
Expand Down Expand Up @@ -128,8 +130,8 @@ public RepositoryDelegatorFunction(
this.externalPackageHelper = externalPackageHelper;
}

public static RepositoryDirectoryValue.Builder symlink(
Path source, PathFragment destination, String userDefinedPath, Environment env)
public static RepositoryDirectoryValue.Builder symlinkRepoRoot(
Path source, Path destination, String userDefinedPath, Environment env)
throws RepositoryFunctionException, InterruptedException {
try {
source.createSymbolicLink(destination);
Expand All @@ -142,28 +144,41 @@ public static RepositoryDirectoryValue.Builder symlink(
e),
Transience.TRANSIENT);
}
FileValue repositoryValue = RepositoryFunction.getRepositoryDirectory(source, env);
if (repositoryValue == null) {

// Check that the target directory exists and is a directory.
// Note that we have to check `destination` and not `source` here, otherwise we'd have a
// circular dependency between SkyValues.
RootedPath targetDirRootedPath =
RootedPath.toRootedPath(Root.absoluteRoot(destination.getFileSystem()), destination);
FileValue targetDirValue;
try {
targetDirValue =
(FileValue) env.getValueOrThrow(FileValue.key(targetDirRootedPath), IOException.class);
} catch (IOException e) {
throw new RepositoryFunctionException(
new IOException("Could not access " + destination + ": " + e.getMessage()),
Transience.PERSISTENT);
}
if (targetDirValue == null) {
// TODO(bazel-team): If this returns null, we unnecessarily recreate the symlink above on the
// second execution.
return null;
}

if (!repositoryValue.isDirectory()) {
if (!targetDirValue.isDirectory()) {
throw new RepositoryFunctionException(
new IOException(
String.format(
"The repository's path is \"%s\" (absolute: \"%s\") "
+ "but this directory does not exist.",
+ "but it does not exist or is not a directory.",
userDefinedPath, destination)),
Transience.PERSISTENT);
}

// Check that the repository contains a WORKSPACE file.
// It's important to check the real path, otherwise this looks under the "external/[repo]" path
// and cause a Skyframe cycle in the lookup.
FileValue workspaceFileValue =
LocalRepositoryFunction.getWorkspaceFile(repositoryValue.realRootedPath(), env);
// Note that we need to do this here since we're not creating a WORKSPACE file ourselves, but
// entrusting the entire contents of the repo root to this target directory.
FileValue workspaceFileValue = getWorkspaceFile(targetDirRootedPath, env);
if (workspaceFileValue == null) {
return null;
}
Expand All @@ -172,10 +187,37 @@ public static RepositoryDirectoryValue.Builder symlink(
throw new RepositoryFunctionException(
new IOException("No WORKSPACE file found in " + source), Transience.PERSISTENT);
}

return RepositoryDirectoryValue.builder().setPath(source);
}

@Nullable
private static FileValue getWorkspaceFile(RootedPath directory, Environment env)
throws RepositoryFunctionException, InterruptedException {
RootedPath workspaceRootedFile;
try {
workspaceRootedFile = WorkspaceFileHelper.getWorkspaceRootedFile(directory, env);
if (workspaceRootedFile == null) {
return null;
}
} catch (IOException e) {
throw new RepositoryFunctionException(
new IOException(
"Could not determine workspace file (\"WORKSPACE.bazel\" or \"WORKSPACE\"): "
+ e.getMessage()),
Transience.PERSISTENT);
}
SkyKey workspaceFileKey = FileValue.key(workspaceRootedFile);
FileValue value;
try {
value = (FileValue) env.getValueOrThrow(workspaceFileKey, IOException.class);
} catch (IOException e) {
throw new RepositoryFunctionException(
new IOException("Could not access " + workspaceRootedFile + ": " + e.getMessage()),
Transience.PERSISTENT);
}
return value;
}

private void setupRepositoryRoot(Path repoRoot) throws RepositoryFunctionException {
try {
repoRoot.deleteTree();
Expand Down Expand Up @@ -255,12 +297,6 @@ && managedDirectoriesExist(directories.getWorkspace(), managedDirectories)) {
return null;
}
if (markerHash != null) {
// Now that we know that it exists and that we should not fetch unconditionally, we can
// declare a Skyframe dependency on the repository root.
RepositoryFunction.getRepositoryDirectory(repoRoot, env);
if (env.valuesMissing()) {
return null;
}
return RepositoryDirectoryValue.builder()
.setPath(repoRoot)
.setDigest(markerHash)
Expand Down Expand Up @@ -294,13 +330,6 @@ && managedDirectoriesExist(directories.getWorkspace(), managedDirectories)) {
Transience.TRANSIENT);
}

// Declare a Skyframe dependency so that this is re-evaluated when something happens to the
// directory.
RepositoryFunction.getRepositoryDirectory(repoRoot, env);
if (env.valuesMissing()) {
return null;
}

// Try to build with whatever is on the file system and emit a warning.
env.getListener()
.handle(Event.warn(rule.getLocation(),
Expand Down Expand Up @@ -400,11 +429,13 @@ private RepositoryDirectoryValue setupOverride(
PathFragment sourcePath, Environment env, Path repoRoot, String pathAttr)
throws RepositoryFunctionException, InterruptedException {
setupRepositoryRoot(repoRoot);
RepositoryDirectoryValue.Builder directoryValue = symlink(repoRoot, sourcePath, pathAttr, env);
RepositoryDirectoryValue.Builder directoryValue =
symlinkRepoRoot(
repoRoot, directories.getWorkspace().getRelative(sourcePath), pathAttr, env);
if (directoryValue == null) {
return null;
}
byte[] digest = new byte[] {};
byte[] digest = new Fingerprint().addPath(sourcePath).digestAndReset();
return directoryValue.setDigest(digest).build();
}

Expand Down
79 changes: 3 additions & 76 deletions src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.repository.ExternalPackageException;
import com.google.devtools.build.lib.repository.ExternalPackageHelper;
import com.google.devtools.build.lib.repository.ExternalRuleNotFoundException;
import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
import com.google.devtools.build.lib.skyframe.AlreadyReportedException;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
Expand Down Expand Up @@ -524,28 +522,6 @@ static void createSymbolicLink(Path from, Path to)
}
}

/**
* Adds the repository's directory to the graph and, if it's a symlink, resolves it to an actual
* directory.
*/
@Nullable
protected static FileValue getRepositoryDirectory(Path repositoryDirectory, Environment env)
throws RepositoryFunctionException, InterruptedException {
SkyKey outputDirectoryKey =
FileValue.key(
RootedPath.toRootedPath(
Root.fromPath(repositoryDirectory), PathFragment.EMPTY_FRAGMENT));
FileValue value;
try {
value = (FileValue) env.getValueOrThrow(outputDirectoryKey, IOException.class);
} catch (IOException e) {
throw new RepositoryFunctionException(
new IOException("Could not access " + repositoryDirectory + ": " + e.getMessage()),
Transience.PERSISTENT);
}
return value;
}

protected static Path getExternalRepositoryDirectory(BlazeDirectories directories) {
return directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION);
}
Expand All @@ -562,11 +538,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,
boolean isDirectory,
BlazeDirectories directories,
Environment env,
ExternalPackageHelper externalPackageHelper)
RootedPath rootedPath, BlazeDirectories directories, Environment env)
throws InterruptedException {
Path externalRepoDir = getExternalRepositoryDirectory(directories);
PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir);
Expand All @@ -576,53 +548,8 @@ public static void addExternalFilesDependencies(
return;
}
String repositoryName = repositoryPath.getSegment(0);

try {
// Add a dependency to the repository rule. RepositoryDirectoryValue does add this
// dependency already but we want to catch RepositoryNotFoundException, so invoke
// #getRuleByName
// first.
Rule rule = externalPackageHelper.getRuleByName(repositoryName, env);
if (rule == null) {
// Still an override might change the content of the repository.
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.get(env);
return;
}

if (isDirectory || repositoryPath.isMultiSegment()) {
if (!isDirectory
&& rule.getRuleClass().equals(LocalRepositoryRule.NAME)
&& WorkspaceFileHelper.endsWithWorkspaceFileName(repositoryPath)) {
// Ignore this, there is a dependency from LocalRepositoryFunction->WORKSPACE file already
return;
}

// For all files under the repository directory, depend on the actual RepositoryDirectory
// function so we get invalidation when the repository is fetched.
// For the repository directory itself, we cannot depends on the RepositoryDirectoryValue
// (cycle).
env.getValue(
RepositoryDirectoryValue.key(
RepositoryName.createFromValidStrippedName(repositoryName)));
} else {
// Invalidate external/<repo> if the repository overrides change.
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.get(env);
}
} catch (ExternalRuleNotFoundException ex) {
// The repository we are looking for does not exist so we should depend on the whole
// WORKSPACE file. In that case, the call to RepositoryFunction#getRuleByName(String,
// Environment)
// already requested all repository functions from the WORKSPACE file from Skyframe as part
// of the resolution.
//
// Alternatively, the repository might still be provided by an override. Therefore, in
// any case, register the dependency on the repository overrides.
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.get(env);
} catch (ExternalPackageException ex) {
// This should never happen.
throw new IllegalStateException(
"Repository " + repositoryName + " cannot be resolved for path " + rootedPath, ex);
}
env.getValue(
RepositoryDirectoryValue.key(RepositoryName.createFromValidStrippedName(repositoryName)));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public DirectoryListingStateValue compute(SkyKey skyKey, Environment env)
RootedPath dirRootedPath = (RootedPath) skyKey.argument();

try {
FileType fileType = externalFilesHelper.maybeHandleExternalFile(dirRootedPath, true, env);
FileType fileType = externalFilesHelper.maybeHandleExternalFile(dirRootedPath, env);
if (env.valuesMissing()) {
return null;
}
Expand Down
Loading

0 comments on commit 9732d23

Please sign in to comment.