From 7e689a55ccdcd752c102d25fe9acb257bd7d881c Mon Sep 17 00:00:00 2001 From: Xdng Yng Date: Tue, 6 Aug 2024 14:50:17 -0700 Subject: [PATCH] Be resilient to outdated exec paths in action cache entries https://github.com/bazelbuild/bazel/commit/60924fdc2972184494f6382d39e8c786aa14b9a9 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 https://github.com/bazelbuild/bazel/issues/23180. Closes #23227. PiperOrigin-RevId: 660105601 Change-Id: Iea5d86c635056d12ba20598383da463bdde03ab0 --- .../build/lib/cmdline/PackageIdentifier.java | 17 +++++++++++++---- .../build/lib/rules/cpp/HeaderDiscovery.java | 13 ++++++++----- .../lib/skyframe/ActionExecutionFunction.java | 13 ++++++++----- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java index af37200bff3387..f4dcf2194e98b4 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java @@ -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; @@ -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. + * + *

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 discoverFromExecPath( PathFragment execPath, boolean forFiles, boolean siblingRepositoryLayout) { Preconditions.checkArgument(!execPath.isAbsolute(), execPath); PathFragment tofind = @@ -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)); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java index 40f1f71d461c48..bb3d0689562123 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java @@ -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; @@ -32,6 +31,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -178,10 +178,13 @@ private static NestedSet runDiscovery( } Artifact artifact = regularDerivedArtifacts.get(execPathFragment); if (artifact == null) { - RepositoryName repository = - PackageIdentifier.discoverFromExecPath(execPathFragment, false, siblingRepositoryLayout) - .getRepository(); - artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repository); + Optional 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. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 835f707ef33796..ff8c72207dd942 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -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; @@ -728,11 +729,13 @@ public Map findPackageRootsForFiles(Iterable 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 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());