Skip to content

Commit

Permalink
Be resilient to outdated exec paths in action cache entries
Browse files Browse the repository at this point in the history
60924fd changed the canonical repo name separator from `~` to `+`. Older repo names containing `~` now trigger a syntax error. If an action cache entry refers to an exec path from a previous version of Bazel that used `~`, we need to be resilient and treat the cache entry as corrupted, rather than just crash.

Fixes #23180.

Closes #23227.

PiperOrigin-RevId: 660105601
Change-Id: Iea5d86c635056d12ba20598383da463bdde03ab0
  • Loading branch information
Wyverald authored and keertk committed Aug 12, 2024
1 parent 914db36 commit 354ea83
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.protobuf.CodedInputStream;
import com.google.protobuf.CodedOutputStream;
import java.io.IOException;
import java.util.Optional;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -88,8 +89,11 @@ public static PackageIdentifier createRootPackage(RepositoryName repository) {
*
* In this case, this method returns a package identifier for foo/bar, even though that is not a
* package. Callers need to look up the actual package if needed.
*
* <p>Returns {@link Optional#empty()} if the path corresponds to an invalid label (e.g. with a
* malformed repo name).
*/
public static PackageIdentifier discoverFromExecPath(
public static Optional<PackageIdentifier> discoverFromExecPath(
PathFragment execPath, boolean forFiles, boolean siblingRepositoryLayout) {
Preconditions.checkArgument(!execPath.isAbsolute(), execPath);
PathFragment tofind =
Expand All @@ -104,10 +108,15 @@ public static PackageIdentifier discoverFromExecPath(
if (tofind.startsWith(prefix)) {
// Using the path prefix can be either "external" or "..", depending on whether the sibling
// repository layout is used.
RepositoryName repository = RepositoryName.createUnvalidated(tofind.getSegment(1));
return PackageIdentifier.create(repository, tofind.subFragment(2));
try {
RepositoryName repository = RepositoryName.create(tofind.getSegment(1));
return Optional.of(PackageIdentifier.create(repository, tofind.subFragment(2)));
} catch (LabelSyntaxException e) {
// The path corresponds to an invalid label.
return Optional.empty();
}
} else {
return PackageIdentifier.createInMainRepo(tofind);
return Optional.of(PackageIdentifier.createInMainRepo(tofind));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand All @@ -32,6 +31,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -178,10 +178,13 @@ private static NestedSet<Artifact> runDiscovery(
}
Artifact artifact = regularDerivedArtifacts.get(execPathFragment);
if (artifact == null) {
RepositoryName repository =
PackageIdentifier.discoverFromExecPath(execPathFragment, false, siblingRepositoryLayout)
.getRepository();
artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repository);
Optional<PackageIdentifier> pkgId =
PackageIdentifier.discoverFromExecPath(
execPathFragment, false, siblingRepositoryLayout);
if (pkgId.isPresent()) {
artifact =
artifactResolver.resolveSourceArtifact(execPathFragment, pkgId.get().getRepository());
}
}
if (artifact != null) {
// We don't need to add the sourceFile itself as it is a mandatory input.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.IntFunction;
Expand Down Expand Up @@ -728,11 +729,13 @@ public Map<PathFragment, Root> findPackageRootsForFiles(Iterable<PathFragment> e
PathFragment parent =
checkNotNull(path.getParentDirectory(), "Must pass in files, not root directory");
checkArgument(!parent.isAbsolute(), path);
ContainingPackageLookupValue.Key depKey =
ContainingPackageLookupValue.key(
PackageIdentifier.discoverFromExecPath(path, true, siblingRepositoryLayout));
depKeys.put(path, depKey);
packageLookupsRequested.add(depKey);
Optional<PackageIdentifier> pkgId =
PackageIdentifier.discoverFromExecPath(path, true, siblingRepositoryLayout);
if (pkgId.isPresent()) {
ContainingPackageLookupValue.Key depKey = ContainingPackageLookupValue.key(pkgId.get());
depKeys.put(path, depKey);
packageLookupsRequested.add(depKey);
}
}

SkyframeLookupResult values = env.getValuesAndExceptions(depKeys.values());
Expand Down

0 comments on commit 354ea83

Please sign in to comment.