From d90b96800579cfde9fc00434f1555391afb340b0 Mon Sep 17 00:00:00 2001
From: Colin Alworth <colinalworth@deephaven.io>
Date: Tue, 24 Oct 2023 09:46:11 -0500
Subject: [PATCH] Fix intermittent build failure, ensuring docker test output
 is still usable (#4692)

This PR reverts #4254, and rewrites the changes to now only support the
use case that it was written for, but also avoid the gradle artifact
issue that it appears to have introduced. The patch works by both using
dependsOn and finalizedBy to set up two different downstream Sync tasks
that do the same thing - but only one runs on success, and only one
runs on failure.

Fixes #4691
---
 buildSrc/src/main/groovy/Docker.groovy        | 44 +++++++++++++------
 .../tools/docker/CombinedDockerRunTask.groovy |  1 -
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/buildSrc/src/main/groovy/Docker.groovy b/buildSrc/src/main/groovy/Docker.groovy
index e9cabd94bed..c1dfb6c7863 100644
--- a/buildSrc/src/main/groovy/Docker.groovy
+++ b/buildSrc/src/main/groovy/Docker.groovy
@@ -302,19 +302,8 @@ class Docker {
             // Single task with explicit inputs and outputs, to let gradle detect if it is up to date, and let docker
             // cache what it can.
 
-            // Sync outputs to the desired location
-            def syncAfterBuildAndRun = project.tasks.register("${taskName}Sync", Sync) { sync ->
-                sync.with {
-                    // run the provided closure first
-                    cfg.copyOut.execute(sync)
-
-                    // then set the from location
-                    from dockerCopyLocation
-                }
-            }
-
             // Note that if "showLogsOnSuccess" is true, we don't run this way, since that would omit logs when cached.
-            return project.tasks.register(taskName, CombinedDockerRunTask) { cacheableDockerTask ->
+            def buildAndRun = project.tasks.register("${taskName}Run", CombinedDockerRunTask) { cacheableDockerTask ->
                 cacheableDockerTask.with {
                     // mark inputs, depend on dockerfile task and input sync task
                     inputs.files(makeImage.get().outputs.files)
@@ -345,7 +334,36 @@ class Docker {
 
                     remotePath.set(cfg.containerOutPath)
                     outputDir.set(project.file(dockerCopyLocation))
-                    finalizedBy syncAfterBuildAndRun
+                }
+            }
+
+            // Handle copying failure. This is now distinct from the "actual" Sync task that depends directly
+            // on the CombinedDockerRunTask.
+            def syncAfterFail = project.tasks.register("${taskName}SyncAfterFail", Sync) { sync ->
+                sync.with {
+                    // run the provided closure first
+                    cfg.copyOut.execute(sync)
+
+                    // then set the from location
+                    from dockerCopyLocation
+
+                    onlyIf { buildAndRun.get().state.failure != null }
+                }
+            }
+            buildAndRun.configure {t ->
+                t.finalizedBy syncAfterFail
+            }
+
+            // Sync outputs to the desired location
+            return project.tasks.register(taskName, Sync) { sync ->
+                sync.with {
+                    dependsOn buildAndRun
+
+                    // run the provided closure first
+                    cfg.copyOut.execute(sync)
+
+                    // then set the from location
+                    from dockerCopyLocation
                 }
             }
         }
diff --git a/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy b/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy
index 64d8250e8ac..5a812a0fc01 100644
--- a/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy
+++ b/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy
@@ -111,7 +111,6 @@ class CombinedDockerRunTask extends AbstractDockerRemoteApiTask {
                     throw new GradleException("${failedMessage}, check logs for details")
                 }
             }
-
         } finally {
             if (containerId == null) {
                 return;