From e8c8cedfc25e1285e557ed416f357cf10cfe5fd6 Mon Sep 17 00:00:00 2001 From: Cristian Ferretti Date: Mon, 31 Jul 2023 21:52:58 -0400 Subject: [PATCH 1/4] Ensure the buildAndRun task runs the sync task even if it fails. --- buildSrc/src/main/groovy/Docker.groovy | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/buildSrc/src/main/groovy/Docker.groovy b/buildSrc/src/main/groovy/Docker.groovy index 3d1a2f77c7e..3c0ec2cc62e 100644 --- a/buildSrc/src/main/groovy/Docker.groovy +++ b/buildSrc/src/main/groovy/Docker.groovy @@ -302,6 +302,17 @@ 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. def buildAndRun = project.tasks.register("${taskName}Run", CombinedDockerRunTask) { cacheableDockerTask -> cacheableDockerTask.with { @@ -334,18 +345,14 @@ class Docker { remotePath.set(cfg.containerOutPath) outputDir.set(project.file(dockerCopyLocation)) + finalizedBy syncAfterBuildAndRun } } - // 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 + return project.tasks.register(taskName) { task -> + task.with { + dependsOn buildAndRun + outputs.files syncAfterBuildAndRun.get().outputs.files } } } From 891ff58b0c1d09735b8aba71a06d44d3b66bd278 Mon Sep 17 00:00:00 2001 From: Cristian Ferretti Date: Wed, 2 Aug 2023 01:45:05 -0400 Subject: [PATCH 2/4] Followup to review comment. --- buildSrc/src/main/groovy/Docker.groovy | 9 ++------- cpp-client/build.gradle | 2 +- go/build.gradle | 2 +- py/client/build.gradle | 8 +++++--- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/buildSrc/src/main/groovy/Docker.groovy b/buildSrc/src/main/groovy/Docker.groovy index 3c0ec2cc62e..ff2fb9fcadc 100644 --- a/buildSrc/src/main/groovy/Docker.groovy +++ b/buildSrc/src/main/groovy/Docker.groovy @@ -314,7 +314,7 @@ class Docker { } // Note that if "showLogsOnSuccess" is true, we don't run this way, since that would omit logs when cached. - def buildAndRun = project.tasks.register("${taskName}Run", CombinedDockerRunTask) { cacheableDockerTask -> + def buildAndRun = project.tasks.register(taskName, CombinedDockerRunTask) { cacheableDockerTask -> cacheableDockerTask.with { // mark inputs, depend on dockerfile task and input sync task inputs.files(makeImage.get().outputs.files) @@ -349,12 +349,7 @@ class Docker { } } - return project.tasks.register(taskName) { task -> - task.with { - dependsOn buildAndRun - outputs.files syncAfterBuildAndRun.get().outputs.files - } - } + return buildAndRun } // With no outputs, we can use the standard individual containers, and gradle will have to re-run each time // the task is invoked, can never be marked as up to date. diff --git a/cpp-client/build.gradle b/cpp-client/build.gradle index d1ee54b4ee8..ab6bf7cf670 100644 --- a/cpp-client/build.gradle +++ b/cpp-client/build.gradle @@ -124,5 +124,5 @@ def testCppClient = Docker.registerDockerTask(project, 'testCppClient') { parentContainers = [ Docker.registryTask(project, 'cpp-client-base') ] entrypoint = ['/cpp-client/install/bin/cpp-tests-to-junit.sh', '/out/cpp-test.xml', '/out/cpp-test.log'] } -deephavenDocker.shouldLogIfTaskFails tasks.named('testCppClientRun') +deephavenDocker.shouldLogIfTaskFails testCppClient tasks.check.dependsOn(testCppClient) diff --git a/go/build.gradle b/go/build.gradle index 1231e46622b..a1baf10f6c2 100644 --- a/go/build.gradle +++ b/go/build.gradle @@ -86,5 +86,5 @@ def testGoClient = Docker.registerDockerTask(project, 'testGoClient') { parentContainers = [ Docker.registryTask(project, 'go') ] entrypoint = ['./go-test-to-junit.sh', '/out/go-test.xml', '/out/go.log'] } -deephavenDocker.shouldLogIfTaskFails tasks.named('testGoClientRun') +deephavenDocker.shouldLogIfTaskFails testGoClient tasks.check.dependsOn(testGoClient) diff --git a/py/client/build.gradle b/py/client/build.gradle index 7c2ec933f85..589c006e8e2 100644 --- a/py/client/build.gradle +++ b/py/client/build.gradle @@ -81,7 +81,7 @@ deephavenDocker { networkName.set "pydeephaven-network-${randomSuffix}" } -tasks.getByName('check').dependsOn(Docker.registerDockerTask(project, 'testPyClient') { +def testPyClient = Docker.registerDockerTask(project, 'testPyClient') { copyIn { from('pydeephaven') { into 'project/pydeephaven' @@ -112,5 +112,7 @@ tasks.getByName('check').dependsOn(Docker.registerDockerTask(project, 'testPyCli copyOut { into layout.buildDirectory.dir('test-results') } -}) -deephavenDocker.shouldLogIfTaskFails tasks.named('testPyClientRun') \ No newline at end of file +} + +tasks.getByName('check').dependsOn(testPyClient) +deephavenDocker.shouldLogIfTaskFails testPyClient From 9580c278e7a30d239385d078c553c2ec2982dd62 Mon Sep 17 00:00:00 2001 From: Cristian Ferretti Date: Wed, 2 Aug 2023 14:58:32 -0400 Subject: [PATCH 3/4] Recover previous fix. --- .../deephaven/tools/docker/CombinedDockerRunTask.groovy | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 bc0322bc60f..6267c6ff978 100644 --- a/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy +++ b/buildSrc/src/main/groovy/io/deephaven/tools/docker/CombinedDockerRunTask.groovy @@ -112,6 +112,10 @@ class CombinedDockerRunTask extends AbstractDockerRemoteApiTask { } } + } finally { + if (containerId == null) { + return; + } // Copy output to internal output directory CopyArchiveFromContainerCmd copyCommand = dockerClient.copyArchiveFromContainerCmd(containerId, remotePath.get()) logger.quiet "Copying '${remotePath.get()}' from container with ID '${containerId}' to '${outputDir.get()}'." @@ -127,10 +131,6 @@ class CombinedDockerRunTask extends AbstractDockerRemoteApiTask { } finally { tarStream?.close() } - } finally { - if (containerId == null) { - return; - } RemoveContainerCmd removeCommand = dockerClient.removeContainerCmd(containerId) removeCommand.withRemoveVolumes(true) removeCommand.withForce(true) From 7e4aabd26452763dafd48ff9973cc0ae3b583ea9 Mon Sep 17 00:00:00 2001 From: Cristian Ferretti Date: Wed, 2 Aug 2023 15:23:51 -0400 Subject: [PATCH 4/4] Followup to review comments. --- buildSrc/src/main/groovy/Docker.groovy | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/buildSrc/src/main/groovy/Docker.groovy b/buildSrc/src/main/groovy/Docker.groovy index ff2fb9fcadc..e9cabd94bed 100644 --- a/buildSrc/src/main/groovy/Docker.groovy +++ b/buildSrc/src/main/groovy/Docker.groovy @@ -314,7 +314,7 @@ class Docker { } // Note that if "showLogsOnSuccess" is true, we don't run this way, since that would omit logs when cached. - def buildAndRun = project.tasks.register(taskName, CombinedDockerRunTask) { cacheableDockerTask -> + return project.tasks.register(taskName, CombinedDockerRunTask) { cacheableDockerTask -> cacheableDockerTask.with { // mark inputs, depend on dockerfile task and input sync task inputs.files(makeImage.get().outputs.files) @@ -348,8 +348,6 @@ class Docker { finalizedBy syncAfterBuildAndRun } } - - return buildAndRun } // With no outputs, we can use the standard individual containers, and gradle will have to re-run each time // the task is invoked, can never be marked as up to date.