Skip to content

Commit f311efc

Browse files
justinhorvitzcopybara-github
justinhorvitz
authored and
copybara-github
committed
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
1 parent ce96a4c commit f311efc

2 files changed

Lines changed: 67 additions & 56 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,11 @@ interface ManifestWriter {
7676
*
7777
* @param manifestWriter the output stream
7878
* @param rootRelativePath path of an entry relative to the manifest's root
79-
* @param symlink (optional) symlink that resolves the above path
79+
* @param symlinkTarget target of the entry at {@code rootRelativePath} if it is a symlink,
80+
* otherwise {@code null}
8081
*/
8182
void writeEntry(
82-
Writer manifestWriter, PathFragment rootRelativePath, @Nullable Artifact symlink)
83+
Writer manifestWriter, PathFragment rootRelativePath, @Nullable PathFragment symlinkTarget)
8384
throws IOException;
8485

8586
/** Fulfills {@link com.google.devtools.build.lib.actions.AbstractAction#getMnemonic()} */
@@ -235,7 +236,16 @@ private void writeFile(OutputStream out, Map<PathFragment, Artifact> output) thr
235236
List<Map.Entry<PathFragment, Artifact>> sortedManifest = new ArrayList<>(output.entrySet());
236237
sortedManifest.sort(ENTRY_COMPARATOR);
237238
for (Map.Entry<PathFragment, Artifact> line : sortedManifest) {
238-
manifestWriter.writeEntry(manifestFile, line.getKey(), line.getValue());
239+
Artifact artifact = line.getValue();
240+
PathFragment symlinkTarget;
241+
if (artifact == null) {
242+
symlinkTarget = null;
243+
} else if (artifact.isSymlink()) {
244+
symlinkTarget = artifact.getPath().readSymbolicLink();
245+
} else {
246+
symlinkTarget = artifact.getPath().asFragment();
247+
}
248+
manifestWriter.writeEntry(manifestFile, line.getKey(), symlinkTarget);
239249
}
240250

241251
manifestFile.flush();
@@ -287,17 +297,16 @@ public enum ManifestType implements ManifestWriter {
287297
*/
288298
SOURCE_SYMLINKS {
289299
@Override
290-
public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink)
300+
public void writeEntry(
301+
Writer manifestWriter,
302+
PathFragment rootRelativePath,
303+
@Nullable PathFragment symlinkTarget)
291304
throws IOException {
292305
manifestWriter.append(rootRelativePath.getPathString());
293306
// This trailing whitespace is REQUIRED to process the single entry line correctly.
294307
manifestWriter.append(' ');
295-
if (symlink != null) {
296-
if (symlink.isSymlink()) {
297-
manifestWriter.append(symlink.getPath().readSymbolicLink().getPathString());
298-
} else {
299-
manifestWriter.append(symlink.getPath().getPathString());
300-
}
308+
if (symlinkTarget != null) {
309+
manifestWriter.append(symlinkTarget.getPathString());
301310
}
302311
manifestWriter.append('\n');
303312
}
@@ -335,7 +344,10 @@ public boolean emitsAbsolutePaths() {
335344
*/
336345
SOURCES_ONLY {
337346
@Override
338-
public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Artifact symlink)
347+
public void writeEntry(
348+
Writer manifestWriter,
349+
PathFragment rootRelativePath,
350+
@Nullable PathFragment symlinkTarget)
339351
throws IOException {
340352
manifestWriter.append(rootRelativePath.getPathString());
341353
manifestWriter.append('\n');

‎src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java‎

Lines changed: 44 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
2626
import com.google.devtools.build.lib.actions.ArtifactRoot;
2727
import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
28-
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
2928
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
3029
import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType;
3130
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
@@ -34,7 +33,6 @@
3433
import com.google.devtools.build.lib.vfs.Path;
3534
import com.google.devtools.build.lib.vfs.PathFragment;
3635
import com.google.devtools.build.lib.vfs.Root;
37-
import java.io.IOException;
3836
import java.io.Writer;
3937
import java.util.ArrayList;
4038
import java.util.LinkedHashMap;
@@ -54,38 +52,33 @@
5452
public final class SourceManifestActionTest extends BuildViewTestCase {
5553

5654
private Map<PathFragment, Artifact> fakeManifest;
57-
58-
private Path pythonSourcePath;
59-
private Artifact pythonSourceFile;
60-
private Path buildFilePath;
6155
private Artifact buildFile;
6256
private Artifact relativeSymlink;
6357
private Artifact absoluteSymlink;
64-
65-
private Path manifestOutputPath;
6658
private Artifact manifestOutputFile;
6759

6860
@Before
69-
public final void createFiles() throws Exception {
61+
public void createFiles() throws Exception {
7062
analysisMock.pySupport().setup(mockToolsConfig);
7163
// Test with a raw manifest Action.
7264
fakeManifest = new LinkedHashMap<>();
7365
ArtifactRoot trivialRoot =
7466
ArtifactRoot.asSourceRoot(Root.fromPath(rootDirectory.getRelative("trivial")));
75-
buildFilePath = scratch.file("trivial/BUILD",
76-
"py_binary(name='trivial', srcs =['trivial.py'])");
67+
Path buildFilePath =
68+
scratch.file("trivial/BUILD", "py_binary(name='trivial', srcs =['trivial.py'])");
7769
buildFile = ActionsTestUtil.createArtifact(trivialRoot, buildFilePath);
7870

79-
pythonSourcePath = scratch.file("trivial/trivial.py",
80-
"#!/usr/bin/python \n print 'Hello World'");
81-
pythonSourceFile = ActionsTestUtil.createArtifact(trivialRoot, pythonSourcePath);
71+
Path pythonSourcePath =
72+
scratch.file("trivial/trivial.py", "#!/usr/bin/python \n print 'Hello World'");
73+
Artifact pythonSourceFile = ActionsTestUtil.createArtifact(trivialRoot, pythonSourcePath);
8274
fakeManifest.put(buildFilePath.relativeTo(rootDirectory), buildFile);
8375
fakeManifest.put(pythonSourcePath.relativeTo(rootDirectory), pythonSourceFile);
8476
ArtifactRoot outputDir =
8577
ArtifactRoot.asDerivedRoot(rootDirectory, RootType.Output, "blaze-output");
8678
outputDir.getRoot().asPath().createDirectoryAndParents();
87-
manifestOutputPath = rootDirectory.getRelative("blaze-output/trivial.runfiles_manifest");
88-
manifestOutputFile = ActionsTestUtil.createArtifact(outputDir, manifestOutputPath);
79+
manifestOutputFile =
80+
ActionsTestUtil.createArtifact(
81+
outputDir, rootDirectory.getRelative("blaze-output/trivial.runfiles_manifest"));
8982

9083
Path relativeSymlinkPath = outputDir.getRoot().asPath().getChild("relative_symlink");
9184
relativeSymlinkPath.createSymbolicLink(PathFragment.create("../some/relative/path"));
@@ -122,28 +115,29 @@ private SourceManifestAction createAction(ManifestType type, boolean addInitPy)
122115
return new SourceManifestAction(type, NULL_ACTION_OWNER, manifestOutputFile, builder.build());
123116
}
124117

125-
/**
126-
* Manifest writer that validates an expected call sequence.
127-
*/
128-
private class MockManifestWriter implements SourceManifestAction.ManifestWriter {
129-
private List<Map.Entry<PathFragment, Artifact>> expectedSequence = new ArrayList<>();
118+
/** Manifest writer that validates an expected call sequence. */
119+
private final class MockManifestWriter implements SourceManifestAction.ManifestWriter {
120+
private final List<Map.Entry<PathFragment, Artifact>> expectedSequence = new ArrayList<>();
130121

131-
public MockManifestWriter() {
122+
MockManifestWriter() {
132123
expectedSequence.addAll(fakeManifest.entrySet());
133124
}
134125

135126
@Override
136-
public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath,
137-
@Nullable Artifact symlink) throws IOException {
138-
assertWithMessage("Expected manifest input to be exhausted").that(expectedSequence)
127+
public void writeEntry(
128+
Writer manifestWriter,
129+
PathFragment rootRelativePath,
130+
@Nullable PathFragment symlinkTarget) {
131+
assertWithMessage("Expected manifest input to be exhausted")
132+
.that(expectedSequence)
139133
.isNotEmpty();
140134
Map.Entry<PathFragment, Artifact> expectedEntry = expectedSequence.remove(0);
141135
assertThat(rootRelativePath)
142136
.isEqualTo(PathFragment.create("TESTING").getRelative(expectedEntry.getKey()));
143-
assertThat(symlink).isEqualTo(expectedEntry.getValue());
137+
assertThat(symlinkTarget).isEqualTo(expectedEntry.getValue().getPath().asFragment());
144138
}
145139

146-
public int unconsumedInputs() {
140+
int unconsumedInputs() {
147141
return expectedSequence.size();
148142
}
149143

@@ -190,9 +184,11 @@ public void testSimpleFileWriting() throws Exception {
190184
String manifestContents = createSymlinkAction().getFileContents(reporter);
191185
assertThat(manifestContents)
192186
.isEqualTo(
193-
"TESTING/trivial/BUILD /workspace/trivial/BUILD\n"
194-
+ "TESTING/trivial/__init__.py \n"
195-
+ "TESTING/trivial/trivial.py /workspace/trivial/trivial.py\n");
187+
"""
188+
TESTING/trivial/BUILD /workspace/trivial/BUILD
189+
TESTING/trivial/__init__.py\s
190+
TESTING/trivial/trivial.py /workspace/trivial/trivial.py
191+
""");
196192
}
197193

198194
/**
@@ -204,9 +200,11 @@ public void testSourceOnlyFormatting() throws Exception {
204200
String manifestContents = createSourceOnlyAction().getFileContents(reporter);
205201
assertThat(manifestContents)
206202
.isEqualTo(
207-
"TESTING/trivial/BUILD\n"
208-
+ "TESTING/trivial/__init__.py\n"
209-
+ "TESTING/trivial/trivial.py\n");
203+
"""
204+
TESTING/trivial/BUILD
205+
TESTING/trivial/__init__.py
206+
TESTING/trivial/trivial.py
207+
""");
210208
}
211209

212210
/**
@@ -238,31 +236,31 @@ public void testNoPythonOrSwigLibrariesDoNotTriggerInitDotPyInclusion() throws E
238236
}
239237

240238
@Test
241-
public void testGetMnemonic() throws Exception {
239+
public void testGetMnemonic() {
242240
assertThat(createSymlinkAction().getMnemonic()).isEqualTo("SourceSymlinkManifest");
243241
assertThat(createAction(ManifestType.SOURCE_SYMLINKS, false).getMnemonic())
244242
.isEqualTo("SourceSymlinkManifest");
245243
assertThat(createSourceOnlyAction().getMnemonic()).isEqualTo("PackagingSourcesManifest");
246244
}
247245

248246
@Test
249-
public void testSymlinkProgressMessage() throws Exception {
247+
public void testSymlinkProgressMessage() {
250248
String progress = createSymlinkAction().getProgressMessage();
251249
assertWithMessage("null action not found in " + progress)
252250
.that(progress.contains("//null/action:owner"))
253251
.isTrue();
254252
}
255253

256254
@Test
257-
public void testSymlinkProgressMessageNoPyInitFiles() throws Exception {
255+
public void testSymlinkProgressMessageNoPyInitFiles() {
258256
String progress = createAction(ManifestType.SOURCE_SYMLINKS, false).getProgressMessage();
259257
assertWithMessage("null action not found in " + progress)
260258
.that(progress.contains("//null/action:owner"))
261259
.isTrue();
262260
}
263261

264262
@Test
265-
public void testSourceOnlyProgressMessage() throws Exception {
263+
public void testSourceOnlyProgressMessage() {
266264
SourceManifestAction action =
267265
new SourceManifestAction(
268266
ManifestType.SOURCES_ONLY,
@@ -276,7 +274,7 @@ public void testSourceOnlyProgressMessage() throws Exception {
276274
}
277275

278276
@Test
279-
public void testRootSymlinksAffectKey() throws Exception {
277+
public void testRootSymlinksAffectKey() {
280278
Artifact manifest1 = getBinArtifactWithNoOwner("manifest1");
281279
Artifact manifest2 = getBinArtifactWithNoOwner("manifest2");
282280

@@ -303,7 +301,7 @@ public void testRootSymlinksAffectKey() throws Exception {
303301

304302
// Regression test for b/116254698.
305303
@Test
306-
public void testEmptyFilesAffectKey() throws Exception {
304+
public void testEmptyFilesAffectKey() {
307305
Artifact manifest1 = getBinArtifactWithNoOwner("manifest1");
308306
Artifact manifest2 = getBinArtifactWithNoOwner("manifest2");
309307

@@ -382,15 +380,16 @@ public void testUnresolvedSymlink() throws Exception {
382380

383381
assertThat(action.getFileContents(reporter))
384382
.isEqualTo(
385-
"TESTING/BUILD /workspace/trivial/BUILD\n"
386-
+ "TESTING/absolute_symlink /absolute/path\n"
387-
+ "TESTING/relative_symlink ../some/relative/path\n");
383+
"""
384+
TESTING/BUILD /workspace/trivial/BUILD
385+
TESTING/absolute_symlink /absolute/path
386+
TESTING/relative_symlink ../some/relative/path
387+
""");
388388
}
389389

390-
private String computeKey(SourceManifestAction action)
391-
throws CommandLineExpansionException, InterruptedException {
390+
private String computeKey(SourceManifestAction action) {
392391
Fingerprint fp = new Fingerprint();
393-
action.computeKey(actionKeyContext, /*artifactExpander=*/ null, fp);
392+
action.computeKey(actionKeyContext, /* artifactExpander= */ null, fp);
394393
return fp.hexDigestAndReset();
395394
}
396395
}

0 commit comments

Comments
 (0)