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

Add code to cpp-client/build.gradle to run tests in docker. #3006

Merged
merged 20 commits into from
Oct 21, 2022

Conversation

jcferretti
Copy link
Member

@jcferretti jcferretti commented Oct 20, 2022

This does not include yet code to automatically check consistency of generated protos, that should come in a later PR.

Summary of changes:

  • Removed build-dependencies.sh, now lives in the deephaven-base-images repository (https://github.com/deephaven/deephaven-base-images/tree/main/cpp-client). The script there is refactored some, to allow for building individual parts. Also made the script write a new env.sh file at the end with environment variables to source for, eg, CMAKE_PREFIX_PATH. Updated README.md to reflect the change.
  • In a separate PR for the deephaven-base-images repository, and with a lot of help from Devin (thanks!)
    we have added code to create a "cpp-client-base" image (code here) that contains all the dependent libraries already built
    in ubuntu-22.04. The code added to cpp-client/build.gradle to do checks via docker leans on that to avoid
    having to wait too long and create too many layer of the new image.
  • Small change to tests/test_util.cc to support changing the host:port for the server via setting environment variables DH_HOST and DH_PORT, needed for automating the tests.

@jcferretti jcferretti added this to the Oct 2022 milestone Oct 20, 2022
@jcferretti jcferretti self-assigned this Oct 20, 2022
devinrsmith added a commit to devinrsmith/deephaven-base-images that referenced this pull request Oct 20, 2022
devinrsmith added a commit to deephaven/deephaven-base-images that referenced this pull request Oct 20, 2022
kosak
kosak previously approved these changes Oct 21, 2022
Copy link
Contributor

@kosak kosak left a comment

Choose a reason for hiding this comment

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

Some micro-comments.

cpp-client/README.md Outdated Show resolved Hide resolved
cpp-examples/demos/crypto.cc Outdated Show resolved Hide resolved
cpp-client/tests/test_util.cc Outdated Show resolved Hide resolved
cpp-client/tests/test_util.cc Outdated Show resolved Hide resolved
cpp-examples/demos/crypto.cc Outdated Show resolved Hide resolved
cpp-client/build-dependencies.sh Outdated Show resolved Hide resolved
cpp-client/cpp-client-Dockerfile Outdated Show resolved Hide resolved
docker/registry/cpp-client-base/gradle.properties Outdated Show resolved Hide resolved
@jcferretti jcferretti changed the title Create a Dockerfile for cpp-client. Add code to cpp-client/build.gradle to run tests in docker. Add code to cpp-client/build.gradle to run tests in docker. Oct 21, 2022
cpp-client/build.gradle Show resolved Hide resolved
cpp-client/build.gradle Show resolved Hide resolved
cpp-client/build.gradle Outdated Show resolved Hide resolved
@devinrsmith
Copy link
Member

Just for reference, the base image will be our largest yet, topping in at over 3GB.

deephaven/cpp-client-base                                 local-build       2893a2a398e3   16 hours ago     3.11GB

@jcferretti
Copy link
Member Author

jcferretti commented Oct 21, 2022

Just for reference, the base image will be our largest yet, topping in at over 3GB.

deephaven/cpp-client-base                                 local-build       2893a2a398e3   16 hours ago     3.11GB

Devin and myself discussed making a docker ARG for image generation setting BUILD_TYPE=Release as default; the build-dependencies.sh script itself is not changing, but the way it is invoked from the dockerfile will change to use the ARG. This results in a more economical image:

cfs@guanaco 15:46:55 ~/dh/oss1/deephaven-base-images
$ docker images | grep cpp-client-base
ghcr.io/deephaven/cpp-client-base                         latest        071706e8d34c   26 seconds ago   689MB

I tried that locally, I am updating the PR for deephaven-base-images right after writing this.

@jcferretti jcferretti merged commit c86e7a8 into deephaven:main Oct 21, 2022
@jcferretti jcferretti deleted the cfs-cpp-client-dockerfile branch October 21, 2022 21:26
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants