Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R dockerized test run gradle target and CI #4625

Merged
merged 18 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/tag-base-images.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ jobs:
if: ${{ startsWith(github.ref, 'refs/heads/release/v') }}
run: |
./docker/registry/cpp-client-base/build/crane/retag.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove cpp-client-base? Here, but also delete the registry directory and make sure nothing references it?

Copy link
Member Author

@jcferretti jcferretti Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r-base is based on cpp-client-base, so I don't think we can remove it from the base-images repo. Maybe we can remove (some?) of its references from the deephaven-core repo tho... although, the tagging here I am not sure what it does, maybe we should have a short chat on slack or VC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove the cpp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I Devin request I remove earlier in this same review. The C++ build is now using the r-client-base image (which we plan to rename cpp-clients-fat-base on the py ticking coming PR).

./docker/registry/r-client-base/build/crane/retag.sh
./docker/registry/protoc-base/build/crane/retag.sh
./docker/registry/slim-base/build/crane/retag.sh
./docker/registry/server-base/build/crane/retag.sh
107 changes: 107 additions & 0 deletions R/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
plugins {
id 'com.bmuschko.docker-remote-api'
id 'io.deephaven.project.register'
id 'io.deephaven.deephaven-in-docker'
}

evaluationDependsOn(':cpp-client')

configurations {
cpp {}
}

dependencies {
cpp project(':cpp-client')
}

def prefix = '/opt/deephaven'

// start a grpc-api server
String randomSuffix = UUID.randomUUID().toString();
deephavenDocker {
envVars.set([
'START_OPTS':'-Xmx512m -DAuthHandlers=io.deephaven.auth.AnonymousAuthenticationHandler'
])
containerName.set "dh-server-for-r-${randomSuffix}"
networkName.set "r-test-network-${randomSuffix}"
}

def buildRClient = Docker.registerDockerTask(project, 'rClient') {
// Only tested on x86-64, and we only build dependencies for x86-64
platform = 'linux/amd64'

copyIn {
from(layout.projectDirectory) {
include 'r-build.sh'
include 'r-tests.sh'
include 'rdeephaven/DESCRIPTION'
include 'rdeephaven/LICENSE'
include 'rdeephaven/NAMESPACE'
include 'rdeephaven/README.md'
include 'rdeephaven/inst/**'
jcferretti marked this conversation as resolved.
Show resolved Hide resolved
include 'rdeephaven/man/**'
include 'rdeephaven/etc/**'
include 'rdeephaven/R/**'
include 'rdeephaven/src/*.cpp'
include 'rdeephaven/src/Makevars'
}
niloc132 marked this conversation as resolved.
Show resolved Hide resolved
}
dockerfile {
from('deephaven/cpp-client:local-build')
//
// Build and install client.
//
runCommand("""mkdir -p \\
/out \\
${prefix}/log \\
${prefix}/bin/rdeephaven \\
${prefix}/src/rdeephaven/{inst,man,etc,src,R,bin}
""")
copyFile('rdeephaven/DESCRIPTION', "${prefix}/src/rdeephaven/")
copyFile('rdeephaven/LICENSE', "${prefix}/src/rdeephaven/")
copyFile('rdeephaven/NAMESPACE', "${prefix}/src/rdeephaven/")
copyFile('rdeephaven/README.md', "${prefix}/src/rdeephaven/")
copyFile('rdeephaven/inst/', "${prefix}/src/rdeephaven/inst/")
copyFile('rdeephaven/man/', "${prefix}/src/rdeephaven/man/")
copyFile('rdeephaven/etc/', "${prefix}/src/rdeephaven/etc/")
copyFile('rdeephaven/R/', "${prefix}/src/rdeephaven/R/")
copyFile('rdeephaven/src/*.cpp', "${prefix}/src/rdeephaven/src/")
copyFile('rdeephaven/src/Makevars', "${prefix}/src/rdeephaven/src/")
copyFile('r-tests.sh', "${prefix}/bin/rdeephaven")
copyFile('r-build.sh', "${prefix}/bin/rdeephaven")
runCommand("PREFIX=\"" + prefix + "\"; \\\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a strong preference for substitution in this way, injecting a groovy var into a sh env var, then consuming that sh env var (but not exporting it, so nothing else is using it)?

that is, something like this to reduce the escape-spam above

        runCommand("""set -eux ; \
                     cd "${prefix}/src/rdeephaven"; \
                     . "${prefix}/env.sh"; \
                     ${prefix}/bin/rdeephaven/r-build.sh
                   """)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this block is not as obvious, but if you look at the similar (longer, more env variables) block in cpp-client/build.gradle, the issue becomes apparent: Using double quotes and allowing for groovy to make string interpolation with its own (groovy's) variables makes using the regular syntax for shell environment variables in the shell code more complicated. It also makes it harder to read; while reading shell code I expect shell syntax to mean what it means in shell.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified the syntax of that first line a bit trying to capture the spirit of your comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double quotes were a deliberate change on my part, to ensure that no shell variables existed at all in this code, but that the shell was finished while still in groovy.

That said, this is just intended as an observation/idea/suggestion and not a requirement for merging.

'''set -eux ; \
cd "${PREFIX}/src/rdeephaven"; \
. "${PREFIX}/env.sh"; \
${PREFIX}/bin/rdeephaven/r-build.sh
''')
}
parentContainers = [ project.tasks.getByPath(':cpp-client:cppClient') ]
}
tasks.getByName('rClient').dependsOn project.tasks.getByPath(':cpp-client:cppClient')

def testRClient = Docker.registerDockerTask(project, 'testRClient') {
// Only tested on x86-64, and we only build dependencies for x86-64
platform = 'linux/amd64'
copyIn {}
copyOut {
into layout.buildDirectory.dir('test-results')
}
dockerfile {
from('deephaven/r-client:local-build')
//
// Setup for test run; we should be inheriting other env vars
// like LD_LIBRARY_PATH from the cpp-client image.
//
environmentVariable 'DH_HOST', deephavenDocker.containerName.get()
environmentVariable 'DH_PORT', '10000'
}
containerDependencies.dependsOn = [deephavenDocker.healthyTask]
containerDependencies.finalizedBy = deephavenDocker.endTask
network = deephavenDocker.networkName.get()
parentContainers = [ project.tasks.getByName('rClient') ]
entrypoint = ["${prefix}/bin/rdeephaven/r-tests.sh", '/out/r-test.xml', '/out/r-test.log']
}

deephavenDocker.shouldLogIfTaskFails testRClient
tasks.check.dependsOn(testRClient)
1 change: 1 addition & 0 deletions R/gradle.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
io.deephaven.project.ProjectType=BASIC
36 changes: 36 additions & 0 deletions R/r-build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#!/bin/bash

set -euo pipefail

for var in DHCPP NCPUS LD_LIBRARY_PATH; do
if [ -z "${!var}" ]; then
echo "$0: Environment variable $var is not set, aborting." 1>&2
exit 1
fi
done

if [ ! -d ./src ] || [ ! -f ./src/Makevars ] || [ ! -d ./R ] || [ ! -f ./DESCRIPTION ]; then
echo "The current directory `pwd` does not look like an R package source directory, aborting." 1>&2
exit 1
fi

# Ensure builds are always done from a clean slate.
trap 'rm -f src/*.o src/*.so' 1 2 15
rm -f src/*.o src/*.so

MAKE="make -j${NCPUS}" R --no-save --no-restore <<EOF
status = tryCatch(
{
install.packages(".", repos=NULL, type="source")
0
},
error=function(e) 1,
warning=function(w) 2
)
print(paste0('status=', status))
quit(save='no', status=status)
EOF

rm -f src/*.o src/*.so

exit 0
34 changes: 34 additions & 0 deletions R/r-tests.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#!/bin/bash

set -euo pipefail

if [ "$#" -ne 2 ]; then
echo "Usage: $0 out.xml out.log" 1>&2
exit 1
fi

if [ -z "${DH_PREFIX}" ]; then
echo "$0: Environment variable DH_PREFIX is not set, aborting." 1>&2
exit 1
fi

cd $DH_PREFIX/src/rdeephaven

OUT_XML="$1"
OUT_LOG="$2"

R --no-save --no-restore <<EOF >& "${OUT_LOG}"
library('testthat')
options(testthat.output_file = '${OUT_XML}')
status = tryCatch(
{
test_package('rdeephaven', reporter = 'junit')
0
},
error=function(e) 1
)
print(paste0('status=', status))
quit(save='no', status=status)
EOF

exit 0
11 changes: 11 additions & 0 deletions R/rdeephaven/inst/tests/testthat/helper.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
get_dh_target <- function() {
dh_host = Sys.getenv("DH_HOST")
if (dh_host == '') {
dh_host = "localhost"
}
dh_port = Sys.getenv("DH_PORT")
if (dh_port == '') {
dh_port = 10000
}
return(paste0(dh_host, ':', dh_port))
}
2 changes: 1 addition & 1 deletion R/rdeephaven/inst/tests/testthat/test_agg_by.R
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ setup <- function() {
)

# set up client
client <- Client$new(target = "localhost:10000")
client <- Client$new(target = get_dh_target())

# move dataframes to server and get TableHandles for testing
th1 <- client$import_table(df1)
Expand Down
3 changes: 1 addition & 2 deletions R/rdeephaven/inst/tests/testthat/test_client_wrapper.R
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
library(testthat)
library(rdeephaven)

target <- "localhost:10000"
target <- get_dh_target()

setup <- function() {
df1 <- data.frame(
Expand Down Expand Up @@ -31,7 +31,6 @@ setup <- function() {

test_that("client dhConnection works in the simple case of anonymous authentication", {

# TODO: assumes server is actually running on localhost:10000, this is probably bad for CI
expect_no_error(client <- Client$new(target = target))

})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ setup <- function() {
)

# set up client
client <- Client$new(target = "localhost:10000")
client <- Client$new(target = get_dh_target())

# move dataframes to server and get TableHandles for testing
th1 <- client$import_table(df1)
Expand Down
2 changes: 1 addition & 1 deletion R/rdeephaven/inst/tests/testthat/test_table_ops.R
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ setup <- function() {
)

# set up client
client <- Client$new(target = "localhost:10000")
client <- Client$new(target = get_dh_target())

# move dataframes to server and get TableHandles for testing
th1 <- client$import_table(df1)
Expand Down
2 changes: 1 addition & 1 deletion R/rdeephaven/inst/tests/testthat/test_update_by.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ setup <- function() {
)

# set up client
client <- Client$new(target = "localhost:10000")
client <- Client$new(target = get_dh_target())

# move dataframes to server and get TableHandles for testing
th1 <- client$import_table(df1)
Expand Down
1 change: 1 addition & 0 deletions cpp-client/.gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
*~
cmake-build-debug
cmake-build-release
cmake-build-relwithdebinfo
41 changes: 30 additions & 11 deletions cpp-client/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ plugins {
id 'license'
}

evaluationDependsOn Docker.registryProject('cpp-client-base')
// We use the r-client-base image instead of the cpp-client-base,
// so that the R client test task in R/build.gradle can in turn use the
// image we will generate here as a base.
evaluationDependsOn Docker.registryProject('r-client-base')

configurations {
cpp {}
Expand Down Expand Up @@ -57,10 +60,11 @@ deephavenDocker {
networkName.set "cpp-test-network-${randomSuffix}"
}

def testCppClient = Docker.registerDockerTask(project, 'testCppClient') {
def prefix = '/opt/deephaven'

def buildCppClientImage = Docker.registerDockerTask(project, 'cppClient') {
// Only tested on x86-64, and we only build dependencies for x86-64
platform = 'linux/amd64'

copyIn {
from(layout.projectDirectory) {
include 'cpp-tests-to-junit.sh'
Expand All @@ -71,12 +75,10 @@ def testCppClient = Docker.registerDockerTask(project, 'testCppClient') {
include 'deephaven/tests/**'
}
}
copyOut {
into layout.buildDirectory.dir('test-results')
}
def prefix = '/opt/deephaven'

dockerfile {
from('deephaven/cpp-client-base:local-build')
// See comment at the beginning of this file for why we use this base image.
from('deephaven/r-client-base:local-build')
//
// Build and install client.
//
Expand Down Expand Up @@ -108,19 +110,36 @@ def testCppClient = Docker.registerDockerTask(project, 'testCppClient') {
cd ..; \\
rm -fr ${PREFIX}/src/deephaven/build
''')
// Note environment variables defined here are inherited by other images
// using this image as a base.
environmentVariable 'DH_PREFIX', prefix
environmentVariable 'LD_LIBRARY_PATH', "${prefix}/lib"
}
parentContainers = [ Docker.registryTask(project, 'r-client-base') ]
}

def testCppClient = Docker.registerDockerTask(project, 'testCppClient') {
// Only tested on x86-64, and we only build dependencies for x86-64
platform = 'linux/amd64'
copyIn {}
copyOut {
into layout.buildDirectory.dir('test-results')
}
dockerfile {
from('deephaven/cpp-client:local-build')
//
// Setup for test run.
//
environmentVariable 'DH_HOST', deephavenDocker.containerName.get()
environmentVariable 'DH_PORT', '10000'
environmentVariable 'DH_PREFIX', prefix
environmentVariable 'LD_LIBRARY_PATH', "${prefix}/lib"
}
containerDependencies.dependsOn = [deephavenDocker.healthyTask]
containerDependencies.finalizedBy = deephavenDocker.endTask
network = deephavenDocker.networkName.get()
parentContainers = [ Docker.registryTask(project, 'cpp-client-base') ]
parentContainers = [ project.tasks.getByName('cppClient') ]
entrypoint = ["${prefix}/bin/dhcpp/cpp-tests-to-junit.sh", '/out/cpp-test.xml', '/out/cpp-test.log']
}
tasks.getByName('testCppClient').dependsOn project.tasks.getByName('cppClient')
jcferretti marked this conversation as resolved.
Show resolved Hide resolved

deephavenDocker.shouldLogIfTaskFails testCppClient
tasks.check.dependsOn(testCppClient)
2 changes: 1 addition & 1 deletion cpp-client/cpp-tests-to-junit.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ if [ "$#" -ne 2 ]; then
fi

if [ -z "${DH_PREFIX}" ]; then
echo "$0: Environment variable DHCPP_PREFIX is not set, aborting." 1>&2
echo "$0: Environment variable DH_PREFIX is not set, aborting." 1>&2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would DH_CPP_PREFIX not be more appropriate? DH_PREFIX is pretty generic, but since I don't really understand the full scope of this variable, feel free to say "no, I like what I did" / just resolve my comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it because we are installing there now more than just C++, eg, also R and potentially python/cython.

exit 1
fi

Expand Down
3 changes: 3 additions & 0 deletions docker/registry/r-client-base/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
plugins {
id 'io.deephaven.project.register'
}
4 changes: 4 additions & 0 deletions docker/registry/r-client-base/gradle.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
io.deephaven.project.ProjectType=DOCKER_REGISTRY
deephaven.registry.imageName=ghcr.io/deephaven/r-client-base:latest
deephaven.registry.imageId=ghcr.io/deephaven/r-client-base@sha256:29cb2969a746cf1f8556b469508a03796297afcfc6fa502622df372ae64d0039
deephaven.registry.platform=linux/amd64
3 changes: 3 additions & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ String[] mods = [
'go',
'authentication',
'cpp-client',
'R',
]

// new web projects; these modules are intended to form a complete, modular web application,
Expand Down Expand Up @@ -148,6 +149,8 @@ include(':codegen')

include(':cpp-client')

include(':R')

include(':replication-util')
project(':replication-util').projectDir = file('replication/util')

Expand Down