From f311efc565c8e0eeddd5283834df69f69c860601 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 20 Jun 2024 07:40:24 -0700 Subject: [PATCH] Refactor `ManifestWriter` to accept `PathFragment` instead of `Artifact`. There is only one caller that needs to do the transformation from `Artifact` to `PathFragment`, so pushing it there doesn't lead to any duplication. PiperOrigin-RevId: 645026800 Change-Id: I04cb980a547442c168c9ad1363b7a7d36dbb7d65 --- .../lib/analysis/SourceManifestAction.java | 34 ++++--- .../analysis/SourceManifestActionTest.java | 89 +++++++++---------- 2 files changed, 67 insertions(+), 56 deletions(-) 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 9e39233c6695c3..f359e6cb9ec27a 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 @@ -76,10 +76,11 @@ interface ManifestWriter { * * @param manifestWriter the output stream * @param rootRelativePath path of an entry relative to the manifest's root - * @param symlink (optional) symlink that resolves the above path + * @param symlinkTarget target of the entry at {@code rootRelativePath} if it is a symlink, + * otherwise {@code null} */ void writeEntry( - Writer manifestWriter, PathFragment rootRelativePath, @Nullable Artifact symlink) + Writer manifestWriter, PathFragment rootRelativePath, @Nullable PathFragment symlinkTarget) throws IOException; /** Fulfills {@link com.google.devtools.build.lib.actions.AbstractAction#getMnemonic()} */ @@ -235,7 +236,16 @@ private void writeFile(OutputStream out, Map output) thr List> sortedManifest = new ArrayList<>(output.entrySet()); sortedManifest.sort(ENTRY_COMPARATOR); for (Map.Entry line : sortedManifest) { - manifestWriter.writeEntry(manifestFile, line.getKey(), line.getValue()); + Artifact artifact = line.getValue(); + PathFragment symlinkTarget; + if (artifact == null) { + symlinkTarget = null; + } else if (artifact.isSymlink()) { + symlinkTarget = artifact.getPath().readSymbolicLink(); + } else { + symlinkTarget = artifact.getPath().asFragment(); + } + manifestWriter.writeEntry(manifestFile, line.getKey(), symlinkTarget); } manifestFile.flush(); @@ -287,17 +297,16 @@ public enum ManifestType implements ManifestWriter { */ SOURCE_SYMLINKS { @Override - public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink) + public void writeEntry( + Writer manifestWriter, + PathFragment rootRelativePath, + @Nullable PathFragment symlinkTarget) throws IOException { manifestWriter.append(rootRelativePath.getPathString()); // This trailing whitespace is REQUIRED to process the single entry line correctly. manifestWriter.append(' '); - if (symlink != null) { - if (symlink.isSymlink()) { - manifestWriter.append(symlink.getPath().readSymbolicLink().getPathString()); - } else { - manifestWriter.append(symlink.getPath().getPathString()); - } + if (symlinkTarget != null) { + manifestWriter.append(symlinkTarget.getPathString()); } manifestWriter.append('\n'); } @@ -335,7 +344,10 @@ public boolean emitsAbsolutePaths() { */ SOURCES_ONLY { @Override - public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink) + public void writeEntry( + Writer manifestWriter, + PathFragment rootRelativePath, + @Nullable PathFragment symlinkTarget) throws IOException { manifestWriter.append(rootRelativePath.getPathString()); manifestWriter.append('\n'); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java index 7242963c6e1a4b..d51212772d5394 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java @@ -25,7 +25,6 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; -import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; @@ -34,7 +33,6 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; -import java.io.IOException; import java.io.Writer; import java.util.ArrayList; import java.util.LinkedHashMap; @@ -54,38 +52,33 @@ public final class SourceManifestActionTest extends BuildViewTestCase { private Map fakeManifest; - - private Path pythonSourcePath; - private Artifact pythonSourceFile; - private Path buildFilePath; private Artifact buildFile; private Artifact relativeSymlink; private Artifact absoluteSymlink; - - private Path manifestOutputPath; private Artifact manifestOutputFile; @Before - public final void createFiles() throws Exception { + public void createFiles() throws Exception { analysisMock.pySupport().setup(mockToolsConfig); // Test with a raw manifest Action. fakeManifest = new LinkedHashMap<>(); ArtifactRoot trivialRoot = ArtifactRoot.asSourceRoot(Root.fromPath(rootDirectory.getRelative("trivial"))); - buildFilePath = scratch.file("trivial/BUILD", - "py_binary(name='trivial', srcs =['trivial.py'])"); + Path buildFilePath = + scratch.file("trivial/BUILD", "py_binary(name='trivial', srcs =['trivial.py'])"); buildFile = ActionsTestUtil.createArtifact(trivialRoot, buildFilePath); - pythonSourcePath = scratch.file("trivial/trivial.py", - "#!/usr/bin/python \n print 'Hello World'"); - pythonSourceFile = ActionsTestUtil.createArtifact(trivialRoot, pythonSourcePath); + Path pythonSourcePath = + scratch.file("trivial/trivial.py", "#!/usr/bin/python \n print 'Hello World'"); + Artifact pythonSourceFile = ActionsTestUtil.createArtifact(trivialRoot, pythonSourcePath); fakeManifest.put(buildFilePath.relativeTo(rootDirectory), buildFile); fakeManifest.put(pythonSourcePath.relativeTo(rootDirectory), pythonSourceFile); ArtifactRoot outputDir = ArtifactRoot.asDerivedRoot(rootDirectory, RootType.Output, "blaze-output"); outputDir.getRoot().asPath().createDirectoryAndParents(); - manifestOutputPath = rootDirectory.getRelative("blaze-output/trivial.runfiles_manifest"); - manifestOutputFile = ActionsTestUtil.createArtifact(outputDir, manifestOutputPath); + manifestOutputFile = + ActionsTestUtil.createArtifact( + outputDir, rootDirectory.getRelative("blaze-output/trivial.runfiles_manifest")); Path relativeSymlinkPath = outputDir.getRoot().asPath().getChild("relative_symlink"); relativeSymlinkPath.createSymbolicLink(PathFragment.create("../some/relative/path")); @@ -122,28 +115,29 @@ private SourceManifestAction createAction(ManifestType type, boolean addInitPy) return new SourceManifestAction(type, NULL_ACTION_OWNER, manifestOutputFile, builder.build()); } - /** - * Manifest writer that validates an expected call sequence. - */ - private class MockManifestWriter implements SourceManifestAction.ManifestWriter { - private List> expectedSequence = new ArrayList<>(); + /** Manifest writer that validates an expected call sequence. */ + private final class MockManifestWriter implements SourceManifestAction.ManifestWriter { + private final List> expectedSequence = new ArrayList<>(); - public MockManifestWriter() { + MockManifestWriter() { expectedSequence.addAll(fakeManifest.entrySet()); } @Override - public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, - @Nullable Artifact symlink) throws IOException { - assertWithMessage("Expected manifest input to be exhausted").that(expectedSequence) + public void writeEntry( + Writer manifestWriter, + PathFragment rootRelativePath, + @Nullable PathFragment symlinkTarget) { + assertWithMessage("Expected manifest input to be exhausted") + .that(expectedSequence) .isNotEmpty(); Map.Entry expectedEntry = expectedSequence.remove(0); assertThat(rootRelativePath) .isEqualTo(PathFragment.create("TESTING").getRelative(expectedEntry.getKey())); - assertThat(symlink).isEqualTo(expectedEntry.getValue()); + assertThat(symlinkTarget).isEqualTo(expectedEntry.getValue().getPath().asFragment()); } - public int unconsumedInputs() { + int unconsumedInputs() { return expectedSequence.size(); } @@ -190,9 +184,11 @@ public void testSimpleFileWriting() throws Exception { String manifestContents = createSymlinkAction().getFileContents(reporter); assertThat(manifestContents) .isEqualTo( - "TESTING/trivial/BUILD /workspace/trivial/BUILD\n" - + "TESTING/trivial/__init__.py \n" - + "TESTING/trivial/trivial.py /workspace/trivial/trivial.py\n"); + """ + TESTING/trivial/BUILD /workspace/trivial/BUILD + TESTING/trivial/__init__.py\s + TESTING/trivial/trivial.py /workspace/trivial/trivial.py + """); } /** @@ -204,9 +200,11 @@ public void testSourceOnlyFormatting() throws Exception { String manifestContents = createSourceOnlyAction().getFileContents(reporter); assertThat(manifestContents) .isEqualTo( - "TESTING/trivial/BUILD\n" - + "TESTING/trivial/__init__.py\n" - + "TESTING/trivial/trivial.py\n"); + """ + TESTING/trivial/BUILD + TESTING/trivial/__init__.py + TESTING/trivial/trivial.py + """); } /** @@ -238,7 +236,7 @@ public void testNoPythonOrSwigLibrariesDoNotTriggerInitDotPyInclusion() throws E } @Test - public void testGetMnemonic() throws Exception { + public void testGetMnemonic() { assertThat(createSymlinkAction().getMnemonic()).isEqualTo("SourceSymlinkManifest"); assertThat(createAction(ManifestType.SOURCE_SYMLINKS, false).getMnemonic()) .isEqualTo("SourceSymlinkManifest"); @@ -246,7 +244,7 @@ public void testGetMnemonic() throws Exception { } @Test - public void testSymlinkProgressMessage() throws Exception { + public void testSymlinkProgressMessage() { String progress = createSymlinkAction().getProgressMessage(); assertWithMessage("null action not found in " + progress) .that(progress.contains("//null/action:owner")) @@ -254,7 +252,7 @@ public void testSymlinkProgressMessage() throws Exception { } @Test - public void testSymlinkProgressMessageNoPyInitFiles() throws Exception { + public void testSymlinkProgressMessageNoPyInitFiles() { String progress = createAction(ManifestType.SOURCE_SYMLINKS, false).getProgressMessage(); assertWithMessage("null action not found in " + progress) .that(progress.contains("//null/action:owner")) @@ -262,7 +260,7 @@ public void testSymlinkProgressMessageNoPyInitFiles() throws Exception { } @Test - public void testSourceOnlyProgressMessage() throws Exception { + public void testSourceOnlyProgressMessage() { SourceManifestAction action = new SourceManifestAction( ManifestType.SOURCES_ONLY, @@ -276,7 +274,7 @@ public void testSourceOnlyProgressMessage() throws Exception { } @Test - public void testRootSymlinksAffectKey() throws Exception { + public void testRootSymlinksAffectKey() { Artifact manifest1 = getBinArtifactWithNoOwner("manifest1"); Artifact manifest2 = getBinArtifactWithNoOwner("manifest2"); @@ -303,7 +301,7 @@ public void testRootSymlinksAffectKey() throws Exception { // Regression test for b/116254698. @Test - public void testEmptyFilesAffectKey() throws Exception { + public void testEmptyFilesAffectKey() { Artifact manifest1 = getBinArtifactWithNoOwner("manifest1"); Artifact manifest2 = getBinArtifactWithNoOwner("manifest2"); @@ -382,15 +380,16 @@ public void testUnresolvedSymlink() throws Exception { assertThat(action.getFileContents(reporter)) .isEqualTo( - "TESTING/BUILD /workspace/trivial/BUILD\n" - + "TESTING/absolute_symlink /absolute/path\n" - + "TESTING/relative_symlink ../some/relative/path\n"); + """ + TESTING/BUILD /workspace/trivial/BUILD + TESTING/absolute_symlink /absolute/path + TESTING/relative_symlink ../some/relative/path + """); } - private String computeKey(SourceManifestAction action) - throws CommandLineExpansionException, InterruptedException { + private String computeKey(SourceManifestAction action) { Fingerprint fp = new Fingerprint(); - action.computeKey(actionKeyContext, /*artifactExpander=*/ null, fp); + action.computeKey(actionKeyContext, /* artifactExpander= */ null, fp); return fp.hexDigestAndReset(); } }