From a61a8ef123c55dd8b94dc3e16c0722a59d0f8e44 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 13 Nov 2023 15:32:31 -0600 Subject: [PATCH] Server logs should be written when integration tests fail (#4804) Fixes #4766 --- buildSrc/src/main/groovy/Docker.groovy | 66 ++++++++++++++----- .../groovy/io.deephaven.python-wheel.gradle | 8 ++- proto/raw-js-openapi/build.gradle | 2 +- 3 files changed, 55 insertions(+), 21 deletions(-) diff --git a/buildSrc/src/main/groovy/Docker.groovy b/buildSrc/src/main/groovy/Docker.groovy index c1dfb6c7863..618dd9f6404 100644 --- a/buildSrc/src/main/groovy/Docker.groovy +++ b/buildSrc/src/main/groovy/Docker.groovy @@ -300,7 +300,34 @@ class Docker { if (cfg.copyOut && !cfg.showLogsOnSuccess) { // Single task with explicit inputs and outputs, to let gradle detect if it is up to date, and let docker - // cache what it can. + // cache what it can. While far more efficient for gradle to run (or rather, know when it does not need to + // run), it is also more bug-prone while we try to get various competing features working. + // + // To handle these use cases, we're not using dependsOn as we typically would do, but instead using + // finalizedBy and onlyIf. Here's a mermaid diagram: + // + // graph LR; + // MakeImage -. finalizedBy .-> Run + // Sync -- dependsOn --> MakeImage + // Run -. finalizedBy .-> Sync + // + // + // Unlike "A dependsOn B", "B finalized A" will let B run if A failed, and will not run A if B must run. + // Combining the chain of finalizedBys between MakeImage <- Run <- Sync with the dependsOn from + // Sync -> MakeImage lets us handle the following cases: + // * Successful run, output is sync'd afterwards, final task succeeds + // * Failed run, output is sync'd afterwards, final task fails + // * Failed image creation, no run, no sync, no final task + // * Previously successful run with no source changes, no tasks run (all "UP-TO-DATE") + // + // Tests to run to confirm functionality: + // * After changes, confirm that :web:assemble runs (isn't all "UP-TO-DATE") + // * Then run again with no changes, confirm all are UP-TO-DATE, roughly 2s build time + // * Edit a test that uses deephavenDocker to fail, confirm that the test fails, that the test-reports + // are copied out, and that server logs are written to console + // * Ensure that if the test is set to pass that the test-reports are copied out, and server logs are + // not written. + // Note that at this time integration tests using the deephavenDocker plugin are never UP-TO-DATE. // 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 -> @@ -337,35 +364,38 @@ class Docker { } } - // 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 -> + // Specify that makeImage is finalized by buildAndRun - that is, in this configuration buildAndRun + // must run after makeImage finishes + makeImage.configure {it -> + it.finalizedBy(buildAndRun) + } + + // Handle copying output from the docker task to the user-controlled location + def syncOutput = project.tasks.register(taskName, Sync) { sync -> sync.with { + dependsOn(makeImage) // run the provided closure first cfg.copyOut.execute(sync) // then set the from location from dockerCopyLocation - onlyIf { buildAndRun.get().state.failure != null } + doLast { + // If the actual task has already failed, we need to fail this task to signal to any downstream + // tasks to not continue. Under normal circumstances, we might just not run this Sync task at + // all to signal this, however in our case we want to copy out artifacts of failure for easier + // debugging. + if (buildAndRun.get().state.failure != null) { + throw new GradleException('Docker task failed, see earlier task failures for details') + } + } } } buildAndRun.configure {t -> - t.finalizedBy syncAfterFail + t.finalizedBy syncOutput } - // 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 syncOutput } // 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/buildSrc/src/main/groovy/io.deephaven.python-wheel.gradle b/buildSrc/src/main/groovy/io.deephaven.python-wheel.gradle index 47af3abbd3d..08b6814fe2c 100644 --- a/buildSrc/src/main/groovy/io.deephaven.python-wheel.gradle +++ b/buildSrc/src/main/groovy/io.deephaven.python-wheel.gradle @@ -34,6 +34,8 @@ configurations { } project.evaluationDependsOn(Docker.registryProject('python')) +def wheelPath = project.layout.buildDirectory.dir('wheel') + def buildWheel = Docker.registerDockerTask(project, 'buildWheel') { config -> config.copyIn { Sync sync -> // apply the extension spec, copying into src @@ -58,10 +60,12 @@ def buildWheel = Docker.registerDockerTask(project, 'buildWheel') { config -> config.parentContainers = [ Docker.registryTask(project, 'python') ] config.containerOutPath='/usr/src/app/dist' config.copyOut { Sync sync -> - sync.into "build/wheel" + sync.into wheelPath } } artifacts { - pythonWheel buildWheel + pythonWheel(wheelPath) { + builtBy buildWheel + } } diff --git a/proto/raw-js-openapi/build.gradle b/proto/raw-js-openapi/build.gradle index a9e9a9bd909..782df4ad7d3 100644 --- a/proto/raw-js-openapi/build.gradle +++ b/proto/raw-js-openapi/build.gradle @@ -15,7 +15,7 @@ dependencies { def webpackSourcesLocation = layout.buildDirectory.dir("${buildDir}/dhapi") -Docker.registerDockerTask(project, 'webpackSources') { +def webpackSources = Docker.registerDockerTask(project, 'webpackSources') { copyIn { from(configurations.js) { // note: we are only copying the JS and not TS files.