R dockerized test run gradle target and CI #4625

jcferretti · 2023-10-11T02:15:10Z

…ut containers created.

jcferretti · 2023-10-11T02:17:18Z

Not ready for review yet, thus "draft".
At this point is mostly working:

cpp-client/build.gradle refactoring is working; separate targets for image building (that can be used from the R build) and test run work.
Need to add xml2 package to r-base-image for R junit test reporter to work https://github.com/deephaven/deephaven-base-images/pull/94/files
Need to fix dependency at the beginning of cpp-client/build.gradle that says evaluationDependsOn Docker.registryProject('cpp-client-base') to say evaluationDependsOn Docker.registryProject('r-client-base'). The later is failing and I don't understand why.

R/build.gradle

mofojed

I'm missing some context. What ticket is this fixing? It looks like this is just running tests, is there a reason it needs to make this release?

R/build.gradle

cpp-client/build.gradle

JamesXNelson · 2023-10-13T16:07:47Z

cpp-client/cpp-tests-to-junit.sh

-    echo "$0: Environment variable DHCPP_PREFIX is not set, aborting." 1>&2
+    echo "$0: Environment variable DH_PREFIX is not set, aborting." 1>&2


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.

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

devinrsmith

I made a bad change to R/rdeephaven/src/client.cpp to see what would happen, and gradle still reports "success".

Internally, I see:

ERROR: compilation failed for package 'rdeephaven'

I'm assuming we need something that will complete the loop when something internally goes wrong.

devinrsmith · 2023-10-13T17:20:10Z

.github/workflows/tag-base-images.yml

@@ -60,6 +60,7 @@ jobs:
        if: ${{ startsWith(github.ref, 'refs/heads/release/v') }}
        run: |
          ./docker/registry/cpp-client-base/build/crane/retag.sh


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

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.

…side R with proper exit status.

…igh.

jcferretti · 2023-10-15T03:39:56Z

I fixed the problem Devin's noted (thanks!) about a failure of compilation of the C++ code inside R's install.packages not triggering a gradle task failure. Interestingly enough, if some source fails compilation (which means the whole package won't build), install.packages only gives a warning... what? Sigh.

niloc132 · 2023-10-16T14:45:49Z

R/build.gradle

+        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" +


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 """)

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.

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

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.

R/build.gradle

…ndsOn).

R/build.gradle

jcferretti · 2023-10-16T16:28:49Z

I'm missing some context. What ticket is this fixing? It looks like this is just running tests, is there a reason it needs to make this release?

Discussed with Mike over slack. He said he is ok deferring to Devin and/or Ryan on the .github/workflows part of the change.

mofojed

I defer to Devin/Ryan for changes to the deephaven-core workflows.

…m deephaven-core repo.

Mike is deferring to Devin as per his message in slack and comment above.

alexpeters1208

I only looked at the R files. LGTM.

devinrsmith

Verified the points I care about - gradle now properly fails when error in R :)

JamesXNelson

Gradle wiring looks good to me

chipkent · 2023-10-16T17:41:29Z

.github/workflows/tag-base-images.yml

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


Did you mean to remove the cpp?

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).

jcferretti added 4 commits October 9, 2023 12:17

Draft.

26b3584

Merge branch 'main' into cfs-r-ci

f7046fe

Shaping up; cpp-client build image step now works; R test run fails b…

6568a65

…ut containers created.

Almost there... missing xml2 package in R for junit reporter.

5ba4a87

jcferretti added NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Oct 11, 2023

jcferretti added this to the October 2023 milestone Oct 11, 2023

jcferretti self-assigned this Oct 11, 2023

jcferretti requested a review from niloc132 October 11, 2023 02:17

alexpeters1208 reviewed Oct 11, 2023

View reviewed changes

R/build.gradle Outdated Show resolved Hide resolved

jcferretti and others added 8 commits October 11, 2023 20:37

Update gradle.properties

1a8ffdf

Updated r-client-base tag.

70b1642

Fix issues with right image.

184e923

Make R tests configurable for host:port.

9e90d53

Moving away from MAKEFLAGS into MAKE env var for R builds

4ba32b6

Merge branch 'main' into cfs-r-ci

37281a0

Now it works

ef1351c

New r-client-base image.

31a57c5

jcferretti marked this pull request as ready for review October 13, 2023 01:18

jcferretti requested review from chipkent, rcaudy, devinrsmith, JamesXNelson and mofojed as code owners October 13, 2023 01:18

mofojed previously requested changes Oct 13, 2023

View reviewed changes

alexpeters1208 previously approved these changes Oct 13, 2023

View reviewed changes

JamesXNelson reviewed Oct 13, 2023

View reviewed changes

devinrsmith reviewed Oct 13, 2023

View reviewed changes

Refactored the build code, separated image building, handle errors in…

852bb64

…side R with proper exit status.

jcferretti dismissed alexpeters1208’s stale review via 852bb64 October 15, 2023 03:33

R's install.packages only gives a warning on compilation failure... s…

0b6ddaf

…igh.

niloc132 reviewed Oct 16, 2023

View reviewed changes

R/build.gradle Show resolved Hide resolved

Followup to James/Colin review comments on dependsOn.

cc56455

niloc132 reviewed Oct 16, 2023

View reviewed changes

R/build.gradle Outdated Show resolved Hide resolved

Followup to the followup to the followup to review comments (ref depe…

cce7931

…ndsOn).

niloc132 reviewed Oct 16, 2023

View reviewed changes

R/build.gradle Show resolved Hide resolved

Followup to review comments.

5a880f2

jcferretti requested a review from mofojed October 16, 2023 16:26

mofojed reviewed Oct 16, 2023

View reviewed changes

jcferretti requested a review from mofojed October 16, 2023 16:30

Followup to review comments: remove references to cpp-client-base fro…

0634eb9

…m deephaven-core repo.

alexpeters1208 self-requested a review October 16, 2023 17:16

jcferretti requested review from devinrsmith and niloc132 October 16, 2023 17:17

alexpeters1208 approved these changes Oct 16, 2023

View reviewed changes

jcferretti requested a review from JamesXNelson October 16, 2023 17:25

devinrsmith approved these changes Oct 16, 2023

View reviewed changes

niloc132 approved these changes Oct 16, 2023

View reviewed changes

JamesXNelson approved these changes Oct 16, 2023

View reviewed changes

chipkent approved these changes Oct 16, 2023

View reviewed changes

jcferretti merged commit 39c5951 into deephaven:main Oct 16, 2023
9 checks passed

jcferretti deleted the cfs-r-ci branch October 16, 2023 17:44

github-actions bot locked and limited conversation to collaborators Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R dockerized test run gradle target and CI #4625

R dockerized test run gradle target and CI #4625

jcferretti commented Oct 11, 2023 •

edited

Loading

jcferretti commented Oct 11, 2023 •

edited

Loading

mofojed left a comment

JamesXNelson Oct 13, 2023

jcferretti Oct 13, 2023

devinrsmith left a comment

devinrsmith Oct 13, 2023

jcferretti Oct 15, 2023 •

edited

Loading

jcferretti commented Oct 15, 2023

niloc132 Oct 16, 2023

jcferretti Oct 16, 2023

jcferretti Oct 16, 2023

niloc132 Oct 16, 2023

jcferretti commented Oct 16, 2023

mofojed left a comment

alexpeters1208 left a comment

devinrsmith left a comment

JamesXNelson left a comment

chipkent Oct 16, 2023

jcferretti Oct 16, 2023

		echo "$0: Environment variable DHCPP_PREFIX is not set, aborting." 1>&2
		echo "$0: Environment variable DH_PREFIX is not set, aborting." 1>&2

R dockerized test run gradle target and CI #4625

R dockerized test run gradle target and CI #4625

Conversation

jcferretti commented Oct 11, 2023 • edited Loading

jcferretti commented Oct 11, 2023 • edited Loading

mofojed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcferretti Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

jcferretti commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcferretti commented Oct 16, 2023

mofojed left a comment

Choose a reason for hiding this comment

alexpeters1208 left a comment

Choose a reason for hiding this comment

devinrsmith left a comment

Choose a reason for hiding this comment

JamesXNelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcferretti commented Oct 11, 2023 •

edited

Loading

jcferretti commented Oct 11, 2023 •

edited

Loading

jcferretti Oct 15, 2023 •

edited

Loading