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
Changes from all 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
2 changes: 1 addition & 1 deletion .github/workflows/tag-base-images.yml
Original file line number Diff line number Diff line change
@@ -59,7 +59,7 @@ jobs:
- name: Tag upstream images
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.

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
111 changes: 111 additions & 0 deletions R/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
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}; " +
'''set -eux ; \
cd "${PREFIX}/src/rdeephaven"; \
. "${PREFIX}/env.sh"; \
${PREFIX}/bin/rdeephaven/r-build.sh
''')
}
parentContainers = [ 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 {
from(layout.projectDirectory) {
include 'r-tests.sh'
include 'rdeephaven/inst/**'
}
}
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
@@ -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)
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(
@@ -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))

})
Original file line number Diff line number Diff line change
@@ -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)
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
@@ -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)
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
@@ -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)
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
42 changes: 30 additions & 12 deletions cpp-client/build.gradle
Original file line number Diff line number Diff line change
@@ -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 {}
@@ -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'
@@ -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.
//
@@ -92,7 +94,7 @@ def testCppClient = Docker.registerDockerTask(project, 'testCppClient') {
copyFile('deephaven/examples/', "${prefix}/src/deephaven/examples/")
copyFile('deephaven/tests/', "${prefix}/src/deephaven/tests/")
copyFile('cpp-tests-to-junit.sh', "${prefix}/bin/dhcpp")
runCommand("PREFIX=\"" + prefix + "\"; \\\n" +
runCommand("PREFIX=${prefix}; " +
'''set -eux; \\
rm -fr ${PREFIX}/src/deephaven/build; \\
mkdir -p ${PREFIX}/src/deephaven/build; \\
@@ -108,19 +110,35 @@ 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']
}

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
@@ -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

13 changes: 0 additions & 13 deletions docker/registry/cpp-client-base/gradle.properties

This file was deleted.

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
Loading