From 3ed13c847ec4953442f8ff36461196c1b978d1ef Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 7 Mar 2016 21:53:29 +0000 Subject: Detect and warn about runfiles conflicts. A runfile conflict is when two different artifacts have been added to a Runfiles object under the same relative path. Conflict resolution is unchanged (last artifact wins). -- MOS_MIGRATED_REVID=116584195 --- .../devtools/build/lib/analysis/Runfiles.java | 158 +++++++++++++++++---- .../build/lib/analysis/SourceManifestAction.java | 8 +- 2 files changed, 135 insertions(+), 31 deletions(-) (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java index 83657f695e..c2dfa756e7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; @@ -43,6 +44,7 @@ import java.util.Iterator; import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.Objects; import java.util.Set; import javax.annotation.Nullable; @@ -122,6 +124,8 @@ public final class Runfiles { * The directory to put all runfiles under. * *

Using "foo" will put runfiles under <target>.runfiles/foo.

+ * + *

This is either set to the workspace name, or is empty. */ private final String suffix; @@ -164,6 +168,25 @@ public final class Runfiles { /** Generates extra (empty file) inputs. */ private final EmptyFilesSupplier emptyFilesSupplier; + /** + * Behavior upon finding a conflict between two runfile entries. A conflict means that two + * different artifacts have the same runfiles path specified. For example, adding artifact + * "a.foo" at path "bar" when there is already an artifact "b.foo" at path "bar". + * + *

Note that conflicts are found relatively late, when the manifest file is created, not when + * the symlinks are added to runfiles. + * + *

If no EventHandler is available, all values are treated as IGNORE. + */ + public static enum ConflictPolicy { + IGNORE, + WARN, + ERROR, + } + + /** Policy for this Runfiles tree */ + private ConflictPolicy conflictPolicy = ConflictPolicy.IGNORE; + /** * Defines a set of artifacts that may or may not be included in the runfiles directory and * a manifest file that makes that determination. These are applied on top of any artifacts @@ -288,9 +311,11 @@ public final class Runfiles { /** * Returns the symlinks as a map from path fragment to artifact. + * + * @param checker If not null, check for conflicts using this checker. */ - public Map getSymlinksAsMap() { - return entriesToMap(symlinks); + public Map getSymlinksAsMap(@Nullable ConflictChecker checker) { + return entriesToMap(symlinks, checker); } /** @@ -340,18 +365,19 @@ public final class Runfiles { * Returns the symlinks as a map from PathFragment to Artifact. * * @param eventHandler Used for throwing an error if we have an obscuring runlink within the - * normal source tree entries. May be null, in which case obscuring symlinks are silently - * discarded. + * normal source tree entries, or runfile conflicts. May be null, in which case obscuring + * symlinks are silently discarded, and conflicts are overwritten. * @param location Location for eventHandler warnings. Ignored if eventHandler is null. * @return Map path fragment to artifact, of normal source tree entries * and elements that live outside the source tree. Null values represent empty input files. */ public Map getRunfilesInputs(EventHandler eventHandler, Location location) throws IOException { - Map manifest = getSymlinksAsMap(); + ConflictChecker checker = new ConflictChecker(conflictPolicy, eventHandler, location); + Map manifest = getSymlinksAsMap(checker); // Add unconditional artifacts (committed to inclusion on construction of runfiles). for (Artifact artifact : getUnconditionalArtifactsWithoutMiddlemen()) { - manifest.put(artifact.getRootRelativePath(), artifact); + checker.put(manifest, artifact.getRootRelativePath(), artifact); } // Add conditional artifacts (only included if they appear in a pruning manifest). @@ -367,7 +393,7 @@ public final class Runfiles { while ((line = reader.readLine()) != null) { Artifact artifact = allowedRunfiles.get(line); if (artifact != null) { - manifest.put(artifact.getRootRelativePath(), artifact); + checker.put(manifest, artifact.getRootRelativePath(), artifact); } } } @@ -376,29 +402,30 @@ public final class Runfiles { // TODO(bazel-team): Create /dev/null-like Artifact to avoid nulls? for (PathFragment extraPath : emptyFilesSupplier.getExtraPaths(manifest.keySet())) { - manifest.put(extraPath, null); + checker.put(manifest, extraPath, null); } - PathFragment path = new PathFragment(suffix); - Map result = new HashMap<>(); + // Copy manifest map to another manifest map, prepending the workspace name to every path. + // E.g. for workspace "myworkspace", the runfile entry "mylib.so"->"/path/to/mylib.so" becomes + // "myworkspace/mylib.so"->"/path/to/mylib.so". + PathFragment suffixPath = new PathFragment(suffix); + Map rootManifest = new HashMap<>(); for (Map.Entry entry : manifest.entrySet()) { - result.put(path.getRelative(entry.getKey()), entry.getValue()); + checker.put(rootManifest, suffixPath.getRelative(entry.getKey()), entry.getValue()); } - // Finally add symlinks outside the source tree on top of everything else. - for (Map.Entry entry : getRootSymlinksAsMap().entrySet()) { + // Finally add symlinks relative to the root of the runfiles tree, on top of everything else. + // This operation is always checked for conflicts, to match historical behavior. + if (conflictPolicy == ConflictPolicy.IGNORE) { + checker = new ConflictChecker(ConflictPolicy.WARN, eventHandler, location); + } + for (Map.Entry entry : getRootSymlinksAsMap(checker).entrySet()) { PathFragment mappedPath = entry.getKey(); Artifact mappedArtifact = entry.getValue(); - if (result.put(mappedPath, mappedArtifact) != null) { - // Emit warning if we overwrote something and we're capable of emitting warnings. - if (eventHandler != null) { - eventHandler.handle(Event.warn(location, "overwrote " + mappedPath + " symlink mapping " - + "with root symlink to " + mappedArtifact)); - } - } + checker.put(rootManifest, mappedPath, mappedArtifact); } - return result; + return rootManifest; } /** @@ -409,10 +436,12 @@ public final class Runfiles { } /** - * Returns the root symlinks. + * Returns the root symlinks as a map from path fragment to artifact. + * + * @param checker If not null, check for conflicts using this checker. */ - public Map getRootSymlinksAsMap() { - return entriesToMap(rootSymlinks); + public Map getRootSymlinksAsMap(@Nullable ConflictChecker checker) { + return entriesToMap(rootSymlinks, checker); } /** @@ -420,7 +449,7 @@ public final class Runfiles { * account. */ public Map asMapWithoutRootSymlinks() { - Map result = entriesToMap(symlinks); + Map result = entriesToMap(symlinks, null); // If multiple artifacts have the same root-relative path, the last one in the list will win. // That is because the runfiles tree cannot contain the same artifact for different // configurations, because it only uses root-relative paths. @@ -472,19 +501,94 @@ public final class Runfiles { pruningManifests.isEmpty(); } - private static Map entriesToMap(Iterable entrySet) { + /** + * Flatten a sequence of entries into a single map. + * + * @param entrySet Sequence of entries to add. + * @param checker If not null, check for conflicts with this checker, otherwise silently allow + * entries to overwrite previous entries. + * @return Map Map of runfile entries. + */ + private static Map entriesToMap( + Iterable entrySet, @Nullable ConflictChecker checker) { + checker = (checker != null) ? checker : ConflictChecker.IGNORE_CHECKER; Map map = new LinkedHashMap<>(); for (SymlinkEntry entry : entrySet) { - map.put(entry.getPath(), entry.getArtifact()); + checker.put(map, entry.getPath(), entry.getArtifact()); } return map; } + /** Set whether we should warn about conflicting symlink entries. */ + public void setConflictPolicy(ConflictPolicy conflictPolicy) { + this.conflictPolicy = conflictPolicy; + } + + /** + * Checks for conflicts between entries in a runfiles tree while putting them in a map. + */ + public static final class ConflictChecker { + /** Prebuilt ConflictChecker with policy set to IGNORE */ + public static final ConflictChecker IGNORE_CHECKER = + new ConflictChecker(ConflictPolicy.IGNORE, null, null); + + /** Behavior when a conflict is found. */ + private final ConflictPolicy policy; + + /** Used for warning on conflicts. May be null, in which case conflicts are ignored. */ + private final EventHandler eventHandler; + + /** Location for eventHandler warnings. Ignored if eventHandler is null. */ + private final Location location; + + /** Type of event to emit */ + private final EventKind eventKind; + + /** Construct a ConflictChecker for the given reporter with the given behavior */ + public ConflictChecker(ConflictPolicy policy, EventHandler eventHandler, Location location) { + if (eventHandler == null) { + this.policy = ConflictPolicy.IGNORE; // Can't warn even if we wanted to + } else { + this.policy = policy; + } + this.eventHandler = eventHandler; + this.location = location; + this.eventKind = (policy == ConflictPolicy.ERROR) ? EventKind.ERROR : EventKind.WARNING; + } + + /** + * Add an entry to a Map of symlinks, optionally reporting conflicts. + * + * @param map Manifest of runfile entries. + * @param path Path fragment to use as key in map. + * @param artifact Artifact to store in map. This may be null to indicate an empty file. + */ + public void put(Map map, PathFragment path, Artifact artifact) { + if (policy != ConflictPolicy.IGNORE && map.containsKey(path)) { + // Previous and new entry might have value of null + Artifact previous = map.get(path); + if (!Objects.equals(previous, artifact)) { + String previousStr = (previous == null) ? "empty file" : previous.getPath().toString(); + String artifactStr = (artifact == null) ? "empty file" : artifact.getPath().toString(); + String message = + String.format( + "overwrote runfile %s, was symlink to %s, now symlink to %s", + path.getSafePathString(), + previousStr, + artifactStr); + eventHandler.handle(new Event(eventKind, location, message)); + } + } + map.put(path, artifact); + } + } + /** * Builder for Runfiles objects. */ public static final class Builder { + /** This is set to the workspace name */ private String suffix; /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java index c782656b26..04792e8db4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java @@ -70,12 +70,12 @@ public class SourceManifestAction extends AbstractFileWriteAction { @Nullable Artifact symlink) throws IOException; /** - * Fulfills {@link #ActionMetadata.getMnemonic()} + * Fulfills {@link com.google.devtools.build.lib.actions.AbstractAction#getMnemonic()} */ String getMnemonic(); /** - * Fulfills {@link #AbstractAction.getRawProgressMessage()} + * Fulfills {@link com.google.devtools.build.lib.actions.AbstractAction#getRawProgressMessage()} */ String getRawProgressMessage(); } @@ -189,13 +189,13 @@ public class SourceManifestAction extends AbstractFileWriteAction { protected String computeKey() { Fingerprint f = new Fingerprint(); f.addString(GUID); - Map symlinks = runfiles.getSymlinksAsMap(); + Map symlinks = runfiles.getSymlinksAsMap(null); f.addInt(symlinks.size()); for (Map.Entry symlink : symlinks.entrySet()) { f.addPath(symlink.getKey()); f.addPath(symlink.getValue().getPath()); } - Map rootSymlinks = runfiles.getRootSymlinksAsMap(); + Map rootSymlinks = runfiles.getRootSymlinksAsMap(null); f.addInt(rootSymlinks.size()); for (Map.Entry rootSymlink : rootSymlinks.entrySet()) { f.addPath(rootSymlink.getKey()); -- cgit v1.2.3