Remove the runfiles middleman from py_binary's data runfiles. This requires polluting Runfiles and RuleConfiguredTargetBuilder with some custom infrastructure to lift a py_binary runfiles middleman into the FilesToRunProviders and output groups of its reverse dependency. That can all go away when the --experimental_build_transitive_python_runfiles transition is complete. This is another step towards https://bazel-review.googlesource.com/c/bazel/+/14010. Change-Id: Ib750d72d4be42324c8edec485707480690b9fc9c PiperOrigin-RevId: 173514090
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index 44ab47c..a67e9d0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -88,16 +88,21 @@ return null; } + NestedSetBuilder<Artifact> runfilesMiddlemenBuilder = NestedSetBuilder.stableOrder(); + if (runfilesSupport != null) { + runfilesMiddlemenBuilder.add(runfilesSupport.getRunfilesMiddleman()); + runfilesMiddlemenBuilder.addTransitive(runfilesSupport.getRunfiles().getExtraMiddlemen()); + } + NestedSet<Artifact> runfilesMiddlemen = runfilesMiddlemenBuilder.build(); FilesToRunProvider filesToRunProvider = new FilesToRunProvider( - buildFilesToRun(runfilesSupport, filesToBuild), runfilesSupport, executable); + buildFilesToRun(runfilesMiddlemen, filesToBuild), runfilesSupport, executable); addProvider(new FileProvider(filesToBuild)); addProvider(filesToRunProvider); if (runfilesSupport != null) { // If a binary is built, build its runfiles, too - addOutputGroup( - OutputGroupProvider.HIDDEN_TOP_LEVEL, runfilesSupport.getRunfilesMiddleman()); + addOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL, runfilesMiddlemen); } else if (providersBuilder.contains(RunfilesProvider.class)) { // If we don't have a RunfilesSupport (probably because this is not a binary rule), we still // want to build the files this rule contributes to runfiles of dependent rules so that we @@ -142,15 +147,13 @@ /** * Compute the artifacts to put into the {@link FilesToRunProvider} for this target. These are the - * filesToBuild, any artifacts added by the rule with {@link #addFilesToRun}, and the runfiles - * middleman if it exists. + * filesToBuild, any artifacts added by the rule with {@link #addFilesToRun}, and the runfiles' + * middlemen if they exists. */ private NestedSet<Artifact> buildFilesToRun( - RunfilesSupport runfilesSupport, NestedSet<Artifact> filesToBuild) { + NestedSet<Artifact> runfilesMiddlemen, NestedSet<Artifact> filesToBuild) { filesToRunBuilder.addTransitive(filesToBuild); - if (runfilesSupport != null) { - filesToRunBuilder.add(runfilesSupport.getRunfilesMiddleman()); - } + filesToRunBuilder.addTransitive(runfilesMiddlemen); return filesToRunBuilder.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 65009a3..f75d290 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
@@ -201,6 +201,12 @@ private final NestedSet<SymlinkEntry> rootSymlinks; /** + * A set of middlemen artifacts. {@link RuleConfiguredTargetBuilder} adds these to the {@link + * FilesToRunProvider} of binaries that include this runfiles tree in their runfiles. + */ + private final NestedSet<Artifact> extraMiddlemen; + + /** * Interface used for adding empty files to the runfiles at the last minute. Mainly to support * python-related rules adding __init__.py files. */ @@ -294,6 +300,7 @@ NestedSet<SymlinkEntry> symlinks, NestedSet<SymlinkEntry> rootSymlinks, NestedSet<PruningManifest> pruningManifests, + NestedSet<Artifact> extraMiddlemen, EmptyFilesSupplier emptyFilesSupplier, ConflictPolicy conflictPolicy, boolean legacyExternalRunfiles) { @@ -301,6 +308,7 @@ this.unconditionalArtifacts = Preconditions.checkNotNull(artifacts); this.symlinks = Preconditions.checkNotNull(symlinks); this.rootSymlinks = Preconditions.checkNotNull(rootSymlinks); + this.extraMiddlemen = Preconditions.checkNotNull(extraMiddlemen); this.pruningManifests = Preconditions.checkNotNull(pruningManifests); this.emptyFilesSupplier = Preconditions.checkNotNull(emptyFilesSupplier); this.conflictPolicy = conflictPolicy; @@ -331,6 +339,10 @@ return Iterables.filter(unconditionalArtifacts, Artifact.MIDDLEMAN_FILTER); } + public NestedSet<Artifact> getExtraMiddlemen() { + return extraMiddlemen; + } + /** * Returns the collection of runfiles as artifacts, including both unconditional artifacts and * pruning manifest candidates. @@ -641,8 +653,11 @@ * Returns if there are no runfiles. */ public boolean isEmpty() { - return unconditionalArtifacts.isEmpty() && symlinks.isEmpty() && rootSymlinks.isEmpty() - && pruningManifests.isEmpty(); + return unconditionalArtifacts.isEmpty() + && symlinks.isEmpty() + && rootSymlinks.isEmpty() + && pruningManifests.isEmpty() + && extraMiddlemen.isEmpty(); } /** @@ -753,6 +768,7 @@ NestedSetBuilder.stableOrder(); private NestedSetBuilder<PruningManifest> pruningManifestsBuilder = NestedSetBuilder.stableOrder(); + private NestedSetBuilder<Artifact> extraMiddlemenBuilder = NestedSetBuilder.stableOrder(); private EmptyFilesSupplier emptyFilesSupplier = DUMMY_EMPTY_FILES_SUPPLIER; /** Build the Runfiles object with this policy */ @@ -804,9 +820,16 @@ * Builds a new Runfiles object. */ public Runfiles build() { - return new Runfiles(suffix, artifactsBuilder.build(), symlinksBuilder.build(), - rootSymlinksBuilder.build(), pruningManifestsBuilder.build(), - emptyFilesSupplier, conflictPolicy, legacyExternalRunfiles); + return new Runfiles( + suffix, + artifactsBuilder.build(), + symlinksBuilder.build(), + rootSymlinksBuilder.build(), + pruningManifestsBuilder.build(), + extraMiddlemenBuilder.build(), + emptyFilesSupplier, + conflictPolicy, + legacyExternalRunfiles); } /** @@ -1051,6 +1074,17 @@ } /** + * Add extra middlemen artifacts that should be built by reverse dependency binaries. This + * method exists solely to support the unfortunate legacy behavior of some rules; new uses + * should not be added. + */ + public Builder addLegacyExtraMiddleman(Artifact middleman) { + Preconditions.checkArgument(middleman.isMiddlemanArtifact(), middleman); + extraMiddlemenBuilder.add(middleman); + return this; + } + + /** * Add the other {@link Runfiles} object transitively. */ public Builder merge(Runfiles runfiles) { @@ -1097,6 +1131,7 @@ if (includePruningManifests) { pruningManifestsBuilder.addTransitive(runfiles.getPruningManifests()); } + extraMiddlemenBuilder.addTransitive(runfiles.getExtraMiddlemen()); if (emptyFilesSupplier == DUMMY_EMPTY_FILES_SUPPLIER) { emptyFilesSupplier = runfiles.getEmptyFilesProvider(); } else {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java index 56c5185..e070692 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
@@ -107,7 +107,7 @@ ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles()) .merge(commonRunfiles) - .addArtifact(runfilesSupport.getRunfilesMiddleman()) + .addLegacyExtraMiddleman(runfilesSupport.getRunfilesMiddleman()) .build(); } else { dataRunfiles = commonRunfiles;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java index aff8e5c..5a16caa 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
@@ -16,6 +16,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; @@ -408,4 +409,25 @@ assertThat(runfilesC.getSymlinksAsMap(null).get(sympathA)).isEqualTo(artifactA); assertThat(runfilesC.getSymlinksAsMap(null).get(sympathB)).isEqualTo(artifactB); } + + @Test + public void testOnlyExtraMiddlemenNotConsideredEmpty() { + Root root = Root.middlemanRoot(scratch.resolve("execroot"), scratch.resolve("execroot/out")); + Artifact mm = new Artifact(PathFragment.create("a-middleman"), root); + Runfiles runfiles = new Runfiles.Builder("TESTING").addLegacyExtraMiddleman(mm).build(); + assertThat(runfiles.isEmpty()).isFalse(); + } + + @Test + public void testMergingExtraMiddlemen() { + Root root = Root.middlemanRoot(scratch.resolve("execroot"), scratch.resolve("execroot/out")); + Artifact mm1 = new Artifact(PathFragment.create("middleman-1"), root); + Artifact mm2 = new Artifact(PathFragment.create("middleman-2"), root); + Runfiles runfiles1 = new Runfiles.Builder("TESTING").addLegacyExtraMiddleman(mm1).build(); + Runfiles runfiles2 = new Runfiles.Builder("TESTING").addLegacyExtraMiddleman(mm2).build(); + Runfiles runfilesMerged = + new Runfiles.Builder("TESTING").merge(runfiles1).merge(runfiles2).build(); + assertThat(runfilesMerged.getExtraMiddlemen()) + .containsExactlyElementsIn(ImmutableList.of(mm1, mm2)); + } }