Skip to content

Commit

Permalink
Fix intermittent build failure, ensuring docker test output is still …
Browse files Browse the repository at this point in the history
…usable (deephaven#4692)

This PR reverts deephaven#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 deephaven#4691
  • Loading branch information
niloc132 committed Oct 24, 2023
1 parent 8b631ae commit d90b968
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 14 deletions.
44 changes: 31 additions & 13 deletions buildSrc/src/main/groovy/Docker.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ class CombinedDockerRunTask extends AbstractDockerRemoteApiTask {
throw new GradleException("${failedMessage}, check logs for details")
}
}

} finally {
if (containerId == null) {
return;
Expand Down

0 comments on commit d90b968

Please sign in to comment.