Skip to content

Commit

Permalink
Refactor ManifestWriter to accept PathFragment instead of `Artifa…
Browse files Browse the repository at this point in the history 
…ct`.

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
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Jun 20, 2024
1 parent ce96a4c commit f311efc
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 56 deletions.
34 changes: 23 additions & 11 deletions src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()} */
Expand Down Expand Up @@ -235,7 +236,16 @@ private void writeFile(OutputStream out, Map<PathFragment, Artifact> output) thr
List<Map.Entry<PathFragment, Artifact>> sortedManifest = new ArrayList<>(output.entrySet());
sortedManifest.sort(ENTRY_COMPARATOR);
for (Map.Entry<PathFragment, Artifact> 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();
Expand Down Expand Up @@ -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');
}
Expand Down Expand Up @@ -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');
Expand Down
89 changes: 44 additions & 45 deletions src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -54,38 +52,33 @@
public final class SourceManifestActionTest extends BuildViewTestCase {

private Map<PathFragment, Artifact> 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"));
Expand Down Expand Up @@ -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<Map.Entry<PathFragment, Artifact>> expectedSequence = new ArrayList<>();
/** Manifest writer that validates an expected call sequence. */
private final class MockManifestWriter implements SourceManifestAction.ManifestWriter {
private final List<Map.Entry<PathFragment, Artifact>> 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<PathFragment, Artifact> 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();
}

Expand Down Expand Up @@ -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
""");
}

/**
Expand All @@ -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
""");
}

/**
Expand Down Expand Up @@ -238,31 +236,31 @@ 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");
assertThat(createSourceOnlyAction().getMnemonic()).isEqualTo("PackagingSourcesManifest");
}

@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"))
.isTrue();
}

@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"))
.isTrue();
}

@Test
public void testSourceOnlyProgressMessage() throws Exception {
public void testSourceOnlyProgressMessage() {
SourceManifestAction action =
new SourceManifestAction(
ManifestType.SOURCES_ONLY,
Expand All @@ -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");

Expand All @@ -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");

Expand Down Expand Up @@ -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();
}
}

0 comments on commit f311efc

Please sign in to comment.