From 4260c30a03a9b83d48a5e8690aca19cd80be4c38 Mon Sep 17 00:00:00 2001 From: lberki Date: Tue, 14 Nov 2017 01:28:25 -0800 Subject: Automated rollback of commit 10b0d8aa6b73a024cc007c5e075cb329add878ef. *** Reason for rollback *** Breaks Google-internal targets, sadly. *** Original change description *** Ban middlemen from runfiles artifacts. Previous changes have removed all middlemen from runfiles artifacts. This CL locks it down and removes various now-redundant *WithoutMiddlemen() methods from Runfiles. I put a check for middlemen in ConflictChecker.put, which should be a chokepoint for runfiles artifacts. It's unfortunate we can't detect middlemen earlier than execution, but I can't see a way to efficiently check every runfiles artifact earlier. Cha... *** RELNOTES: None. PiperOrigin-RevId: 175650018 --- .../devtools/build/lib/analysis/Runfiles.java | 23 ++++++++++++++++++---- .../build/lib/analysis/RunfilesSupplierImpl.java | 3 ++- .../build/lib/analysis/RunfilesSupport.java | 10 +++++----- .../build/lib/analysis/SourceManifestAction.java | 2 +- .../bazel/rules/python/BazelPythonSemantics.java | 2 +- .../rules/android/AndroidHostServiceFixture.java | 2 +- .../android/LibraryRGeneratorActionBuilder.java | 2 +- .../rules/android/ManifestMergerActionBuilder.java | 2 +- .../android/RClassGeneratorActionBuilder.java | 2 +- 9 files changed, 32 insertions(+), 16 deletions(-) (limited to 'src/main/java/com/google/devtools/build') 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 7392ed4e79..1313ab6c6e 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 @@ -330,6 +330,15 @@ public final class Runfiles { return unconditionalArtifacts; } + /** + * Returns the artifacts that are unconditionally included in the runfiles (as opposed to + * pruning manifest candidates, which may or may not be included). Middleman artifacts are + * excluded. + */ + public Iterable getUnconditionalArtifactsWithoutMiddlemen() { + return Iterables.filter(unconditionalArtifacts, Artifact.MIDDLEMAN_FILTER); + } + public NestedSet getExtraMiddlemen() { return extraMiddlemen; } @@ -352,6 +361,14 @@ public final class Runfiles { return allArtifacts.build(); } + /** + * Returns the collection of runfiles as artifacts, including both unconditional artifacts + * and pruning manifest candidates. Middleman artifacts are excluded. + */ + public Iterable getArtifactsWithoutMiddlemen() { + return Iterables.filter(getArtifacts(), Artifact.MIDDLEMAN_FILTER); + } + /** Returns the symlinks. */ @SkylarkCallable(name = "symlinks", doc = "Returns the set of symlinks.", structField = true) public NestedSet getSymlinks() { @@ -439,7 +456,7 @@ public final class Runfiles { ConflictChecker checker = new ConflictChecker(conflictPolicy, eventHandler, location); Map manifest = getSymlinksAsMap(checker); // Add unconditional artifacts (committed to inclusion on construction of runfiles). - for (Artifact artifact : getUnconditionalArtifacts()) { + for (Artifact artifact : getUnconditionalArtifactsWithoutMiddlemen()) { checker.put(manifest, artifact.getRootRelativePath(), artifact); } @@ -592,7 +609,7 @@ public final class Runfiles { // 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. - for (Artifact artifact : unconditionalArtifacts) { + for (Artifact artifact : Iterables.filter(unconditionalArtifacts, Artifact.MIDDLEMAN_FILTER)) { result.put(artifact.getRootRelativePath(), artifact); } return result; @@ -712,8 +729,6 @@ public final class Runfiles { * @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) { - Preconditions.checkArgument( - artifact == null || !artifact.isMiddlemanArtifact(), "%s", artifact); if (policy != ConflictPolicy.IGNORE && map.containsKey(path)) { // Previous and new entry might have value of null Artifact previous = map.get(path); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java index c609d3d787..a86323496d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupplierImpl.java @@ -18,6 +18,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.BaseSpawn; import com.google.devtools.build.lib.actions.RunfilesSupplier; @@ -72,7 +73,7 @@ public class RunfilesSupplierImpl implements RunfilesSupplier { @Override public Iterable getArtifacts() { - return runfiles.getAllArtifacts(); + return Iterables.filter(runfiles.getAllArtifacts(), Artifact.MIDDLEMAN_FILTER); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index a58a020ee3..75f0a1d9c0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java @@ -233,12 +233,12 @@ public final class RunfilesSupport { } /** - * Returns both runfiles artifacts and "conditional" artifacts that may be part of a Runfiles - * PruningManifest. This means the returned set may be an overapproximation of the actual set of - * runfiles (see {@link Runfiles.PruningManifest}). + * Returns both runfiles artifacts and "conditional" artifacts that may be part of a + * Runfiles PruningManifest. This means the returned set may be an overapproximation of the + * actual set of runfiles (see {@link Runfiles.PruningManifest}). */ - public Iterable getRunfilesArtifacts() { - return runfiles.getArtifacts(); + public Iterable getRunfilesArtifactsWithoutMiddlemen() { + return runfiles.getArtifactsWithoutMiddlemen(); } /** 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 864f2cfe81..0fe1ba736e 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 @@ -210,7 +210,7 @@ public final class SourceManifestAction extends AbstractFileWriteAction { f.addPath(rootSymlink.getValue().getPath()); } - for (Artifact artifact : runfiles.getArtifacts()) { + for (Artifact artifact : runfiles.getArtifactsWithoutMiddlemen()) { f.addPath(artifact.getRootRelativePath()); f.addPath(artifact.getPath()); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 8b0cfaf5d3..52d839c4d9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -276,7 +276,7 @@ public class BazelPythonSemantics implements PythonSemantics { // Read each runfile from execute path, add them into zip file at the right runfiles path. // Filter the executable file, cause we are building it. - for (Artifact artifact : runfilesSupport.getRunfilesArtifacts()) { + for (Artifact artifact : runfilesSupport.getRunfilesArtifactsWithoutMiddlemen()) { if (!artifact.equals(executable) && !artifact.equals(zipFile)) { argv.addDynamicString( getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName) diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidHostServiceFixture.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidHostServiceFixture.java index 9996d47e5d..676a9ee6df 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidHostServiceFixture.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidHostServiceFixture.java @@ -43,7 +43,7 @@ public class AndroidHostServiceFixture implements RuleConfiguredTargetFactory { .build(); Runfiles runfiles = new Runfiles.Builder(ruleContext.getWorkspaceName()) - .addTransitiveArtifacts(supportApks) + .addTransitiveArtifacts(filesToBuild) .merge(executable.getRunfilesSupport()) .build(); return ruleBuilder diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/LibraryRGeneratorActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/LibraryRGeneratorActionBuilder.java index a74487fe61..24ec6aec33 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/LibraryRGeneratorActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/LibraryRGeneratorActionBuilder.java @@ -63,7 +63,7 @@ public class LibraryRGeneratorActionBuilder { NestedSetBuilder inputs = NestedSetBuilder.naiveLinkOrder(); FilesToRunProvider executable = ruleContext.getExecutablePrerequisite("$android_resources_busybox", Mode.HOST); - inputs.addAll(executable.getRunfilesSupport().getRunfilesArtifacts()); + inputs.addAll(executable.getRunfilesSupport().getRunfilesArtifactsWithoutMiddlemen()); builder.add("--tool").add("GENERATE_LIBRARY_R").add("--"); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java index a4d43ed382..0df89bf3a8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/ManifestMergerActionBuilder.java @@ -99,7 +99,7 @@ public class ManifestMergerActionBuilder { ruleContext .getExecutablePrerequisite("$android_resources_busybox", Mode.HOST) .getRunfilesSupport() - .getRunfilesArtifacts()); + .getRunfilesArtifactsWithoutMiddlemen()); builder.addExecPath("--manifest", manifest); inputs.add(manifest); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java index 16505b92b8..f6cf02e841 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/RClassGeneratorActionBuilder.java @@ -85,7 +85,7 @@ public class RClassGeneratorActionBuilder { ruleContext .getExecutablePrerequisite("$android_resources_busybox", Mode.HOST) .getRunfilesSupport() - .getRunfilesArtifacts()); + .getRunfilesArtifactsWithoutMiddlemen()); List outs = new ArrayList<>(); if (primary.getRTxt() != null) { -- cgit v1.2.3