Skip to content

Commit

Permalink
Explain why vendored repositories are considered out-of-date
Browse files Browse the repository at this point in the history
  • Loading branch information
@fmeum
fmeum committed Jan 8, 2025
1 parent 6061e5e commit 30ebaf3
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 71 deletions.
5 changes: 3 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,11 +425,12 @@ private static boolean didRecordedInputsChange(
BlazeDirectories directories,
Map<? extends RepoRecordedInput, String> recordedInputs)
throws InterruptedException, NeedsSkyframeRestartException {
boolean upToDate = RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedInputs);
Optional<String> outdated =
RepoRecordedInput.isAnyValueOutdated(env, directories, recordedInputs);
if (env.valuesMissing()) {
throw new NeedsSkyframeRestartException();
}
return !upToDate;
return outdated.isPresent();
}

/**
Expand Down
103 changes: 68 additions & 35 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 @@ -120,7 +120,7 @@ public static RepoRecordedInput parse(String s) {
* missing, the return value should be ignored; callers are responsible for checking {@code
* env.valuesMissing()} and triggering a Skyframe restart if needed.
*/
public static boolean areAllValuesUpToDate(
public static Optional<String> isAnyValueOutdated(
Environment env,
BlazeDirectories directories,
Map<? extends RepoRecordedInput, String> recordedInputValues)
Expand All @@ -130,17 +130,17 @@ public static boolean areAllValuesUpToDate(
.map(rri -> rri.getSkyKey(directories))
.collect(toImmutableSet()));
if (env.valuesMissing()) {
return false;
return UNDECIDED;
}
for (Map.Entry<? extends RepoRecordedInput, String> recordedInputValue :
recordedInputValues.entrySet()) {
if (!recordedInputValue
.getKey()
.isUpToDate(env, directories, recordedInputValue.getValue())) {
return false;
Optional<String> reason =
recordedInputValue.getKey().isOutdated(env, directories, recordedInputValue.getValue());
if (reason.isPresent()) {
return reason;
}
}
return true;
return Optional.empty();
}

@Override
Expand Down Expand Up @@ -168,19 +168,23 @@ 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}. */
/** Returns the {@link SkyKey} that is necessary to determine {@link #isOutdated}. */
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(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.
* Returns a human-readable reason for why the given {@code oldValue} is no longer up-to-date for
* this recorded input, or an empty Optional if it is still up-to-date. This 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, although {@link #UNDECIDED} is recommended for clarity) and will be reinvoked after a
* Skyframe restart.
*/
public abstract boolean isUpToDate(
public abstract Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException;

private static final Optional<String> UNDECIDED = Optional.of("values missing");

/** A sentinel "input" that's always out-of-date to signify parse failure. */
public static final RepoRecordedInput NEVER_UP_TO_DATE =
new RepoRecordedInput() {
Expand Down Expand Up @@ -211,9 +215,9 @@ public SkyKey getSkyKey(BlazeDirectories directories) {
}

@Override
public boolean isUpToDate(
public Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue) {
return false;
return Optional.of("malformed marker entries encountered");
}
};

Expand Down Expand Up @@ -373,24 +377,26 @@ public static String fileValueToMarkerValue(RootedPath rootedPath, FileValue fil
}

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

@Override
public boolean isUpToDate(
public Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
var skyKey = getSkyKey(directories);
try {
FileValue fileValue = (FileValue) env.getValueOrThrow(skyKey, IOException.class);
if (fileValue == null) {
return false;
return UNDECIDED;
}
return oldValue.equals(fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue));
if (!oldValue.equals(fileValueToMarkerValue((RootedPath) skyKey.argument(), fileValue))) {
return Optional.of("file info or contents of %s changed".formatted(path));
}
return Optional.empty();
} catch (IOException e) {
return false;
return Optional.of("failed to stat %s: %s".formatted(path, e.getMessage()));
}
}
}
Expand Down Expand Up @@ -453,18 +459,21 @@ public SkyKey getSkyKey(BlazeDirectories directories) {
}

@Override
public boolean isUpToDate(
public Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
SkyKey skyKey = getSkyKey(directories);
if (env.getValue(skyKey) == null) {
return false;
return UNDECIDED;
}
try {
return oldValue.equals(
getDirentsMarkerValue(((DirectoryListingKey) skyKey).argument().asPath()));
if (!oldValue.equals(
getDirentsMarkerValue(((DirectoryListingKey) skyKey).argument().asPath()))) {
return Optional.of("directory entries of %s changed".formatted(path));
}
return Optional.empty();
} catch (IOException e) {
return false;
return Optional.of("failed to readdir %s: %s".formatted(path, e.getMessage()));
}
}

Expand Down Expand Up @@ -541,15 +550,18 @@ public SkyKey getSkyKey(BlazeDirectories directories) {
}

@Override
public boolean isUpToDate(
public Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
DirectoryTreeDigestValue value =
(DirectoryTreeDigestValue) env.getValue(getSkyKey(directories));
if (value == null) {
return false;
return UNDECIDED;
}
if (!oldValue.equals(value.hexDigest())) {
return Optional.of("directory tree at %s changed".formatted(path));
}
return oldValue.equals(value.hexDigest());
return Optional.empty();
}
}

Expand Down Expand Up @@ -612,15 +624,23 @@ public SkyKey getSkyKey(BlazeDirectories directories) {
}

@Override
public boolean isUpToDate(
public Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
String v = PrecomputedValue.REPO_ENV.get(env).get(name);
if (v == null) {
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);
if (!Objects.equals(oldValue, v)) {
return Optional.of(
"value of %s changed: %s -> %s"
.formatted(
name,
oldValue == null ? "" : "'%s'".formatted(oldValue),
v == null ? "" : "'%s'".formatted(v)));
}
return Optional.empty();
}
}

Expand Down Expand Up @@ -691,19 +711,32 @@ public SkyKey getSkyKey(BlazeDirectories directories) {
}

@Override
public boolean isUpToDate(
public Optional<String> isOutdated(
Environment env, BlazeDirectories directories, @Nullable String oldValue)
throws InterruptedException {
RepositoryMappingValue repoMappingValue =
(RepositoryMappingValue) env.getValue(getSkyKey(directories));
if (repoMappingValue == RepositoryMappingValue.NOT_FOUND_VALUE) {
return Optional.of("source repo %s doesn't exist anymore".formatted(sourceRepo));
}
RepositoryName oldCanonicalName;
try {
return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE
&& RepositoryName.create(oldValue)
.equals(repoMappingValue.repositoryMapping().get(apparentName));
oldCanonicalName = RepositoryName.create(oldValue);
} catch (LabelSyntaxException e) {
// malformed old value causes refetch
return false;
return Optional.of("invalid recorded repo name: %s".formatted(e.getMessage()));
}
RepositoryName newCanonicalName = repoMappingValue.repositoryMapping().get(apparentName);
if (!oldCanonicalName.equals(newCanonicalName)) {
return Optional.of(
"canonical name for @%s in %s changed: %s -> %s"
.formatted(
apparentName,
sourceRepo,
oldCanonicalName,
newCanonicalName == null ? "<doesn't exist>" : newCanonicalName));
}
return Optional.empty();
}
}
}
59 changes: 33 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 @@ -39,6 +39,8 @@
import com.google.devtools.build.lib.repository.ExternalRuleNotFoundException;
import com.google.devtools.build.lib.repository.RepositoryFailedEvent;
import com.google.devtools.build.lib.repository.RepositoryFetchProgress;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.DigestWriter.RepoDirectoryState.OutOfDate;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.DigestWriter.RepoDirectoryState.UpToDate;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue.NoRepositoryDirectoryValue;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction.AlreadyReportedRepositoryAccessException;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
Expand Down Expand Up @@ -204,14 +206,14 @@ public SkyValue compute(SkyKey skyKey, Environment env)

if (shouldUseCachedRepos(env, handler, repoRoot, rule)) {
// Make sure marker file is up-to-date; correctly describes the current repository state
byte[] markerHash = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env);
if (env.valuesMissing()) {
var repoState = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env);
if (repoState == null) {
return null;
}
if (markerHash != null) { // repo exist & up-to-date
if (repoState instanceof UpToDate(byte[] markerDigest)) {
return RepositoryDirectoryValue.builder()
.setPath(repoRoot)
.setDigest(markerHash)
.setDigest(markerDigest)
.setExcludeFromVendoring(excludeRepoFromVendoring)
.build();
}
Expand Down Expand Up @@ -298,24 +300,25 @@ private RepositoryDirectoryValue tryGettingValueUsingVendoredRepo(
return setupOverride(vendorRepoPath.asFragment(), env, repoRoot, repositoryName);
}

boolean isVendorRepoUpToDate =
digestWriter.areRepositoryAndMarkerFileConsistent(handler, env, vendorMarker) != null;
if (env.valuesMissing()) {
DigestWriter.RepoDirectoryState vendoredRepoState =
digestWriter.areRepositoryAndMarkerFileConsistent(handler, env, vendorMarker);
if (vendoredRepoState == null) {
return null;
}
// If our repo is up-to-date, or this is an offline build (--nofetch), then the vendored repo
// is used.
if (isVendorRepoUpToDate || (!IS_VENDOR_COMMAND.get(env).booleanValue() && !isFetch.get())) {
if (!isVendorRepoUpToDate) { // If the repo is out-of-date, show a warning
if (vendoredRepoState instanceof UpToDate
|| (!IS_VENDOR_COMMAND.get(env).booleanValue() && !isFetch.get())) {
if (vendoredRepoState instanceof OutOfDate(String reason)) {
env.getListener()
.handle(
Event.warn(
rule.getLocation(),
String.format(
"Vendored repository '%s' is out-of-date and fetching is disabled."
"Vendored repository '%s' is out-of-date (%s) and fetching is disabled."
+ " Run build without the '--nofetch' option or run"
+ " the bazel vendor command to update it",
rule.getName())));
rule.getName(), reason)));
}
return setupOverride(vendorRepoPath.asFragment(), env, repoRoot, repositoryName);
} else if (!IS_VENDOR_COMMAND.get(env).booleanValue()) { // build command & fetch enabled
Expand All @@ -325,10 +328,10 @@ private RepositoryDirectoryValue tryGettingValueUsingVendoredRepo(
Event.warn(
rule.getLocation(),
String.format(
"Vendored repository '%s' is out-of-date. The up-to-date version will"
"Vendored repository '%s' is out-of-date (%s). The up-to-date version will"
+ " be fetched into the external cache and used. To update the repo"
+ " in the vendor directory, run the bazel vendor command",
rule.getName())));
rule.getName(), ((OutOfDate) vendoredRepoState).reason())));
}
} else if (vendorFile.pinnedRepos().contains(repositoryName)) {
throw new RepositoryFunctionException(
Expand Down Expand Up @@ -625,7 +628,6 @@ private static class DigestWriter {

private final BlazeDirectories directories;
private final Path markerPath;
private final Rule rule;
private final String ruleKey;

DigestWriter(
Expand All @@ -636,7 +638,6 @@ private static class DigestWriter {
this.directories = directories;
ruleKey = computeRuleKey(rule, starlarkSemantics);
markerPath = getMarkerPath(directories, repositoryName);
this.rule = rule;
}

byte[] writeMarkerFile(Map<? extends RepoRecordedInput, String> recordedInputValues)
Expand All @@ -658,8 +659,14 @@ byte[] writeMarkerFile(Map<? extends RepoRecordedInput, String> recordedInputVal
return new Fingerprint().addString(content).digestAndReset();
}

@Nullable
byte[] areRepositoryAndMarkerFileConsistent(RepositoryFunction handler, Environment env)
sealed interface RepoDirectoryState {
record UpToDate(byte[] markerDigest) implements RepoDirectoryState {}

record OutOfDate(String reason) implements RepoDirectoryState {}
}

RepoDirectoryState areRepositoryAndMarkerFileConsistent(
RepositoryFunction handler, Environment env)
throws InterruptedException, RepositoryFunctionException {
return areRepositoryAndMarkerFileConsistent(handler, env, markerPath);
}
Expand All @@ -668,38 +675,38 @@ byte[] areRepositoryAndMarkerFileConsistent(RepositoryFunction handler, Environm
* Checks if the state of the repository in the file system is consistent with the rule in the
* WORKSPACE file.
*
* <p>Returns null if the file system is not up to date and a hash of the marker file if the
* file system is up to date.
* <p>Returns null if a Skyframe status is needed.
*
* <p>We check the repository root for existence here, but we can't depend on the FileValue,
* because it's possible that we eventually create that directory in which case the FileValue
* and the state of the file system would be inconsistent.
*/
@Nullable
byte[] areRepositoryAndMarkerFileConsistent(
RepoDirectoryState areRepositoryAndMarkerFileConsistent(
RepositoryFunction handler, Environment env, Path markerPath)
throws RepositoryFunctionException, InterruptedException {
if (!markerPath.exists()) {
return null;
return new OutOfDate("repo hasn't been fetched yet");
}

try {
String content = FileSystemUtils.readContent(markerPath, UTF_8);
Map<RepoRecordedInput, String> recordedInputValues =
readMarkerFile(content, Preconditions.checkNotNull(ruleKey));
if (!handler.verifyRecordedInputs(rule, directories, recordedInputValues, env)) {
return null;
}
Optional<String> outdatedReason =
handler.isAnyRecordedInputOutdated(directories, recordedInputValues, env);
if (env.valuesMissing()) {
return null;
}
return new Fingerprint().addString(content).digestAndReset();
if (outdatedReason.isPresent()) {
return new OutOfDate(outdatedReason.get());
}
return new UpToDate(new Fingerprint().addString(content).digestAndReset());
} catch (IOException e) {
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
}

@Nullable
private static Map<RepoRecordedInput, String> readMarkerFile(
String content, String expectedRuleKey) {
Iterable<String> lines = Splitter.on('\n').split(content);
Expand Down
Loading

0 comments on commit 30ebaf3

Please sign in to comment.