Skip to content

Commit

Permalink
[7.3.0] Be resilient to outdated exec paths in action cache entries (#…
Browse files Browse the repository at this point in the history
…23230)

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 Aug 6, 2024
1 parent 5f5355b commit acc1ff6
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 @@ -23,6 +23,7 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyKey.SkyKeyInterner;
import java.util.Optional;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -77,8 +78,11 @@ public static PackageIdentifier createInMainRepo(PathFragment name) {
*
* 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 @@ -93,10 +97,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 @@ -116,6 +116,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.Consumer;
Expand Down Expand Up @@ -686,11 +687,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 acc1ff6

Please sign in to comment.