diff options
Diffstat (limited to 'src')
3 files changed, 68 insertions, 20 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java index 160c2ae4fc..dfb4bdee98 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceTransport.java @@ -25,7 +25,6 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -65,6 +64,8 @@ import io.grpc.StatusException; import java.time.Duration; import java.util.Collection; import java.util.Deque; +import java.util.HashMap; +import java.util.Map; import java.util.Set; import java.util.concurrent.BlockingDeque; import java.util.concurrent.Callable; @@ -343,12 +344,14 @@ public class BuildEventServiceTransport implements BuildEventTransport { } Collection<LocalFile> localFiles = event.referencedLocalFiles(); - ImmutableMap.Builder<Path, LocalFile> localFileMap = - ImmutableMap.builderWithExpectedSize(localFiles.size()); + Map<Path, LocalFile> localFileMap = new HashMap<>(localFiles.size()); for (LocalFile localFile : localFiles) { - localFileMap.put(localFile.path, localFile); + // It is possible for targets to have duplicate artifacts (same path but different owners) + // in their output groups. Since they didn't trigger an artifact conflict they are the + // same file, so just skip either one + localFileMap.putIfAbsent(localFile.path, localFile); } - ListenableFuture<PathConverter> upload = artifactUploader.upload(localFileMap.build()); + ListenableFuture<PathConverter> upload = artifactUploader.upload(localFileMap); InternalOrderedBuildEvent buildEvent = new DefaultInternalOrderedBuildEvent( event, namer, upload, besProtoUtil.nextSequenceNumber(), timestamp()); diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java index e8391a37d9..24689f7073 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java @@ -20,7 +20,6 @@ import static java.lang.String.format; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Throwables; -import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -48,6 +47,8 @@ import java.io.OutputStream; import java.time.Duration; import java.time.Instant; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.BlockingQueue; import java.util.concurrent.LinkedBlockingDeque; import java.util.concurrent.TimeUnit; @@ -254,12 +255,14 @@ abstract class FileTransport implements BuildEventTransport { */ private ListenableFuture<PathConverter> uploadReferencedFiles(Collection<LocalFile> localFiles) { checkNotNull(localFiles); - ImmutableMap.Builder<Path, LocalFile> localFileMap = - ImmutableMap.builderWithExpectedSize(localFiles.size()); + Map<Path, LocalFile> localFileMap = new HashMap<>(localFiles.size()); for (LocalFile localFile : localFiles) { - localFileMap.put(localFile.path, localFile); + // It is possible for targets to have duplicate artifacts (same path but different owners) + // in their output groups. Since they didn't trigger an artifact conflict they are the + // same file, so just skip either one + localFileMap.putIfAbsent(localFile.path, localFile); } - ListenableFuture<PathConverter> upload = uploader.upload(localFileMap.build()); + ListenableFuture<PathConverter> upload = uploader.upload(localFileMap); Futures.addCallback( upload, new FutureCallback<PathConverter>() { diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java index 7bc077e8ac..aba22b48f5 100644 --- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java @@ -14,11 +14,13 @@ package com.google.devtools.build.lib.buildeventstream.transports; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader.LOCAL_FILES_UPLOADER; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -43,7 +45,6 @@ import java.io.File; import java.io.FileInputStream; import java.io.InputStream; import java.util.Collection; -import java.util.Collections; import java.util.Map; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; @@ -206,8 +207,8 @@ public class BinaryFormatFileTransportTest { when(file1.getBaseName()).thenReturn("file1"); Path file2 = Mockito.mock(Path.class); when(file2.getBaseName()).thenReturn("file2"); - BuildEvent event1 = new WithLocalFileEvent(file1); - BuildEvent event2 = new WithLocalFileEvent(file2); + BuildEvent event1 = new WithLocalFilesEvent(ImmutableList.of(file1)); + BuildEvent event2 = new WithLocalFilesEvent(ImmutableList.of(file2)); BuildEventArtifactUploader uploader = Mockito.spy(new BuildEventArtifactUploader() { @Override @@ -243,13 +244,47 @@ public class BinaryFormatFileTransportTest { verify(uploader).shutdown(); } + /** Regression test for b/207287675 */ + @Test + public void testHandlesDuplicateFiles() throws Exception { + Path file1 = Mockito.mock(Path.class); + when(file1.getBaseName()).thenReturn("foo"); + BuildEvent event1 = new WithLocalFilesEvent(ImmutableList.of(file1, file1)); + + BuildEventArtifactUploader uploader = + Mockito.spy( + new BuildEventArtifactUploader() { + @Override + public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) { + return Futures.immediateFuture(new FileUriPathConverter()); + } + + @Override + public void shutdown() { + // Intentionally left empty. + } + }); + File output = tmp.newFile(); + BinaryFormatFileTransport transport = + new BinaryFormatFileTransport(output.getAbsolutePath(), defaultOpts, uploader, (e) -> {}); + transport.sendBuildEvent(event1, artifactGroupNamer); + transport.close().get(); + + assertThat(transport.writer.pendingWrites).isEmpty(); + try (InputStream in = new FileInputStream(output)) { + assertThat(BuildEventStreamProtos.BuildEvent.parseDelimitedFrom(in)) + .isEqualTo(event1.asStreamProto(null)); + assertThat(in.available()).isEqualTo(0); + } + } + @Test public void testCloseWaitsForWritesToFinish() throws Exception { // Test that .close() waits for all writes to finish. Path file1 = Mockito.mock(Path.class); when(file1.getBaseName()).thenReturn("file1"); - BuildEvent event = new WithLocalFileEvent(file1); + BuildEvent event = new WithLocalFilesEvent(ImmutableList.of(file1)); SettableFuture<PathConverter> upload = SettableFuture.create(); BuildEventArtifactUploader uploader = Mockito.spy(new BuildEventArtifactUploader() { @@ -283,18 +318,21 @@ public class BinaryFormatFileTransportTest { verify(uploader).shutdown(); } - private static class WithLocalFileEvent implements BuildEvent { + private static class WithLocalFilesEvent implements BuildEvent { int id; - Path file; + ImmutableList<Path> files; - WithLocalFileEvent(Path file) { - this.file = file; + WithLocalFilesEvent(ImmutableList<Path> files) { + this.files = files; } @Override public Collection<LocalFile> referencedLocalFiles() { - return Collections.singleton(new LocalFile(file, LocalFileType.OUTPUT)); + return files + .stream() + .map(f -> new LocalFile(f, LocalFileType.OUTPUT)) + .collect(toImmutableList()); } @Override @@ -303,7 +341,11 @@ public class BinaryFormatFileTransportTest { .setId(BuildEventId.progressId(id).asStreamProto()) .setProgress( BuildEventStreamProtos.Progress.newBuilder() - .setStdout("basename: " + file.getBaseName()) + .setStdout( + "uploading: " + + Joiner.on(", ") + .join( + files.stream().map(Path::getBaseName).collect(toImmutableList()))) .build()) .build(); } |